From da13a02c9dc89be167129aae27ade0449d1f64f8 Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Thu, 28 Apr 2022 15:54:16 +0300 Subject: [PATCH 1/5] [Writing Flow]: Try to fix multi-selection with `shift+click` --- .../writing-flow/use-click-selection.js | 5 +- .../writing-flow/use-selection-observer.js | 62 +++++++++++++++---- .../src/component/use-input-and-selection.js | 2 +- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/packages/block-editor/src/components/writing-flow/use-click-selection.js b/packages/block-editor/src/components/writing-flow/use-click-selection.js index 3d7aec8d4ea4c..6bd043635052a 100644 --- a/packages/block-editor/src/components/writing-flow/use-click-selection.js +++ b/packages/block-editor/src/components/writing-flow/use-click-selection.js @@ -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, getBlockSelectionStart, hasMultiSelection, } = useSelect( blockEditorStore ); @@ -54,10 +53,8 @@ export default function useClickSelection() { }; }, [ - multiSelect, selectBlock, isSelectionEnabled, - getBlockParents, getBlockSelectionStart, hasMultiSelection, ] diff --git a/packages/block-editor/src/components/writing-flow/use-selection-observer.js b/packages/block-editor/src/components/writing-flow/use-selection-observer.js index 6aabb324274f6..6773c433238d8 100644 --- a/packages/block-editor/src/components/writing-flow/use-selection-observer.js +++ b/packages/block-editor/src/components/writing-flow/use-selection-observer.js @@ -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. + if ( event.shiftKey ) { + 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; + // 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 ), diff --git a/packages/rich-text/src/component/use-input-and-selection.js b/packages/rich-text/src/component/use-input-and-selection.js index c1c36808d5996..b8b7d4f16c66e 100644 --- a/packages/rich-text/src/component/use-input-and-selection.js +++ b/packages/rich-text/src/component/use-input-and-selection.js @@ -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', From ca3c9613019209f477a10972df6ebde4ce70ea84 Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Thu, 28 Apr 2022 22:31:00 +0300 Subject: [PATCH 2/5] change check with collapsed selection --- .../writing-flow/use-selection-observer.js | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/block-editor/src/components/writing-flow/use-selection-observer.js b/packages/block-editor/src/components/writing-flow/use-selection-observer.js index 6773c433238d8..f0f2ef3c16172 100644 --- a/packages/block-editor/src/components/writing-flow/use-selection-observer.js +++ b/packages/block-editor/src/components/writing-flow/use-selection-observer.js @@ -92,6 +92,15 @@ export default function useSelectionObserver() { setContentEditableWrapper( node, false ); return; } + // If selection is collapsed and we haven't used `shift+click`, + // end multi selection and disable the contentEditable wrapper. + // We have to check about `shift+click` case because elements + // that doesn't support selection might be involved, and we might + // update the clientIds to multi-select blocks. + if ( selection.isCollapsed && ! event.shiftKey ) { + setContentEditableWrapper( node, false ); + return; + } let startClientId = getBlockClientId( extractSelectionStartNode( selection ) @@ -117,26 +126,16 @@ export default function useSelectionObserver() { ) { endClientId = clickedClientId; } - // Handle the case when we have a non-selectable block and - // click another one. + // Handle the case when we have a non-selectable block + // selected and click another one. if ( startClientId !== selectedClientId ) { startClientId = selectedClientId; } } const isSingularSelection = startClientId === endClientId; - // 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; - } - // If the selection did not involve a block, return early. + // If the selection did not involve a block, return. if ( startClientId === undefined && endClientId === undefined From f32b326c5cb10eb2cdc1a0e9abb99ee278ae903a Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Fri, 29 Apr 2022 09:57:19 +0300 Subject: [PATCH 3/5] address feedback --- .../src/components/writing-flow/use-selection-observer.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/components/writing-flow/use-selection-observer.js b/packages/block-editor/src/components/writing-flow/use-selection-observer.js index f0f2ef3c16172..48fccdf8c61a5 100644 --- a/packages/block-editor/src/components/writing-flow/use-selection-observer.js +++ b/packages/block-editor/src/components/writing-flow/use-selection-observer.js @@ -95,7 +95,7 @@ export default function useSelectionObserver() { // If selection is collapsed and we haven't used `shift+click`, // end multi selection and disable the contentEditable wrapper. // We have to check about `shift+click` case because elements - // that doesn't support selection might be involved, and we might + // that don't support text selection might be involved, and we might // update the clientIds to multi-select blocks. if ( selection.isCollapsed && ! event.shiftKey ) { setContentEditableWrapper( node, false ); @@ -110,7 +110,7 @@ export default function useSelectionObserver() { ); // 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. + // text selection has been clicked. if ( event.shiftKey ) { const selectedClientId = getBlockSelectionStart(); const clickedClientId = getBlockClientId( event.target ); @@ -133,8 +133,6 @@ export default function useSelectionObserver() { } } - const isSingularSelection = startClientId === endClientId; - // If the selection did not involve a block, return. if ( startClientId === undefined && @@ -144,6 +142,7 @@ export default function useSelectionObserver() { return; } + const isSingularSelection = startClientId === endClientId; if ( isSingularSelection ) { selectBlock( startClientId ); } else { From d3e9c7a142d6124a3b18791a2ff7d01c132d2675 Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Fri, 29 Apr 2022 10:17:22 +0300 Subject: [PATCH 4/5] target specific mouse event with shift --- .../src/components/writing-flow/use-selection-observer.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/writing-flow/use-selection-observer.js b/packages/block-editor/src/components/writing-flow/use-selection-observer.js index 48fccdf8c61a5..2f37a2c4bed34 100644 --- a/packages/block-editor/src/components/writing-flow/use-selection-observer.js +++ b/packages/block-editor/src/components/writing-flow/use-selection-observer.js @@ -97,7 +97,9 @@ export default function useSelectionObserver() { // We have to check about `shift+click` case because elements // that don't support text selection might be involved, and we might // update the clientIds to multi-select blocks. - if ( selection.isCollapsed && ! event.shiftKey ) { + // For now we check if the event is a `mouse` event. + const isClickShift = event.shiftKey && event.type === 'mouseup'; + if ( selection.isCollapsed && ! isClickShift ) { setContentEditableWrapper( node, false ); return; } @@ -111,7 +113,7 @@ export default function useSelectionObserver() { // If the selection has changed and we had pressed `shift+click`, // we need to check if in an element that doesn't support // text selection has been clicked. - if ( event.shiftKey ) { + if ( isClickShift ) { const selectedClientId = getBlockSelectionStart(); const clickedClientId = getBlockClientId( event.target ); // `endClientId` is not defined if we end the selection by clicking a non-selectable block. From a661620a776793b624e29d2fae1a516cefdd749e Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Fri, 29 Apr 2022 11:08:43 +0300 Subject: [PATCH 5/5] add e2e tests --- .../various/multi-block-selection.test.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/packages/e2e-tests/specs/editor/various/multi-block-selection.test.js b/packages/e2e-tests/specs/editor/various/multi-block-selection.test.js index 226a801446cf5..2a98d2aec100e 100644 --- a/packages/e2e-tests/specs/editor/various/multi-block-selection.test.js +++ b/packages/e2e-tests/specs/editor/various/multi-block-selection.test.js @@ -10,6 +10,7 @@ import { clickBlockToolbarButton, clickButton, clickMenuItem, + insertBlock, openListView, saveDraft, transformBlockTo, @@ -975,4 +976,51 @@ describe( 'Multi-block selection', () => { // Expect two blocks with "&" in between. expect( await getEditedPostContent() ).toMatchSnapshot(); } ); + describe( 'shift+click multi-selection', () => { + it( 'should multi-select block with text selection and a block without text selection', async () => { + await page.keyboard.press( 'Enter' ); + await page.keyboard.type( 'hi' ); + await page.keyboard.press( 'Enter' ); + await insertBlock( 'Spacer' ); + await page.keyboard.press( 'ArrowUp' ); + + const spacerBlock = await page.waitForSelector( + '.wp-block.wp-block-spacer' + ); + const boundingBox = await spacerBlock.boundingBox(); + const mousePosition = { + x: boundingBox.x + boundingBox.width / 2, + y: boundingBox.y + boundingBox.height / 2, + }; + await page.keyboard.down( 'Shift' ); + await page.mouse.click( mousePosition.x, mousePosition.y ); + await page.keyboard.up( 'Shift' ); + + const selectedBlocks = await page.$$( + '.wp-block.is-multi-selected' + ); + expect( selectedBlocks.length ).toBe( 2 ); + } ); + it( 'should multi-select blocks without text selection', async () => { + await insertBlock( 'Spacer' ); + // Get the first spacer block element. + const spacerBlock = await page.waitForSelector( + '.wp-block.wp-block-spacer' + ); + const boundingBox = await spacerBlock.boundingBox(); + await page.keyboard.press( 'Enter' ); + await insertBlock( 'Spacer' ); + const mousePosition = { + x: boundingBox.x + boundingBox.width / 2, + y: boundingBox.y + boundingBox.height / 2, + }; + await page.keyboard.down( 'Shift' ); + await page.mouse.click( mousePosition.x, mousePosition.y ); + await page.keyboard.up( 'Shift' ); + const selectedBlocks = await page.$$( + '.wp-block.is-multi-selected' + ); + expect( selectedBlocks.length ).toBe( 2 ); + } ); + } ); } );