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.

Sign up for GitHub

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

Move navigation and selection logic to WritingFlow #19397

Merged
merged 16 commits into from
Jan 8, 2020
Merged

Move navigation and selection logic to WritingFlow #19397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f4727bc
WritingFlow: absorb navigation logic from block
ellatrix Jan 3, 2020
5c1bdba
Move selection and navigation logic to WritingFlow
ellatrix Jan 3, 2020
5fb2ddf
Use getBlockClientId in useMultiSelection
ellatrix Jan 3, 2020
dadd96d
Fix block dragging
ellatrix Jan 3, 2020
815e7ca
Fix focus after escape to nav mode
ellatrix Jan 3, 2020
8178ca9
Remove IgnoreNestedEvents
ellatrix Jan 3, 2020
bdd923f
Add getBlockClientId docs
ellatrix Jan 3, 2020
43ca6ea
Restore tabindex comment
ellatrix Jan 3, 2020
9ad9c44
Fix nested block appender click
ellatrix Jan 3, 2020
90d30ca
Move focus and drag handling
ellatrix Jan 3, 2020
c1239e3
Create root container for block list
ellatrix Jan 4, 2020
dbfb783
Polish
ellatrix Jan 4, 2020
6c6acc0
Conditionally bind block event handlers
ellatrix Jan 4, 2020
82be361
Ensure multiselection has 2 items before selecting
ellatrix Jan 6, 2020
3ddc1d5
Update packages/block-editor/src/utils/dom.js
ellatrix Jan 7, 2020
0491a00
Adjust onFocus comment
ellatrix Jan 8, 2020
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
3 changes: 2 additions & 1 deletion packages/block-editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,8 @@ _Returns_

<a name="WritingFlow" href="#WritingFlow">#</a> **WritingFlow**

Undocumented declaration.
Handles selection and navigation across blocks. This component should be
wrapped around BlockList.


<!-- END TOKEN(Autogenerated API docs) -->
Expand Down
41 changes: 21 additions & 20 deletions packages/block-editor/src/components/block-list-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ import { getDefaultBlockName } from '@wordpress/blocks';
/**
* Internal dependencies
*/
import IgnoreNestedEvents from '../ignore-nested-events';
import DefaultBlockAppender from '../default-block-appender';
import ButtonBlockAppender from '../button-block-appender';

function stopPropagation( event ) {
event.stopPropagation();
}

function BlockListAppender( {
blockClientIds,
rootClientId,
Expand Down Expand Up @@ -51,26 +54,24 @@ function BlockListAppender( {
);
}

// IgnoreNestedEvents is used to treat interactions within the appender as
// subject to the same conditions as those which occur within nested blocks.
// Notably, this effectively prevents event bubbling to block ancestors
// which can otherwise interfere with the intended behavior of the appender
// (e.g. focus handler on the ancestor block).
//
// A `tabIndex` is used on the wrapping `div` element in order to force a
// focus event to occur when an appender `button` element is clicked. In
// some browsers (Firefox, Safari), button clicks do not emit a focus event,
// which could cause this event to propagate unexpectedly. The `tabIndex`
// ensures that the interaction is captured as a focus, without also adding
// an extra tab stop.
//
// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
return (
<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
<div tabIndex={ -1 } className="block-list-appender">
{ appender }
</div>
</IgnoreNestedEvents>
<div
// A `tabIndex` is used on the wrapping `div` element in order to
// force a focus event to occur when an appender `button` element
// is clicked. In some browsers (Firefox, Safari), button clicks do
// not emit a focus event, which could cause this event to propagate
// unexpectedly. The `tabIndex` ensures that the interaction is
// captured as a focus, without also adding an extra tab stop.
//
// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
tabIndex={ -1 }
// Prevent the block from being selected when the appender is
// clicked.
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

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

I was actually surprised to find that in the latest version of the plugin / core version, the behavior of clicking on the inserter in the column block is such that it requires a second click to open. I don't think it's always been like this, since I distinctly remember trying to require only a single click. One potential difference now is that, while it does only require a single click, it doesn't actually select the block. I don't particularly see this as being problematic, and in some ways it's actually an ideal behavior (since the user's intent is to insert, not necessarily to select the block).

onFocus={ stopPropagation }
Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth This fixed the issue you found (#19397 (comment)). Does everything now work correctly?

Copy link
Member

Choose a reason for hiding this comment

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

Initial impression: I imagine this could work, yes, considering that IgnoreNestedEvents was in many ways a glorified stopPropagation anyways. Will test as part of a broader re-review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! It would be nice the eventually get rid of this as well. I'll look into it sometime separately.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! It would be nice the eventually get rid of this as well. I'll look into it sometime separately.

Yeah, that's fair. I'm very reluctant about any stopPropagation, but I think it's fair to say it's not really "introduced" here, since it was more-or-less the same effect previously.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, same here. At least it's now explicit that propagation is stopped here. IgnoreNestedEvents kind of made it look like it's more ok.

Copy link
Member

Choose a reason for hiding this comment

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

Right, same here. At least it's now explicit that propagation is stopped here. IgnoreNestedEvents kind of made it look like it's more ok.

It's a give and take, I think. One nice thing about IgnoreNestedEvents is that it wouldn't stop all propagation, only that which it expressly cares about, thus potentially avoiding issues with other (intended) event handling further up the element hierarchy.

className="block-list-appender"
>
{ appender }
</div>
);
}

Expand Down
124 changes: 16 additions & 108 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import { animated } from 'react-spring/web.cjs';
/**
* WordPress dependencies
*/
import { useRef, useEffect, useLayoutEffect, useState, useCallback } from '@wordpress/element';
import { useRef, useEffect, useLayoutEffect, useState, useCallback, useContext } from '@wordpress/element';
import {
focus,
isTextField,
placeCaretAtHorizontalEdge,
} from '@wordpress/dom';
import { BACKSPACE, DELETE, ENTER, ESCAPE } from '@wordpress/keycodes';
import { BACKSPACE, DELETE, ENTER } from '@wordpress/keycodes';
import {
getBlockType,
getSaveElement,
Expand Down Expand Up @@ -45,20 +45,11 @@ import BlockHtml from './block-html';
import BlockBreadcrumb from './breadcrumb';
import BlockContextualToolbar from './block-contextual-toolbar';
import BlockInsertionPoint from './insertion-point';
import IgnoreNestedEvents from '../ignore-nested-events';
import Inserter from '../inserter';
import { isInsideRootBlock } from '../../utils/dom';
import useMovingAnimation from './moving-animation';
import { ChildToolbar, ChildToolbarSlot } from './block-child-toolbar';
/**
* Prevents default dragging behavior within a block to allow for multi-
* selection to take effect unhampered.
*
* @param {DragEvent} event Drag event.
*/
const preventDrag = ( event ) => {
event.preventDefault();
};
import { Context } from './root-container';

function BlockListBlock( {
mode,
Expand Down Expand Up @@ -94,17 +85,15 @@ function BlockListBlock( {
onRemove,
onInsertDefaultBlockAfter,
toggleSelection,
onShiftSelection,
onSelectionStart,
animateOnChange,
enableAnimation,
isNavigationMode,
setNavigationMode,
isMultiSelecting,
isLargeViewport,
hasSelectedUI = true,
hasMovers = true,
} ) {
const onSelectionStart = useContext( Context );
// In addition to withSelect, we should favor using useSelect in this component going forward
// to avoid leaking new props to the public API (editor.BlockListBlock filter)
const { isDraggingBlocks } = useSelect( ( select ) => {
Expand Down Expand Up @@ -231,17 +220,6 @@ function BlockListBlock( {

// Other event handlers

/**
* Marks the block as selected when focused and not already selected. This
* specifically handles the case where block does not set focus on its own
* (via `setFocus`), typically if there is no focusable input in the block.
*/
const onFocus = () => {
if ( ! isSelected && ! isPartOfMultiSelection ) {
onSelect();
}
};

/**
* Interprets keydown event intent to remove or insert after block if key
* event occurs on wrapper node. This can occur when the block has no text
Expand All @@ -253,13 +231,9 @@ function BlockListBlock( {
const onKeyDown = ( event ) => {
const { keyCode, target } = event;

// ENTER/BACKSPACE Shortcuts are only available if the wrapper is focused
// and the block is not locked.
const canUseShortcuts = (
isSelected &&
! isLocked &&
( target === wrapper.current || target === breadcrumb.current )
);
// ENTER/BACKSPACE Shortcuts are only available if the wrapper or
// breadcrumb is focused.
const canUseShortcuts = ( target === wrapper.current || target === breadcrumb.current );
const isEditMode = ! isNavigationMode;

switch ( keyCode ) {
Expand All @@ -279,51 +253,6 @@ function BlockListBlock( {
event.preventDefault();
}
break;
case ESCAPE:
if (
isSelected &&
isEditMode
) {
setNavigationMode( true );
wrapper.current.focus();
}
break;
}
};

/**
* Begins tracking cursor multi-selection when clicking down within block.
*
* @param {MouseEvent} event A mousedown event.
*/
const onMouseDown = ( event ) => {
// Not the main button.
// https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
if ( event.button !== 0 ) {
return;
}

if (
isNavigationMode &&
isSelected &&
isInsideRootBlock( blockNodeRef.current, event.target )
) {
setNavigationMode( false );
}

if ( event.shiftKey ) {
if ( ! isSelected ) {
onShiftSelection();
event.preventDefault();
}

// Allow user to escape out of a multi-selection to a singular
// selection of a block via click. This is handled here since
// onFocus excludes blocks involved in a multiselection, as
// focus can be incurred by starting a multiselection (focus
// moved to first block's multi-controls).
} else if ( isPartOfMultiSelection ) {
onSelect();
}
};

Expand All @@ -333,7 +262,7 @@ function BlockListBlock( {
// cases where Firefox might always set `buttons` to `0`.
// See https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons
// See https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/which
if ( isSelected && ( buttons || which ) === 1 ) {
if ( ( buttons || which ) === 1 ) {
onSelectionStart( clientId );
}
};
Expand Down Expand Up @@ -469,18 +398,16 @@ function BlockListBlock( {
);

return (
<IgnoreNestedEvents
Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth Might be of interest to you as you originally created this component.

<animated.div
id={ blockElementId }
ref={ wrapper }
className={ wrapperClassName }
data-type={ name }
onFocus={ onFocus }
onKeyDown={ onKeyDown }
// Only allow shortcuts when a blocks is selected and not locked.
onKeyDown={ isSelected && ! isLocked ? onKeyDown : undefined }
tabIndex="0"
aria-label={ blockLabel }
role="group"
childHandledEvents={ [ 'onDragStart', 'onMouseDown' ] }
tagName={ animated.div }
{ ...wrapperProps }
style={
wrapperProps && wrapperProps.style ?
Expand Down Expand Up @@ -542,11 +469,10 @@ function BlockListBlock( {
) }
</Popover>
) }
<IgnoreNestedEvents
<div
ref={ blockNodeRef }
onDragStart={ preventDrag }
onMouseDown={ onMouseDown }
onMouseLeave={ onMouseLeave }
// Only allow selection to be started from a selected block.
onMouseLeave={ isSelected ? onMouseLeave : undefined }
Comment on lines +474 to +475
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to multi-selection? And if it is, would any of it be expected to be handled in WritingFlow instead? Considering you mention it at https://github.com/WordPress/gutenberg/pull/19397/files#r363036970 as being partly responsible for handling multi-selection.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, this is for multi selection. It would be nice to move this to WritingFlow as well. I didn't do it here to avoid a huge PR, as it seems a little more tricky to solve.

data-block={ clientId }
>
<BlockCrashBoundary onError={ onBlockError }>
Expand All @@ -565,7 +491,7 @@ function BlockListBlock( {
] }
</BlockCrashBoundary>
{ !! hasError && <BlockCrashWarning /> }
</IgnoreNestedEvents>
</div>
{ showEmptyBlockSideInserter && (
<div className="block-editor-block-list__empty-block-inserter">
<Inserter
Expand All @@ -576,7 +502,7 @@ function BlockListBlock( {
/>
</div>
) }
</IgnoreNestedEvents>
</animated.div>
);
}

Expand Down Expand Up @@ -680,14 +606,12 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
const {
updateBlockAttributes,
selectBlock,
multiSelect,
insertBlocks,
insertDefaultBlock,
removeBlock,
mergeBlocks,
replaceBlocks,
toggleSelection,
setNavigationMode,
__unstableMarkLastChangeAsPersistent,
} = dispatch( 'core/block-editor' );

Expand Down Expand Up @@ -750,25 +674,9 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
}
replaceBlocks( [ ownProps.clientId ], blocks, indexToSelect );
},
onShiftSelection() {
if ( ! ownProps.isSelectionEnabled ) {
return;
}

const {
getBlockSelectionStart,
} = select( 'core/block-editor' );

if ( getBlockSelectionStart() ) {
multiSelect( getBlockSelectionStart(), ownProps.clientId );
} else {
selectBlock( ownProps.clientId );
}
},
toggleSelection( selectionEnabled ) {
toggleSelection( selectionEnabled );
},
setNavigationMode,
};
} );

Expand Down
13 changes: 5 additions & 8 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useRef } from '@wordpress/element';
import { AsyncModeProvider, useSelect } from '@wordpress/data';

/**
Expand All @@ -15,7 +14,7 @@ import { AsyncModeProvider, useSelect } from '@wordpress/data';
import BlockListBlock from './block';
import BlockListAppender from '../block-list-appender';
import __experimentalBlockListFooter from '../block-list-footer';
import useMultiSelection from './use-multi-selection';
import RootContainer from './root-container';

/**
* If the block count exceeds the threshold, we disable the reordering animation
Expand Down Expand Up @@ -71,18 +70,17 @@ function BlockList( {
hasMultiSelection,
enableAnimation,
} = useSelect( selector, [ rootClientId ] );
const ref = useRef();
const onSelectionStart = useMultiSelection( { ref, rootClientId } );
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice that this is no longer rendered for every block list, but rather only for the root block list.


const uiParts = {
hasMovers: true,
hasSelectedUI: true,
...__experimentalUIParts,
};

const Container = rootClientId ? 'div' : RootContainer;

return (
<div
ref={ ref }
<Container
className={ classnames(
'block-editor-block-list__layout',
className
Expand All @@ -98,7 +96,6 @@ function BlockList( {
<BlockListBlock
rootClientId={ rootClientId }
clientId={ clientId }
onSelectionStart={ onSelectionStart }
isDraggable={ isDraggable }
moverDirection={ moverDirection }
isMultiSelecting={ isMultiSelecting }
Expand All @@ -118,7 +115,7 @@ function BlockList( {
renderAppender={ renderAppender }
/>
<__experimentalBlockListFooter.Slot />
</div>
</Container>
);
}

Expand Down
Loading