Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Writing Flow]: Try to fix multi-selection with shift+click #40687

Merged
merged 5 commits into from
Apr 29, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@ import { store as blockEditorStore } from '../../store';
import { getBlockClientId } from '../../utils/dom';

export default function useClickSelection() {
const { multiSelect, selectBlock } = useDispatch( blockEditorStore );
const { selectBlock } = useDispatch( blockEditorStore );
const {
isSelectionEnabled,
getBlockParents,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are unrelated - just some cleaning up.

getBlockSelectionStart,
hasMultiSelection,
} = useSelect( blockEditorStore );
Expand Down Expand Up @@ -54,10 +53,8 @@ export default function useClickSelection() {
};
},
[
multiSelect,
selectBlock,
isSelectionEnabled,
getBlockParents,
getBlockSelectionStart,
hasMultiSelection,
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,43 +76,81 @@ export default function useSelectionObserver() {
const { multiSelect, selectBlock, selectionChange } = useDispatch(
blockEditorStore
);
const { getBlockParents } = useSelect( blockEditorStore );
const { getBlockParents, getBlockSelectionStart } = useSelect(
blockEditorStore
);
return useRefEffect(
( node ) => {
const { ownerDocument } = node;
const { defaultView } = ownerDocument;

function onSelectionChange() {
function onSelectionChange( event ) {
const selection = defaultView.getSelection();

// If no selection is found, end multi selection and disable the
// contentEditable wrapper.
if ( ! selection.rangeCount || selection.isCollapsed ) {
if ( ! selection.rangeCount ) {
setContentEditableWrapper( node, false );
return;
}

const clientId = getBlockClientId(
let startClientId = getBlockClientId(
extractSelectionStartNode( selection )
);
const endClientId = getBlockClientId(
let endClientId = getBlockClientId(
extractSelectionEndNode( selection )
);
// If the selection has changed and we had pressed `shift+click`,
// we need to check if in an element that doesn't support
// selection has been clicked.
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
if ( event.shiftKey ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this code should also check that it was a mouse event. While shiftKey doesn't seem to be defined for selectionchange events, I'm pretty sure it can be for other keyboard events. Event payloads can also be quite inconsistent between browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an extra check about being a mouseup event for now and let's see how this goes 😄

const selectedClientId = getBlockSelectionStart();
const clickedClientId = getBlockClientId( event.target );
// `endClientId` is not defined if we end the selection by clicking a non-selectable block.
// We need to check if there was already a selection with a non-selectable focusNode.
const focusNodeIsNonSelectable =
clickedClientId !== endClientId;
if (
( startClientId === endClientId &&
selection.isCollapsed ) ||
! endClientId ||
focusNodeIsNonSelectable
) {
endClientId = clickedClientId;
}
// Handle the case when we have a non-selectable block and
// click another one.
if ( startClientId !== selectedClientId ) {
startClientId = selectedClientId;
}
}

// If the selection did not involve a block, return early.
if ( clientId === undefined && endClientId === undefined ) {
const isSingularSelection = startClientId === endClientId;
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
// If selection is collapsed, end multi selection and disable the
// contentEditable wrapper.
// We also check if we have updated the clientIds when holding `shift`
// and elements that doesn't support selection are involved.
// There might be the case that the selection is collapsed but we need
// to multi-select blocks.
if ( selection.isCollapsed && isSingularSelection ) {
setContentEditableWrapper( node, false );
return;
}

const isSingularSelection = clientId === endClientId;
// If the selection did not involve a block, return early.
if (
startClientId === undefined &&
endClientId === undefined
) {
setContentEditableWrapper( node, false );
return;
}

if ( isSingularSelection ) {
selectBlock( clientId );
selectBlock( startClientId );
} else {
const startPath = [
...getBlockParents( clientId ),
clientId,
...getBlockParents( startClientId ),
startClientId,
];
const endPath = [
...getBlockParents( endClientId ),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ export function useInputAndSelection( props ) {
function onCompositionStart() {
isComposing = true;
// Do not update the selection when characters are being composed as
// this rerenders the component and might distroy internal browser
// this rerenders the component and might destroy internal browser
// editing state.
ownerDocument.removeEventListener(
'selectionchange',
Expand Down