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

Fix multiselecting blocks using shift + arrow #11237

Merged
merged 3 commits into from
Oct 30, 2018
Merged

Fix multiselecting blocks using shift + arrow #11237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41f810a
Fix multiselecting blocks using shift + arrow
youknowriad Oct 30, 2018
59546d4
Test multiple shift + arrows selections
youknowriad Oct 30, 2018
2b9f20a
Fix end2end test failures
youknowriad Oct 30, 2018
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: 6 additions & 0 deletions packages/editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ export class BlockListBlock extends Component {
if ( this.props.isSelected && ! prevProps.isSelected ) {
this.focusTabbable( true );
}

// When triggering a multi-selection,
// move the focus to the wrapper of the first selected block.
Copy link
Member

Choose a reason for hiding this comment

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

These comments explain what we're doing (which is pretty plainly obvious in the implementation anyways) but not why. I don't know why we're doing this.

Copy link
Member

Choose a reason for hiding this comment

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

We move the focus to ensure that it is not possible to continue editing the initially selected block. This happens for all active elements that allow editing like RichText, PlainText and all form fields. This behavior ensures that you no longer can modify the content of the selected blocks.

if ( this.props.isFirstMultiSelected && ! prevProps.isFirstMultiSelected ) {
this.wrapperNode.focus();
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 something which is meant to occur instead of focusTabbable? If so, why do we allow focusTabbable to occur at all? I wonder also if these redundant focus changes are announced loudly in assistive technologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's meant to occur instead of focusTabbable and yes focusTabbable won't occur here because the isSelected is false when the block is multiselected.

I'm not sure about the assertive technologies though, it's very tied with multi-selection, not sure how multi-selection is announced.

}
}

setBlockListRef( node ) {
Expand Down
70 changes: 19 additions & 51 deletions packages/editor/src/components/block-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
* WordPress Dependencies
*/
import { withSelect } from '@wordpress/data';
import { Component, createRef, Fragment } from '@wordpress/element';
import { focus } from '@wordpress/dom';
import { Fragment } from '@wordpress/element';

/**
* Internal Dependencies
Expand All @@ -14,63 +13,32 @@ import BlockControls from '../block-controls';
import BlockFormatControls from '../block-format-controls';
import BlockSettingsMenu from '../block-settings-menu';

class BlockToolbar extends Component {
constructor() {
super( ...arguments );
this.container = createRef();
function BlockToolbar( { blockClientIds, isValid, mode } ) {
if ( blockClientIds.length === 0 ) {
return null;
}

componentDidMount() {
if ( this.props.blockClientIds.length > 1 ) {
this.focusContainer();
}
}

componentDidUpdate( prevProps ) {
if (
prevProps.blockClientIds.length <= 1 &&
this.props.blockClientIds.length > 1
) {
this.focusContainer();
}
}

focusContainer() {
const tabbables = focus.tabbable.find( this.container.current );
if ( tabbables.length ) {
tabbables[ 0 ].focus();
}
}

render() {
const { blockClientIds, isValid, mode } = this.props;

if ( blockClientIds.length === 0 ) {
return null;
}

if ( blockClientIds.length > 1 ) {
return (
<div className="editor-block-toolbar" ref={ this.container }>
<MultiBlocksSwitcher />
<BlockSettingsMenu clientIds={ blockClientIds } />
</div>
);
}

if ( blockClientIds.length > 1 ) {
return (
<div className="editor-block-toolbar">
{ mode === 'visual' && isValid && (
<Fragment>
<BlockSwitcher clientIds={ blockClientIds } />
<BlockControls.Slot />
<BlockFormatControls.Slot />
</Fragment>
) }
<MultiBlocksSwitcher />
<BlockSettingsMenu clientIds={ blockClientIds } />
</div>
);
}

return (
<div className="editor-block-toolbar">
{ mode === 'visual' && isValid && (
<Fragment>
<BlockSwitcher clientIds={ blockClientIds } />
<BlockControls.Slot />
<BlockFormatControls.Slot />
</Fragment>
) }
<BlockSettingsMenu clientIds={ blockClientIds } />
</div>
);
}

export default withSelect( ( select ) => {
Expand Down