Add block navigator movers #18014

talldan · 2019-10-18T04:23:19Z

Description

Adds block navigator movers to the navigation block.

This PR refactors the Block Navigator to use follow a TreeGrid as described in https://w3c.github.io/aria-practices/examples/treegrid/treegrid-1.html?cell=force, as suggested by feedback on this PR. The navigator can be navigated using arrow keys.

Some descriptions have been added to the buttons for selecting blocks and the appender for screenreader users using aria-describedby.

This is quite a large PR, so there are a couple of PRs extracted from this:

Add new TreeGrid and RovingTabIndex components #19799 introduces RovingTabIndex and TreeGrid components
Extract block mover buttons so that they can be individually imported #22122 allows the individual mover buttons to be imported, which is needed in this PR so that focus can be managed separately on the buttons.

How has this been tested?

Add a navigation menu block (ensure the feature is enabled as an experiment first)
Add some menu items to the navigation menu
Click the block navigator button on the toolbar
Try reordering the items using the movers

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

draganescu · 2019-10-18T07:12:12Z

So far this works great and the code is perfect.

talldan · 2019-10-25T09:41:53Z

I think this one is open for reviews now.

I think it needs some a11y feedback and design feedback.

There are some issues I've spotted that I'll continue to look into:

When clicking the up mover button, focus is retained, but clicking the down mover button causes a focus loss
When selected item is focused the background turns white
Select block button uses the new display name, but screenreader text for mover buttons still refer to item by its block type
I think it would be good to read what's read by a screenreader for the select button. e.g. the select button should probably read 'Select [Block Type]' instead of just the block type.

packages/block-editor/src/components/block-navigation/list.js

youknowriad

Can we benefit from all these Block Navigation improvements in the global block navigation popover as well?

mtias · 2019-10-25T10:21:29Z

Can we benefit from all these Block Navigation improvements in the global block navigation popover as well?

The movers is the only one I'd be comfortable enabling in the top level one, but we need some improvements first — selection should be clearer, ideally also support the motion effect we have for moving blocks in the canvas.

mtias · 2019-10-25T10:22:46Z

Allow moving blocks in and out of nesting (follow-up PR?)
Drag and drop (follow-up PR?)
Animate moving of blocks (follow-up PR?)

Follow up PR seems fine. We can look at enabling the movers in the top-level navigator once these are worked out.

tellthemachines

I did a bit of testing on NVDA/Firefox and VoiceOver/Safari. Agree that screen reader output for mover buttons is not very informative right now, would be better to read out something like "move Sample page menu item".

I'm afraid the issue with focus being lost when clicking the down mover is even worse using NVDA/Firefox as focus moves to the document body, even with the modal still open (yikes!). On Firefox with no screen reader, focus can also be moved out of the modal but not every time. Here's an example screenshot of Firefox on Mac with focus moved behind the modal and the menu item still visually selected:

Another issue I'm noticing with the Navigator modal is that clicking on any arrow key closes it. This was already happening before this PR so it's not related to these changes but, because the navigator list now has a role of tree, VoiceOver is announcing it as a table, which is expected to be navigable with arrow keys. (Using the VO keys + arrow keys still works, but just using plain arrows should also work in a table scenario.) Still, it might be easier to handle this in a separate PR.

packages/block-editor/src/components/block-navigation/list.js

packages/block-editor/src/components/block-navigation/style.scss

talldan · 2019-10-28T05:27:15Z

Thanks for testing and reviewing @tellthemachines. There's definitely a bit subtlety to getting these movers to work in the right way.

It's interesting in your screenshot that focus has moved back to the mover outside the popover. Coincidence? I'll do some more debugging/testing to establish.

tellthemachines · 2019-10-28T05:31:40Z

It's interesting in your screenshot that focus has moved back to the mover outside the popover. Coincidence? I'll do some more debugging/testing to establish.

Initially it moves to the block inserter, then if you tab once it goes to the mover. I forgot to mention that bit, sorry.

mtias · 2020-05-07T10:56:39Z

Yes, let's test it for a release or two in the navigation block to squash all the bugs.

tellthemachines · 2020-05-08T06:19:12Z

packages/components/src/roving-tab-index/test/item.js

+		expect( renderer.toJSON() ).toMatchSnapshot();
+	} );
+
+	it( 'allows children to bue declared using a child render function as an alternative to `as`', () => {


Typo: "to be declared".

tellthemachines

This is working much better on VoiceOver, and is working well on NVDA too, apart from one issue: when entering the treegrid, NVDA should go into focus mode automatically, but it's not happening. In browse mode, we are not able to use the arrow keys to navigate the treegrid, so users will have to manually switch to focus mode to be able to interact with it. This is a bit annoying because the wai-aria example correctly switches to focus mode, and the markup used seems to be identical in everything to this one 😞 . @diegohaz any ideas on this?

Approving, because we should get this in and iterate! It would be good to fix the above issue before we extend this functionality to the general block navigator though.

tellthemachines · 2020-05-08T06:32:09Z

packages/components/src/tree-grid/README.md

+		<TreeGrid>
+			<TreeGridRow level={ 1 } positionInSet={ 1 } setSize={ 2 }>
+				<TreeGridCell>
+					{ ( props ) => <Button onClick={ onSelect } { ...props }>Select</Button> }


Does the element inside the cell have to be a button? I wouldn't expect that to be the typical case for this component. If we were to have just plain text, like in the wai-aria example, where would focus be transferred to? (This is something we can iterate on later though)

Yeah, that's a good point! I can think of a few ways to accomplish this, and it shouldn't be too difficult as TreeGridCell has all the information it needs.

tellthemachines · 2020-05-08T06:37:19Z

packages/components/src/tree-grid/index.js

+		<RovingTabIndexContainer>
+			{ /* Disable reason: A treegrid is implemented using a table element. */ }
+			{ /* eslint-disable-next-line jsx-a11y/no-noninteractive-element-to-interactive-role */ }
+			<table { ...props } role="treegrid" onKeyDown={ onKeyDown }>


Might be good to add in an aria-label here that can be passed a short description of what the treegrid is about.

tellthemachines · 2020-05-08T06:38:32Z

packages/components/src/tree-grid/test/cell.js

+) );
+
+describe( 'TreeGridCell', () => {
+	it( 'requires TreeGrid to be declared as a parent component somewhere in the component heirarchy', () => {


Typo: "hierarchy".

diegohaz · 2020-05-08T11:47:30Z

@tellthemachines

This is working much better on VoiceOver, and is working well on NVDA too, apart from one issue: when entering the treegrid, NVDA should go into focus mode automatically, but it's not happening. In browse mode, we are not able to use the arrow keys to navigate the treegrid, so users will have to manually switch to focus mode to be able to interact with it. This is a bit annoying because the wai-aria example correctly switches to focus mode, and the markup used seems to be identical in everything to this one 😞

It seems that it happens because the first active element when you tab into the treegrid is a button, not a role="gridcell" or role="row". I think this can be solved by always focusing the row when entering the treegrid, like in the WAI-ARIA example.

diegohaz · 2020-05-08T13:04:24Z

Hmm, after doing some more tests, it looks like NVDA messes up with focus/browse randomly when you focus on elements inside a tree grid that aren't rows or grid cells.

It happens even on the WAI-ARIA example. As soon as I focus the link, it switches to browse mode, and I'm no longer able to navigate through the tree grid using arrow keys.

The same happens on role="grid" elements. Seems like a bug on NVDA?

diegohaz · 2020-05-08T21:55:27Z

Related issue on NVDA: nvaccess/nvda#8395

Looks like it’s expected behavior and NVDA users should use especial keys to navigate the grid, or enable focus mode manually.

tellthemachines · 2020-05-11T00:10:24Z

Ooooh, great detective work @diegohaz ! I guess there's nothing else to do on our side then.

talldan · 2020-05-11T09:17:02Z

packages/block-editor/src/components/block-navigation/branch.js

-			</li>
-		);
-	}
+export default function BlockNavigationBranch( props ) {


@adamziel I ended up renaming BlockNavigationBranch to BlockNavigationLeaf and BlockNavigationList to BlockNavigationBranch, so this diff looks a bit confusing.

I think that's correct though, a branch should have multiple leaves, and the leaf should be the terminating points on a tree.

talldan · 2020-05-11T09:19:00Z

packages/block-editor/src/components/block-navigation/tree.js

+ *
+ * @param {Object} props
+ */
+export default function BlockNavigationTree( {


@adamziel I renamed BlockNavigationList to BlockNavigationTree to be consistent with the whole tree/branch/leaf thing.

talldan · 2020-05-11T09:22:45Z

packages/block-editor/src/components/block-navigation/context.js

+ */
+import { createContext, useContext } from '@wordpress/element';
+
+export const BlockNavigationContext = createContext( {


@adamziel I moved the context out into its own file and add a useBlockNavigationContext helper.

talldan · 2020-05-11T09:37:57Z

packages/block-editor/src/components/block-navigation/block-contents.js

+					}
+
+					return (
+						<BlockNavigationBlockContentWrapper as="div">


@adamziel This main file now contains a lot of the Wrapper and Slot/Fill logic. I ended up renaming a few things.

The main logic I changed is that the wrapper is now rendered around the slot, while in your PR the wrapper is rendered inside the fill:
https://github.com/WordPress/gutenberg/pull/22210/files#diff-2e071f8429cd2c500b830947ffde4239R226-R231

Pretty open to handling that differently, I think some aspects of this may need to be changed to handle adding the menu described in #22089.

@talldan I went for the "wrapper inside of the fill" approach to make something like this possible:

<BlockNavigationBlockFill> <BlockNavigationBlockSelectButton ref={ ref } block={ block } onClick={ onClick } isSelected={ isSelected } position={ position } siblingCount={ siblingCount } level={ level } { ...props } /> <EllipsisMenu {/* some props */} /> </BlockNavigationBlockFill>

With "wrapper outside of the fill" approach, these slots are only good for something like replacing the button entirely with RichText. You cannot the keep navigation item as a button and you cannot add any additional components.

Hmm, this is made difficult because this PR introduces the movers which are rendered after the fill, and then the designs in #22089 shown the dropdown menu after the movers.

I think most blocks will end up having the dropdown menu, so it probably makes sense to add that as an internal part of BlockNavigation, and then if we need to add custom items for particular blocks we can try a slot for the menu.

talldan · 2020-05-11T09:54:04Z

In case anyone is wondering, the above comments are in relation to resolving merge conflicts between this and the recently merged #21948

Fix doc examples Make components experimental Simplify creation of RovingTabIndex provider value Add storybook snapshots Add tests for RovingTabIndex, RovingTabIndexItem Add tests for TreeGrid components minor variable name change Format JS Import TreeGrid components from @wordpress/components Update styling for block navigator Try adding descender line styles Add some further finesse to styling Extract animated row to fix animation and use change detection that includes a full path so that all children animate Patch up code after rebase Use colSpan instead of colspan Fix row animation when moving a row quickly multiple times Remove descender lines from the accessibility tree Use useCallback for callback Use aria-hidden on purely presentational descender indicator in tree grid Update example Fix arrow key navigation when using Voiceover Only handle arrow key navigation when no modifiers are pressed Fix regressions in BlockMoverButton caused by rebase. Rename file to just button Name buttons in a consistent way Add screenreader description for focused block in navigator Add aria-describedby for appender button Fix selected block movers appearing with darker background Remove writing flow hack Fix typo Allow aria-setsize and aria-posinset on role=row in axe e2e tests Fix translator comment and use Add as the verb instead of Insert Add aria-label for navigation structure tree grid Fix typos Try to fix tests More typo fixes Update snapshot to reflect renaming of test description Update more e2e test classnames Rename components to more closely reflect `master`

paaljoachim · 2020-05-12T10:45:59Z

Awesome that movers are being added (have been added) to the block navigator!
Are there also plans for drag & drop? As it would be so nice to grab a hold of something and just drop it into the place where we need it.

draganescu · 2020-05-12T12:15:18Z

There doesn't appear to be any issue for this specifically, @paaljoachim Maybe you can open one, specifically for the block navigator?

paaljoachim · 2020-05-12T15:46:04Z

I went ahead and opened a new issue: #22297
(I added the Navigation and the Navigator, but can of course adjust to only focus on the Block Navigator.)

talldan added [Type] Enhancement A suggestion for improvement. [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Block] Navigation Affects the Navigation Block labels Oct 18, 2019

talldan requested review from draganescu, noisysocks and tellthemachines October 18, 2019 04:23

talldan self-assigned this Oct 18, 2019

talldan requested review from mtias and karmatosed October 18, 2019 04:24

talldan force-pushed the add/block-nav-movers branch from f1d13f3 to d751f8a Compare October 25, 2019 09:26

talldan added the Needs Accessibility Feedback Need input from accessibility label Oct 25, 2019

talldan marked this pull request as ready for review October 25, 2019 09:28

talldan requested review from ajitbohra, ellatrix, jorgefilipecosta, Soean and youknowriad as code owners October 25, 2019 09:28

talldan added the Needs Design Feedback Needs general design feedback. label Oct 25, 2019

mtias reviewed Oct 25, 2019

View reviewed changes

packages/block-editor/src/components/block-navigation/list.js Outdated Show resolved Hide resolved

talldan requested a review from shaunandrews October 25, 2019 09:46

mtias reviewed Oct 25, 2019

View reviewed changes

packages/block-editor/src/components/block-navigation/list.js Outdated Show resolved Hide resolved

youknowriad reviewed Oct 25, 2019

View reviewed changes

tellthemachines previously requested changes Oct 28, 2019

View reviewed changes

packages/block-editor/src/components/block-navigation/list.js Outdated Show resolved Hide resolved

packages/block-editor/src/components/block-navigation/style.scss Outdated Show resolved Hide resolved

tellthemachines reviewed May 8, 2020

View reviewed changes

tellthemachines approved these changes May 8, 2020

View reviewed changes

talldan force-pushed the add/block-nav-movers branch from 93b7b9b to 606c0b7 Compare May 8, 2020 08:26

talldan force-pushed the add/block-nav-movers branch 2 times, most recently from 9e80c67 to ade2759 Compare May 11, 2020 09:05

talldan commented May 11, 2020

View reviewed changes

talldan force-pushed the add/block-nav-movers branch from ade2759 to 3de8bd1 Compare May 12, 2020 01:21

talldan merged commit cea2a4f into master May 12, 2020

talldan deleted the add/block-nav-movers branch May 12, 2020 02:05

github-actions bot added this to the Gutenberg 8.2 milestone May 12, 2020

gziolo added [Type] Feature New feature to highlight in changelogs. and removed [Type] Enhancement A suggestion for improvement. labels May 12, 2020

talldan mentioned this pull request May 12, 2020

Enable the block movers in List View and add e2e tests #22287

Closed

talldan mentioned this pull request May 20, 2020

Keyboard in block navigation editable text not working correctly #22488

Closed

SchneiderSam mentioned this pull request Aug 4, 2020

List View: Drag & Drop #22297

Closed

gwwar mentioned this pull request May 11, 2021

Block Editor: update react-spring to 9.2.4 #30979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add block navigator movers #18014

Add block navigator movers #18014

talldan commented Oct 18, 2019 •

edited

Loading

draganescu commented Oct 18, 2019

talldan commented Oct 25, 2019

youknowriad left a comment

mtias commented Oct 25, 2019

mtias commented Oct 25, 2019

tellthemachines left a comment

talldan commented Oct 28, 2019

tellthemachines commented Oct 28, 2019

mtias commented May 7, 2020

tellthemachines May 8, 2020

tellthemachines left a comment

tellthemachines May 8, 2020

talldan May 8, 2020

tellthemachines May 8, 2020

tellthemachines May 8, 2020

diegohaz commented May 8, 2020 •

edited

Loading

diegohaz commented May 8, 2020

diegohaz commented May 8, 2020

tellthemachines commented May 11, 2020

talldan May 11, 2020

talldan May 11, 2020

talldan May 11, 2020

talldan May 11, 2020

adamziel May 11, 2020 •

edited

Loading

talldan May 12, 2020

talldan commented May 11, 2020

paaljoachim commented May 12, 2020 •

edited

Loading

draganescu commented May 12, 2020

paaljoachim commented May 12, 2020

Add block navigator movers #18014

Add block navigator movers #18014

Conversation

talldan commented Oct 18, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

draganescu commented Oct 18, 2019

talldan commented Oct 25, 2019

youknowriad left a comment

Choose a reason for hiding this comment

mtias commented Oct 25, 2019

mtias commented Oct 25, 2019

tellthemachines left a comment

Choose a reason for hiding this comment

talldan commented Oct 28, 2019

tellthemachines commented Oct 28, 2019

mtias commented May 7, 2020

Choose a reason for hiding this comment

tellthemachines 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

diegohaz commented May 8, 2020 • edited Loading

diegohaz commented May 8, 2020

diegohaz commented May 8, 2020

tellthemachines commented May 11, 2020

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

adamziel May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan commented May 11, 2020

paaljoachim commented May 12, 2020 • edited Loading

draganescu commented May 12, 2020

paaljoachim commented May 12, 2020

talldan commented Oct 18, 2019 •

edited

Loading

diegohaz commented May 8, 2020 •

edited

Loading

adamziel May 11, 2020 •

edited

Loading

paaljoachim commented May 12, 2020 •

edited

Loading