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

Jump to bottom

Remove multi-selection header #2934

Merged
merged 8 commits into from
Oct 12, 2017
Merged

Remove multi-selection header #2934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6ae545a
Remove multi-selection header
ellatrix Oct 9, 2017
deec597
Fix Firefox event issue
ellatrix Oct 9, 2017
c20bb3d
MouseEvent.buttons not supported in Safari
ellatrix Oct 10, 2017
5514202
Multi-select: keep focus state for start block
ellatrix Oct 10, 2017
7c82b00
Moving blocks should not affect the selection reducer
ellatrix Oct 10, 2017
58bd5f7
Do not nest focusable elements
ellatrix Oct 10, 2017
4c0954e
Remove HTML mode toggle for multi-select
ellatrix Oct 11, 2017
fdea571
Multi-select: do not leave editable focus at end. Do not show hover U…
ellatrix Oct 12, 2017
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
12 changes: 12 additions & 0 deletions editor/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,18 @@ export function selectBlock( uid ) {
};
}

export function startMultiSelect() {
return {
type: 'START_MULTI_SELECT',
};
}

export function stopMultiSelect() {
return {
type: 'STOP_MULTI_SELECT',
};
}

export function multiSelect( start, end ) {
return {
type: 'MULTI_SELECT',
Expand Down
9 changes: 9 additions & 0 deletions editor/block-mover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { getBlockType } from '@wordpress/blocks';
import './style.scss';
import { isFirstBlock, isLastBlock, getBlockIndex, getBlock } from '../selectors';
import { getBlockMoverLabel } from './mover-label';
import { selectBlock } from '../actions';

function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, blockType, firstIndex } ) {
// We emulate a disabled state because forcefully applying the `disabled`
Expand Down Expand Up @@ -68,12 +69,20 @@ export default connect(
} ),
( dispatch, ownProps ) => ( {
onMoveDown() {
if ( ownProps.uids.length === 1 ) {
dispatch( selectBlock( first( ownProps.uids ) ) );
}

dispatch( {
type: 'MOVE_BLOCKS_DOWN',
uids: ownProps.uids,
} );
},
onMoveUp() {
if ( ownProps.uids.length === 1 ) {
dispatch( selectBlock( first( ownProps.uids ) ) );
}

dispatch( {
type: 'MOVE_BLOCKS_UP',
uids: ownProps.uids,
Expand Down
21 changes: 9 additions & 12 deletions editor/block-settings-menu/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@ import { connect } from 'react-redux';
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { __, sprintf, _n } from '@wordpress/i18n';
import { IconButton } from '@wordpress/components';

/**
* Internal dependencies
*/
import { isEditorSidebarOpened } from '../selectors';
import { selectBlock, removeBlock, toggleSidebar, setActivePanel, toggleBlockMode } from '../actions';
import { removeBlocks, toggleSidebar, setActivePanel, toggleBlockMode } from '../actions';

function BlockSettingsMenuContent( { onDelete, onSelect, isSidebarOpened, onToggleSidebar, onShowInspector, onToggleMode } ) {
function BlockSettingsMenuContent( { onDelete, isSidebarOpened, onToggleSidebar, onShowInspector, onToggleMode, uids } ) {
const count = uids.length;
const toggleInspector = () => {
onSelect();
onShowInspector();
if ( ! isSidebarOpened ) {
onToggleSidebar();
Expand All @@ -36,14 +36,14 @@ function BlockSettingsMenuContent( { onDelete, onSelect, isSidebarOpened, onTogg
className="editor-block-settings-menu__control"
onClick={ onDelete }
icon="trash"
label={ __( 'Delete the block' ) }
label={ sprintf( _n( 'Delete the block', 'Delete the %d blocks', count ), count ) }
/>
<IconButton
{ count === 1 && <IconButton
className="editor-block-settings-menu__control"
onClick={ onToggleMode }
icon="html"
label={ __( 'Switch between the visual/text mode' ) }
/>
/> }
</div>
);
}
Expand All @@ -54,10 +54,7 @@ export default connect(
} ),
( dispatch, ownProps ) => ( {
onDelete() {
dispatch( removeBlock( ownProps.uid ) );
},
onSelect() {
dispatch( selectBlock( ownProps.uid ) );
dispatch( removeBlocks( ownProps.uids ) );
},
onShowInspector() {
dispatch( setActivePanel( 'block' ) );
Expand All @@ -66,7 +63,7 @@ export default connect(
dispatch( toggleSidebar() );
},
onToggleMode() {
dispatch( toggleBlockMode( ownProps.uid ) );
dispatch( toggleBlockMode( ownProps.uids[ 0 ] ) );
},
} )
)( BlockSettingsMenuContent );
17 changes: 11 additions & 6 deletions editor/block-settings-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,19 @@ class BlockSettingsMenu extends Component {
}

toggleMenu() {
this.props.onSelect();
// Block could be hovered, not selected.
if ( this.props.uids.length === 1 ) {
this.props.onSelect( this.props.uids[ 0 ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR but I guess this means we could drop this line https://github.com/WordPress/gutenberg/pull/2934/files#diff-1d765b1a62f4ddc8c86819016031b8dfR23

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, was wondering why there were two calls. Not sure which to delete. The call here is not strictly necessary, it's only needed for the inspector I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, actually it's necessary when we click on the toggle for a "hovered" block, we need to select it first to avoid issues when trying to click the menu icons.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done.

}

this.setState( ( state ) => ( {
opened: ! state.opened,
} ) );
}

render() {
const { opened } = this.state;
const { uid } = this.props;
const { uids, focus } = this.props;
const toggleClassname = classnames( 'editor-block-settings-menu__toggle', 'editor-block-settings-menu__control', {
'is-opened': opened,
} );
Expand All @@ -48,19 +52,20 @@ class BlockSettingsMenu extends Component {
onClick={ this.toggleMenu }
icon="ellipsis"
label={ opened ? __( 'Close Settings Menu' ) : __( 'Open Settings Menu' ) }
focus={ focus }
/>

{ opened && <BlockSettingsMenuContent uid={ uid } /> }
{ opened && <BlockSettingsMenuContent uids={ uids } /> }
</div>
);
}
}

export default connect(
undefined,
( dispatch, ownProps ) => ( {
onSelect() {
dispatch( selectBlock( ownProps.uid ) );
( dispatch ) => ( {
onSelect( uid ) {
dispatch( selectBlock( uid ) );
},
} )
)( BlockSettingsMenu );
45 changes: 3 additions & 42 deletions editor/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { connect } from 'react-redux';
/**
* WordPress dependencies
*/
import { sprintf, _n, __ } from '@wordpress/i18n';
import { __ } from '@wordpress/i18n';
import { IconButton } from '@wordpress/components';

/**
Expand All @@ -18,53 +18,17 @@ import PublishButton from './publish-button';
import PreviewButton from './preview-button';
import ModeSwitcher from './mode-switcher';
import Inserter from '../inserter';
import { getMultiSelectedBlockUids, hasEditorUndo, hasEditorRedo, isEditorSidebarOpened } from '../selectors';
import { clearSelectedBlock, toggleSidebar, removeBlocks } from '../actions';
import { hasEditorUndo, hasEditorRedo, isEditorSidebarOpened } from '../selectors';
import { toggleSidebar } from '../actions';

function Header( {
multiSelectedBlockUids,
onRemove,
onDeselect,
undo,
redo,
hasRedo,
hasUndo,
onToggleSidebar,
isSidebarOpened,
} ) {
const count = multiSelectedBlockUids.length;

if ( count ) {
return (
<div
role="region"
aria-label={ __( 'Editor toolbar' ) }
className="editor-header editor-header-multi-select"
>
<div className="editor-selected-count">
{ sprintf( _n( '%d block selected', '%d blocks selected', count ), count ) }
</div>
<div className="editor-selected-delete">
<IconButton
icon="trash"
label={ __( 'Delete selected blocks' ) }
onClick={ () => onRemove( multiSelectedBlockUids ) }
focus={ true }
>
{ __( 'Delete' ) }
</IconButton>
</div>
<div className="editor-selected-clear">
<IconButton
icon="no"
label={ __( 'Clear selected blocks' ) }
onClick={ () => onDeselect() }
/>
</div>
</div>
);
}

return (
<div
role="region"
Expand Down Expand Up @@ -102,14 +66,11 @@ function Header( {

export default connect(
( state ) => ( {
multiSelectedBlockUids: getMultiSelectedBlockUids( state ),
hasUndo: hasEditorUndo( state ),
hasRedo: hasEditorRedo( state ),
isSidebarOpened: isEditorSidebarOpened( state ),
} ),
( dispatch ) => ( {
onDeselect: () => dispatch( clearSelectedBlock() ),
onRemove: ( uids ) => dispatch( removeBlocks( uids ) ),
undo: () => dispatch( { type: 'UNDO' } ),
redo: () => dispatch( { type: 'REDO' } ),
onToggleSidebar: () => dispatch( toggleSidebar() ),
Expand Down
15 changes: 0 additions & 15 deletions editor/header/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,6 @@
right: $admin-sidebar-width-big;
}

.editor-header-multi-select {
background: $blue-medium-100;
border-bottom: 1px solid $blue-medium-200;
}

.editor-selected-count {
padding-right: $item-spacing;
color: $dark-gray-500;
border-right: 1px solid $light-gray-500;
}

.editor-selected-clear {
margin: 0 0 0 auto;
}

// hide all action buttons except the inserter on mobile
.editor-header__content-tools > .components-button {
display: none;
Expand Down
12 changes: 11 additions & 1 deletion editor/modes/visual-editor/block-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
getMultiSelectedBlocks,
getMultiSelectedBlockUids,
} from '../../selectors';
import { insertBlock, multiSelect } from '../../actions';
import { insertBlock, startMultiSelect, stopMultiSelect, multiSelect } from '../../actions';

const INSERTION_POINT_PLACEHOLDER = '[[insertion-point]]';

Expand Down Expand Up @@ -138,6 +138,8 @@ class VisualEditorBlockList extends Component {
// Capture scroll on all elements.
window.addEventListener( 'scroll', this.onScroll, true );
window.addEventListener( 'mouseup', this.onSelectionEnd );

this.props.onStartMultiSelect();
}

onSelectionChange( uid ) {
Expand Down Expand Up @@ -169,6 +171,8 @@ class VisualEditorBlockList extends Component {
window.removeEventListener( 'mousemove', this.onPointerMove );
window.removeEventListener( 'scroll', this.onScroll, true );
window.removeEventListener( 'mouseup', this.onSelectionEnd );

this.props.onStopMultiSelect();
}

appendDefaultBlock() {
Expand Down Expand Up @@ -244,6 +248,12 @@ export default connect(
onInsertBlock( block ) {
dispatch( insertBlock( block ) );
},
onStartMultiSelect() {
dispatch( startMultiSelect() );
},
onStopMultiSelect() {
dispatch( stopMultiSelect() );
},
onMultiSelect( start, end ) {
dispatch( multiSelect( start, end ) );
},
Expand Down
37 changes: 23 additions & 14 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
import {
getBlock,
getBlockFocus,
isMultiSelecting,
getBlockIndex,
getEditedPostAttribute,
getMultiSelectedBlockUids,
Expand Down Expand Up @@ -136,7 +137,6 @@ class VisualEditorBlock extends Component {

bindBlockNode( node ) {
this.node = node;
this.props.blockRef( node );
}

setAttributes( attributes ) {
Expand Down Expand Up @@ -256,8 +256,9 @@ class VisualEditorBlock extends Component {
}

onPointerDown( event ) {
// Not the main button (usually the left button on pointer device).
if ( event.buttons !== 1 ) {
// Not the main button.
// https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
if ( event.button !== 0 ) {
return;
}

Expand Down Expand Up @@ -306,12 +307,13 @@ class VisualEditorBlock extends Component {
// Generate the wrapper class names handling the different states of the block.
const { isHovered, isSelected, isMultiSelected, isFirstMultiSelected, focus } = this.props;
const showUI = isSelected && ( ! this.props.isTyping || focus.collapsed === false );
const isProperlyHovered = isHovered && ! this.props.isSelecting;
const { error } = this.state;
const wrapperClassname = classnames( 'editor-visual-editor__block', {
'has-warning': ! isValid || !! error,
'is-selected': showUI,
'is-multi-selected': isMultiSelected,
'is-hovered': isHovered,
'is-hovered': isProperlyHovered,
} );

const { onMouseLeave, onFocus, onReplace } = this.props;
Expand All @@ -330,31 +332,37 @@ class VisualEditorBlock extends Component {
/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
return (
<div
ref={ this.bindBlockNode }
onKeyDown={ this.onKeyDown }
onFocus={ this.onFocus }
ref={ this.props.blockRef }
onMouseMove={ this.maybeHover }
onMouseEnter={ this.maybeHover }
onMouseLeave={ onMouseLeave }
className={ wrapperClassname }
data-type={ block.name }
tabIndex="0"
aria-label={ blockLabel }
{ ...wrapperProps }
>
<BlockDropZone index={ order } />
{ ( showUI || isHovered ) && <BlockMover uids={ [ block.uid ] } /> }
{ ( showUI || isHovered ) && <BlockRightMenu uid={ block.uid } /> }
{ ( showUI || isProperlyHovered ) && <BlockMover uids={ [ block.uid ] } /> }
{ ( showUI || isProperlyHovered ) && <BlockRightMenu uids={ [ block.uid ] } /> }
{ showUI && isValid && mode === 'visual' && <BlockToolbar uid={ block.uid } /> }

{ isFirstMultiSelected && (
{ isFirstMultiSelected && ! this.props.isSelecting &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why do we need the isSelecting prop? I'm a bit concerned about all these booleans we use here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

isSelecting will be true when the user is in the process of selecting, in which case we should update the visual selection, but we shouldn't (yet) show UI or show hover UI during this process.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Do you think it could be tracked in Redux's state instead of a local state?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can do that.

<BlockMover uids={ multiSelectedBlockUids } />
) }
}
{ isFirstMultiSelected && ! this.props.isSelecting &&
<BlockRightMenu
uids={ multiSelectedBlockUids }
focus={ true }
/>
}
<div
ref={ this.bindBlockNode }
onKeyPress={ this.maybeStartTyping }
onDragStart={ ( event ) => event.preventDefault() }
onMouseDown={ this.onPointerDown }
onKeyDown={ this.onKeyDown }
onFocus={ this.onFocus }
className="editor-visual-editor__block-edit"
tabIndex="0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned about this change (moving the tabIndex here). Because we heavily rely on the .editor-visual-editor__block being tabbable (for arrow navigation for example). I'll have to test the places we use this className.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed arrow navigation is broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it's unfortunate that we rely on .editor-visual-editor__block. :/ We do have to move it because nesting buttons in this is causing bugs in a Firefox and Safari.

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'll update the selector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I don't know honestly what's the best alternative would be. This is all related to focus management which is causing us a lot of trouble in several places. We'll have to rethink it entirely IMO:

  • Clarify the role of the focus config in state
  • Have clear guidelines on when to rely on DOM imperative focus handling and state-based focus updates.
  • All this while ideally hiding everything from the block's author.

I think we should tackle this clarification as soon as we can.

Copy link
Member Author

Choose a reason for hiding this comment

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

How can I resolve this PR? Should I remove the bug fixes for Firefox and Safari (also broken in master).

Copy link
Contributor

Choose a reason for hiding this comment

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

It relates more to the Arrow navigation which is causing issue with this PR.

Maybe it's ok to do a temporary hack in this PR, adding a way to identify the block's tabbable element (className or attribute or something else) and update the WritingFlow component to target this node instead of editor-visual-editor__block

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok. I'll update the temporary hack in WritingFlow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rewritten in dc9859f2935c5add1d4c10edf45ca916542ff4e1.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I don't know honestly what's the best alternative would be. This is all related to focus management which is causing us a lot of trouble in several places. We'll have to rethink it entirely IMO

Noting that I have a local work-in-progress branch which takes a stab at this with a few ideas:

  • We don't explicitly target the block class, but rather allow descendant DOM nodes of WritingFlow assign a broadly-understood data- attribute which defines a focus context (for a block, data-focus-context={ block.uid }
  • State tracks the ID of the current focus context, and when this changes, WritingFlow shifts focus to the first available input within the new context (via querySelector) if the context does not already currently have focus
  • Arrow key navigation works simply by updating the focus ID in state using the closest focus context of the next eligible input, if it is different than the current value, otherwise simply moving focus
  • Blocks can still explicitly move focus to inputs (like video block to its caption) by assigning a custom focus context ID to editable, so e.g. <Editable focusId="caption" />, setFocus( 'caption' )

As a work-in-progress, some of these ideas are still muddy, for example directionality of focus and shifting focus within the same focus context might be deserving of more controlled treatment within state.

aria-label={ blockLabel }
>
<BlockCrashBoundary onError={ this.onBlockError }>
{ isValid && mode === 'visual' && (
Expand Down Expand Up @@ -404,6 +412,7 @@ export default connect(
isFirstMultiSelected: isFirstMultiSelectedBlock( state, ownProps.uid ),
isHovered: isBlockHovered( state, ownProps.uid ),
focus: getBlockFocus( state, ownProps.uid ),
isSelecting: isMultiSelecting( state ),
isTyping: isTyping( state ),
order: getBlockIndex( state, ownProps.uid ),
multiSelectedBlockUids: getMultiSelectedBlockUids( state ),
Expand Down
Loading