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: absorb partial multi selection dispatching #47525

Merged
merged 9 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,10 +317,13 @@ export function RichTextWrapper(
{ ...props }
{ ...autocompleteProps }
ref={ useMergeRefs( [
// Rich text ref must be first because its focus listener
// must be set up before any other ref calls .focus() on
// mount.
richTextRef,
forwardedRef,
autocompleteProps.ref,
props.ref,
richTextRef,
useBeforeInputRules( { value, onChange } ),
useInputRules( {
getValue,
Expand Down Expand Up @@ -379,6 +382,7 @@ export function RichTextWrapper(
// tabIndex because Safari will focus the element. However,
// Safari will correctly ignore nested contentEditable elements.
tabIndex={ props.tabIndex === 0 ? null : props.tabIndex }
data-wp-block-attribute-key={ identifier }
/>
</>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ export function useWritingFlow() {
useRefEffect(
( node ) => {
node.tabIndex = 0;
node.contentEditable = hasMultiSelection;

if ( ! hasMultiSelection ) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ function setContentEditableWrapper( node, value ) {
export default function useDragSelection() {
const { startMultiSelect, stopMultiSelect } =
useDispatch( blockEditorStore );
const { isSelectionEnabled, hasMultiSelection, isDraggingBlocks } =
useSelect( blockEditorStore );
const {
isSelectionEnabled,
hasSelectedBlock,
isDraggingBlocks,
isMultiSelecting,
} = useSelect( blockEditorStore );
return useRefEffect(
( node ) => {
const { ownerDocument } = node;
Expand All @@ -45,7 +49,7 @@ export default function useDragSelection() {
// so wait until the next animation frame to get the browser
// selection.
rafId = defaultView.requestAnimationFrame( () => {
if ( hasMultiSelection() ) {
if ( ! hasSelectedBlock() ) {
return;
}

Expand Down Expand Up @@ -84,6 +88,16 @@ export default function useDragSelection() {
return;
}

// Abort if we are already multi-selecting.
if ( isMultiSelecting() ) {
return;
Copy link
Contributor

@ntsekouras ntsekouras Dec 12, 2023

Choose a reason for hiding this comment

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

This is the reason we can't continue with the selection after having started a multi selection, right?

Screen.Recording.2023-12-12.at.11.18.11.AM.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this :)
It's actually a bit broken in trunk too

}

// Abort if selection is leaving writing flow.
if ( node === target ) {
return;
}

// Check the attribute, not the contentEditable attribute. All
// child elements of the content editable wrapper are editable
// and return true for this property. We only want to start
Expand Down Expand Up @@ -127,7 +141,7 @@ export default function useDragSelection() {
startMultiSelect,
stopMultiSelect,
isSelectionEnabled,
hasMultiSelection,
hasSelectedBlock,
]
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { useRefEffect } from '@wordpress/compose';
import { create } from '@wordpress/rich-text';

/**
* Internal dependencies
Expand Down Expand Up @@ -75,10 +76,20 @@ function setContentEditableWrapper( node, value ) {
// Since we are calling this on every selection change, check if the value
// needs to be updated first because it trigger the browser to recalculate
// style.
if ( node.contentEditable !== String( value ) )
if ( node.contentEditable !== String( value ) ) {
node.contentEditable = value;
// Firefox doesn't automatically move focus.
if ( value ) node.focus();

// Firefox doesn't automatically move focus.
if ( value ) {
node.focus();
}
}
}

function getRichTextElement( node ) {
const element =
node.nodeType === node.ELEMENT_NODE ? node : node.parentElement;
return element?.closest( '[data-wp-block-attribute-key]' );
}

/**
Expand All @@ -87,7 +98,7 @@ function setContentEditableWrapper( node, value ) {
export default function useSelectionObserver() {
const { multiSelect, selectBlock, selectionChange } =
useDispatch( blockEditorStore );
const { getBlockParents, getBlockSelectionStart } =
const { getBlockParents, getBlockSelectionStart, isMultiSelecting } =
useSelect( blockEditorStore );
return useRefEffect(
( node ) => {
Expand All @@ -101,6 +112,16 @@ export default function useSelectionObserver() {
return;
}

const startNode = extractSelectionStartNode( selection );
const endNode = extractSelectionEndNode( selection );

if (
! node.contains( startNode ) ||
! node.contains( endNode )
) {
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
Expand All @@ -109,16 +130,24 @@ export default function useSelectionObserver() {
// 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 );
if (
node.contentEditable === 'true' &&
! isMultiSelecting()
) {
setContentEditableWrapper( node, false );
let element =
startNode.nodeType === startNode.ELEMENT_NODE
? startNode
: startNode.parentElement;
element = element?.closest( '[contenteditable]' );
element?.focus();
}
return;
}

let startClientId = getBlockClientId(
extractSelectionStartNode( selection )
);
let endClientId = getBlockClientId(
extractSelectionEndNode( selection )
);
let startClientId = getBlockClientId( startNode );
let endClientId = getBlockClientId( endNode );

// 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.
Expand Down Expand Up @@ -155,7 +184,11 @@ export default function useSelectionObserver() {

const isSingularSelection = startClientId === endClientId;
if ( isSingularSelection ) {
selectBlock( startClientId );
if ( ! isMultiSelecting() ) {
selectBlock( startClientId );
} else {
multiSelect( startClientId, startClientId );
}
} else {
const startPath = [
...getBlockParents( startClientId ),
Expand All @@ -167,39 +200,68 @@ export default function useSelectionObserver() {
];
const depth = findDepth( startPath, endPath );

multiSelect( startPath[ depth ], endPath[ depth ] );
}
}
if (
startPath[ depth ] !== startClientId ||
endPath[ depth ] !== endClientId
) {
multiSelect( startPath[ depth ], endPath[ depth ] );
return;
}

function addListeners() {
ownerDocument.addEventListener(
'selectionchange',
onSelectionChange
);
defaultView.addEventListener( 'mouseup', onSelectionChange );
const richTextElementStart =
getRichTextElement( startNode );
const richTextElementEnd = getRichTextElement( endNode );

if ( richTextElementStart && richTextElementEnd ) {
const range = selection.getRangeAt( 0 );
const richTextDataStart = create( {
element: richTextElementStart,
range,
__unstableIsEditableTree: true,
} );
const richTextDataEnd = create( {
element: richTextElementEnd,
range,
__unstableIsEditableTree: true,
} );

const startOffset =
richTextDataStart.start ?? richTextDataStart.end;
const endOffset =
richTextDataEnd.start ?? richTextDataEnd.end;
selectionChange( {
start: {
clientId: startClientId,
attributeKey:
richTextElementStart.dataset
.wpBlockAttributeKey,
offset: startOffset,
},
end: {
clientId: endClientId,
attributeKey:
richTextElementEnd.dataset
.wpBlockAttributeKey,
offset: endOffset,
},
} );
} else {
multiSelect( startClientId, endClientId );
}
}
}

function removeListeners() {
ownerDocument.addEventListener(
'selectionchange',
onSelectionChange
);
defaultView.addEventListener( 'mouseup', onSelectionChange );
return () => {
ownerDocument.removeEventListener(
'selectionchange',
onSelectionChange
);
defaultView.removeEventListener( 'mouseup', onSelectionChange );
}

function resetListeners() {
removeListeners();
addListeners();
}

addListeners();
// We must allow rich text to set selection first. This ensures that
// our `selectionchange` listener is always reset to be called after
// the rich text one.
node.addEventListener( 'focusin', resetListeners );
return () => {
removeListeners();
node.removeEventListener( 'focusin', resetListeners );
};
},
[ multiSelect, selectBlock, selectionChange, getBlockParents ]
Expand Down
65 changes: 15 additions & 50 deletions packages/rich-text/src/component/use-input-and-selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,48 +126,14 @@ export function useInputAndSelection( props ) {
return;
}

// If the selection changes where the active element is a parent of
// the rich text instance (writing flow), call `onSelectionChange`
// for the rich text instance that contains the start or end of the
// selection.
// Ensure the active element is the rich text element.
if ( ownerDocument.activeElement !== element ) {
// Only process if the active elment is contentEditable, either
// this rich text instance or the writing flow parent. Fixes a
// bug in Firefox where it strangely selects the closest
// contentEditable element, even though the click was outside
// any contentEditable element.
if ( ownerDocument.activeElement.contentEditable !== 'true' ) {
return;
}

if ( ! ownerDocument.activeElement.contains( element ) ) {
return;
}

const selection = defaultView.getSelection();
const { anchorNode, focusNode } = selection;

if (
element.contains( anchorNode ) &&
element !== anchorNode &&
element.contains( focusNode ) &&
element !== focusNode
) {
const { start, end } = createRecord();
record.current.activeFormats = EMPTY_ACTIVE_FORMATS;
onSelectionChange( start, end );
} else if (
element.contains( anchorNode ) &&
element !== anchorNode
) {
const { start, end: offset = start } = createRecord();
record.current.activeFormats = EMPTY_ACTIVE_FORMATS;
onSelectionChange( offset );
} else if ( element.contains( focusNode ) ) {
const { start, end: offset = start } = createRecord();
record.current.activeFormats = EMPTY_ACTIVE_FORMATS;
onSelectionChange( undefined, offset );
}
// If it is not, we can stop listening for selection changes.
// We resume listening when the element is focused.
ownerDocument.removeEventListener(
'selectionchange',
handleSelectionChange
);
return;
}

Expand Down Expand Up @@ -276,18 +242,21 @@ export function useInputAndSelection( props ) {
};
} else {
applyRecord( record.current );
onSelectionChange( record.current.start, record.current.end );
}

onSelectionChange( record.current.start, record.current.end );

ownerDocument.addEventListener(
'selectionchange',
handleSelectionChange
);
}

element.addEventListener( 'input', onInput );
element.addEventListener( 'compositionstart', onCompositionStart );
element.addEventListener( 'compositionend', onCompositionEnd );
element.addEventListener( 'focus', onFocus );
ownerDocument.addEventListener(
'selectionchange',
handleSelectionChange
);

return () => {
element.removeEventListener( 'input', onInput );
element.removeEventListener(
Expand All @@ -296,10 +265,6 @@ export function useInputAndSelection( props ) {
);
element.removeEventListener( 'compositionend', onCompositionEnd );
element.removeEventListener( 'focus', onFocus );
ownerDocument.removeEventListener(
'selectionchange',
handleSelectionChange
);
};
}, [] );
}
10 changes: 10 additions & 0 deletions test/e2e/specs/editor/various/multi-block-selection.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,16 @@ test.describe( 'Multi-block selection', () => {
force: true,
} );

await expect
.poll( multiBlockSelectionUtils.getSelectedFlatIndices )
.toEqual( [ 1 ] );

await paragraph1.click( {
position: { x: -1, y: 0 },
// Use force since it's outside the bounding box of the element.
force: true,
} );
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I am merely making things consistent here with the Esc shortcut:

// If there is more than one block selected, select the first
// block so that focus is directed back to the beginning of the selection.
// In effect, to the user this feels like deselecting the multi-selection.
if ( clientIds.length > 1 ) {
selectBlock( clientIds[ 0 ] );
} else {
clearSelectedBlock();
}


await expect
.poll( () =>
page.evaluate( () => window.getSelection().rangeCount )
Expand Down
Loading