Remove multi-selection header #2934

ellatrix · 2017-10-09T14:03:30Z

Description

Split off from #1811. This PR remove this multi selection header in favour of inline actions, just like single-selected blocks. Adding inspector tools can be done in a separate PR. See #1811.

How Has This Been Tested?

The multi-select header should be gone.
There should be a right side menu. Opening the inspector should work. Deleting the blocks should work.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

codecov · 2017-10-09T14:12:08Z

Codecov Report

Merging #2934 into master will increase coverage by 0.57%.
The diff coverage is 2.32%.

@@            Coverage Diff             @@
##           master    #2934      +/-   ##
==========================================
+ Coverage   34.47%   35.05%   +0.57%     
==========================================
  Files         196      197       +1     
  Lines        5798     6008     +210     
  Branches     1021     1078      +57     
==========================================
+ Hits         1999     2106     +107     
- Misses       3216     3286      +70     
- Partials      583      616      +33

Impacted Files	Coverage Δ
editor/writing-flow/index.js	`0% <ø> (ø)`	⬆️
editor/modes/visual-editor/block-list.js	`0% <0%> (ø)`	⬆️
editor/block-mover/index.js	`0% <0%> (ø)`	⬆️
editor/header/index.js	`0% <0%> (ø)`	⬆️
editor/sidebar/block-inspector/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
editor/sidebar/header.js	`0% <0%> (ø)`	⬆️
editor/reducer.js	`87.5% <0%> (-1.32%)`	⬇️
editor/block-settings-menu/content.js	`0% <0%> (ø)`	⬆️
editor/actions.js	`35% <0%> (-1.85%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e95242...fdea571. Read the comment docs.

jasmussen · 2017-10-09T14:50:43Z

Thanks for working on this. Very nice! 👍 👍

androb · 2017-10-09T15:00:07Z

Agreed. IMHO this improves the UI by decreasing the number of UI changes happening after select 👍

youknowriad

This is not working on Firefox, a chance I switch to Firefox nightly recently :)

youknowriad · 2017-10-09T15:45:32Z

editor/block-settings-menu/index.js

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


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

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.

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.

This is done.

youknowriad · 2017-10-09T15:49:20Z

editor/sidebar/index.js

@@ -32,6 +32,7 @@ export default connect(
 	( state ) => {
 		return {
 			panel: getActivePanel( state ),
+			selectedBlockCount: getSelectedBlockCount( state ),


Should we connect the Header and the BlockInspector instead since this is not directly used here?

ellatrix · 2017-10-09T16:02:26Z

This is not working on Firefox, a chance I switch to Firefox nightly recently :)

What's not working exactly? Is it the buttons? Are the working buttons not working for you either? Strange...

youknowriad · 2017-10-09T16:04:38Z

@iseulde Yes, no buttons working, neither the arrow buttons nor the toggle.

youknowriad · 2017-10-09T16:11:20Z

No error, nothing, seems like there's another event handler preceding the ones for the buttons.

youknowriad · 2017-10-09T16:17:11Z

It looks unrelated to this PR, I have the same issue with the arrows on master but this PR makes it worse because we can't delete multi selected blocks now

ellatrix · 2017-10-09T16:20:19Z

Looks like there's a SELECT_BLOCK action... Looking where it's coming from.

ellatrix · 2017-10-09T16:24:37Z

Found it. The padding overlaps. This doesn't trigger a mousedown event in Chrome, but it does in FF.

ellatrix · 2017-10-09T17:53:14Z

I seem to be wrong there. It seems FF is retargeting the focus event. In block.js the event has the explicitOriginalTarget property: https://developer.mozilla.org/en-US/docs/Web/API/Event/explicitOriginalTarget.

ellatrix · 2017-10-09T18:07:08Z

@youknowriad That should fix it... Not nice to introduce browser specific APIs, but I don't see another way other than adding tabIndex="0" to a random deeper nested div, which is worse.

youknowriad · 2017-10-09T18:14:48Z

I don't mind the browser specific code if it's commented.

Still seeing some issues:

Can't delete multi selected blocks on Firefox
Multi-selection is lost when moving blocks in Chrome

ellatrix · 2017-10-09T18:17:39Z

Hm, I can't reproduce either issue...

ellatrix · 2017-10-09T18:25:37Z

Tested the event issue sandboxed, and Safari should have the same issue. But multi-selection in safari is broken entirely... 😅

ellatrix · 2017-10-09T18:46:51Z

Fix that, but then other stuff breaks. I'm in focus hell again.

youknowriad · 2017-10-10T08:18:19Z

Testing this more, I think these issues I'm seeing are related. I can't reproduce them consistently, it happens once in 4/5 tries. But if one happens, it means the second one will happen two.

Also, I wonder if it's a z-index issue because I've found that the "trash" icon doesn't get the "cursor" pointer and doesn't change the color when I hover it. (Again not consistently).

youknowriad · 2017-10-10T08:22:36Z

Now that I think about it, I fixed a z-index issue in the Right menu in another PR, I wonder if a rebase would fix at least the "delete" issue.

ellatrix · 2017-10-10T12:35:07Z

@youknowriad Which PR? Already merged?

Re Safari, I can push the change here, but in safari I'm having issues where the focus in editable constantly moves. Still have to track that down.

youknowriad · 2017-10-10T13:19:37Z

@iseulde yes already merged #2794

ellatrix · 2017-10-10T14:11:17Z

editor/modes/visual-editor/block.js

@@ -250,14 +250,18 @@ class VisualEditorBlock extends Component {
 	}

 	onFocus( event ) {
-		if ( event.target === this.node ) {
+		// Firefox retargets to parent with tabIndex.
+		const target = event.nativeEvent.explicitOriginalTarget || event.target;


Safari also retargets, and no way to get the real target... :/ Maybe we're not supposed to put focusable elements inside a tabIndex="0"?

Maybe @afercia knows more about this? :)

I'm not sure nesting focusable elements is a good thing. It's like putting a link inside a link.

Yeah, that's what I think too... This is no good: <div tabindex="0"><button></button></div>.

Thanks @afercia for the quick feedback! I've removed the nesting in this branch now.

ellatrix · 2017-10-11T16:32:31Z

@youknowriad I have no issues in Safari... What's the version? Errors? Any specific steps to reproduce?

ephox-mogran · 2017-10-11T22:39:47Z

This works quite nicely. I'm not sure if it should be part of this PR or a separate issue, but a problem with this multi block select is that the browser still has a selection but it's not visible to the user. Therefore, if they start typing, some of their text (their selection start point to the end of the first block they selected) disappears and is replaced by what you type, but not all of it. This is probably a confusing user experience.

If you think about what users are used to, as soon as they start typing, they would probably expect all selected blocks to be deleted and replaced with what they are typing. Is there an issue for this somewhere?

ephox-mogran · 2017-10-11T22:49:10Z

Also, I'm hitting the same issue as @gziolo on Chrome. It seems that if you click outside the range of <div role="region" aria-label="Editor content" class="editor-visual-editor">, the selected blocks don't get unselected, and focus is set to <div id="wpbody-content" aria-label="Main content" tabindex="0">, which means that you lose keyboard input as well as not losing selection.

jasmussen · 2017-10-12T06:49:22Z

a problem with this multi block select is that the browser still has a selection but it's not visible to the user

Should some of these issues be addressed as part of ongoing work in #2990?

ellatrix · 2017-10-12T07:29:20Z

@ephox-mogran That seems to a regression on this branch, where we keep the focus on the first selected block rather than moving it elsewhere. I'll fix it. I don't necessarily agree that typing should remove all the blocks, but we can discuss that in a separate issue.

ellatrix · 2017-10-12T08:20:44Z

Now it will set focus on the settings menu. Not sure where to best set it for now, but in the future this should probably be that some element as for a cmd/ctrl while in a block (first toolbar element). Also removed hover UI while the user is selecting.

ellatrix · 2017-10-12T08:24:46Z

Any other action items for this branch?

youknowriad · 2017-10-12T10:11:27Z

editor/modes/visual-editor/block.js

 				{ showUI && isValid && mode === 'visual' && <BlockToolbar uid={ block.uid } /> }
-
-				{ isFirstMultiSelected && (
+				{ isFirstMultiSelected && ! this.props.isSelecting &&


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

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.

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

Sure, we can do that.

youknowriad · 2017-10-12T10:14:44Z

editor/modes/visual-editor/block.js

 					className="editor-visual-editor__block-edit"
+					tabIndex="0"


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.

Confirmed arrow navigation is broken.

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.

I'll update the selector.

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.

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

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

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

Rewritten in dc9859f2935c5add1d4c10edf45ca916542ff4e1.

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.

youknowriad

Works great! We're getting there

Feel free to update or not the PR to address the minor remaining comments.

…I while selecting.

afercia · 2017-10-19T10:41:53Z

Worth noting 58bd5f7 removed the tabindex and aria-label from the outer containers.

That was meant for keyboard navigation through blocks as implemented in #1618 and now currently discussed on #2031

tabindex and aria-label should be used on the containers used for keyboard navigation through blocks: depending on the direction #2031 will take, this should probably be changed.

ellatrix · 2017-10-20T14:34:56Z

@afercia As discussed earlier, we also can't leave a tabIndex on the outer wrapper as long as there are buttons inside it. #2934 (comment)

afercia · 2017-10-20T18:42:57Z

@iseulde we should distinguish a bit. Things like:
<div tabindex="0"><button></button></div> are bad and I don't see a use for them.

Instead, a focusable container which is part of the layout is a bit different. For example, in WordPress #wpbody-content is focusable in all the admin screens and has tons of focusable controls inside.

See: #2934 See: https://codepen.io/aduth/pen/MQxRME Something needs to capture the focus event, which is skipped by button press on the block's toolbar, but bubbles up wrongly to the parent's edit wrapper (which has tabIndex) and causes parent block to select itself.

* Writing Flow: Select block by own focus handler * Block List: Mark default appender events as block-handled * Writing Flow: ... * Block: Move tabIndex to wrapper component See: #2934 See: https://codepen.io/aduth/pen/MQxRME Something needs to capture the focus event, which is skipped by button press on the block's toolbar, but bubbles up wrongly to the parent's edit wrapper (which has tabIndex) and causes parent block to select itself. * Writing Flow: Update block focusable target * Block: Remove unused block classname constant Originally used for down-to-new-paragraph in #3973, removed as of #5271 * Block List: Capture focus event on block wrapper Corresponding to move of tabIndex from inner edit element to wrapper, we also need to capture focus from wrapper. This way, when focus is applied via WritingFlow, the block appropriately becomes selected. This may also make "onSelect" of movers unnecessary. * Block List: Remove pointer handling of onSelect Can now rely on focus behavior captured from wrapper node to reflect selection onto blocks, even those without their own focusable elements * Block List: Avoid select on focus if in multi-selection Multi-selection causes focus event to be triggered on the block, but at that point `isSelected` is false, so it will trigger `onSelect` and a subsequent `focusTabbables`, thus breaking the intended multi-selection flow. * Block Mover: Remove explicit select on block move Focus will bubble to block wrapper (or in case of Firefox/Safari, where button click itself does not trigger focus, focus on the wrapper itself) to incur self-select. * Block List: Singular select block on clicking multi-selected This reverts commit 90a5dab.

ellatrix force-pushed the try/remove-multi-select-header branch from cc33594 to e05b90a Compare October 9, 2017 14:07

ellatrix requested review from youknowriad and jasmussen October 9, 2017 14:08

ellatrix force-pushed the try/remove-multi-select-header branch 2 times, most recently from a5aef58 to d596807 Compare October 9, 2017 14:27

ellatrix force-pushed the try/remove-multi-select-header branch from d596807 to aefba78 Compare October 9, 2017 15:03

youknowriad reviewed Oct 9, 2017

View reviewed changes

ellatrix force-pushed the try/remove-multi-select-header branch from 71455ef to 44b1966 Compare October 10, 2017 13:32

ellatrix commented Oct 10, 2017

View reviewed changes

mcsf mentioned this pull request Oct 11, 2017

Improve keyboard navigation across blocks #2990

Closed

ellatrix force-pushed the try/remove-multi-select-header branch from a0c85ef to b807bc2 Compare October 12, 2017 08:22

youknowriad reviewed Oct 12, 2017

View reviewed changes

ellatrix force-pushed the try/remove-multi-select-header branch from b807bc2 to cb55280 Compare October 12, 2017 10:36

youknowriad approved these changes Oct 12, 2017

View reviewed changes

ellatrix added 3 commits October 12, 2017 13:01

Do not nest focusable elements

58bd5f7

Remove HTML mode toggle for multi-select

4c0954e

Multi-select: do not leave editable focus at end. Do not show hover U…

fdea571

…I while selecting.

ellatrix force-pushed the try/remove-multi-select-header branch from cb55280 to fdea571 Compare October 12, 2017 11:02

ellatrix merged commit e09410e into master Oct 12, 2017

ellatrix deleted the try/remove-multi-select-header branch October 12, 2017 12:17

ellatrix mentioned this pull request Oct 12, 2017

isMultiSelecting should persist across selection changes #3013

Merged

afercia mentioned this pull request Oct 19, 2017

Keyboard interaction: standardize the blocks behavior #2031

Closed

aduth mentioned this pull request Oct 27, 2017

Blocks: Improve VisualEditorBlock rendering of settings menu #3202

Merged

ellatrix mentioned this pull request Oct 27, 2017

Pressing Delete with a multi-block selection spanning from a paragraph doesn't delete #3191

Closed

2 tasks

aduth mentioned this pull request Mar 2, 2018

Writing Flow: Move selection to block's focus handler #5289

Merged

Remove multi-selection header #2934

Remove multi-selection header #2934

Conversation

ellatrix commented Oct 9, 2017

Description

How Has This Been Tested?

Checklist:

codecov bot commented Oct 9, 2017 • edited Loading

Codecov Report

jasmussen commented Oct 9, 2017

androb commented Oct 9, 2017

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Oct 9, 2017

youknowriad commented Oct 9, 2017

youknowriad commented Oct 9, 2017

youknowriad commented Oct 9, 2017

ellatrix commented Oct 9, 2017

ellatrix commented Oct 9, 2017

ellatrix commented Oct 9, 2017

ellatrix commented Oct 9, 2017

youknowriad commented Oct 9, 2017

ellatrix commented Oct 9, 2017

ellatrix commented Oct 9, 2017

ellatrix commented Oct 9, 2017

youknowriad commented Oct 10, 2017

youknowriad commented Oct 10, 2017

ellatrix commented Oct 10, 2017

youknowriad commented Oct 10, 2017

ellatrix Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Oct 11, 2017 • edited Loading

ephox-mogran commented Oct 11, 2017

ephox-mogran commented Oct 11, 2017

jasmussen commented Oct 12, 2017

ellatrix commented Oct 12, 2017

ellatrix commented Oct 12, 2017

ellatrix commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

afercia commented Oct 19, 2017

ellatrix commented Oct 20, 2017

afercia commented Oct 20, 2017

codecov bot commented Oct 9, 2017 •

edited

Loading

ellatrix Oct 10, 2017 •

edited

Loading

ellatrix commented Oct 11, 2017 •

edited

Loading