Try/2031 manual focus #2392

BoardJames · 2017-08-14T07:04:52Z

The purpose of this pull request is to try out an alternate idea for keyboard navigation based on techniques used in textbox.io and tinymce.

In the pull request the toolbar is not in the normal focus cycle so pressing tab / shift+tab will move between blocks.

The toolbar can be selected by pressing F10 while editing a block which will display and focus the toolbar.
Toolbar groups can be moved between by using tab / shift+tab and the focus will not leave the toolbars while in this mode.
Toolbar items can be moved between by pressing Right_arrow / Left_arrow and as with tab the selection will not leave the toolbar while in this mode.
To exit toolbar mode and reselect the editor press Esc.

Pressing Esc while the editor is selected will deselect the block as before (though I'm not sure that is a good idea from an accessibility perspective)

Note I also changed the block switcher so it uses the drop-down menu so we didn't have two implementations of drop-down menus.

Note that this is meant to address #2031

aduth · 2017-08-14T15:39:01Z

blocks/api/registration.js

@@ -68,6 +68,12 @@ export function registerBlockType( name, settings ) {
 		);
 		return;
 	}
+	if ( 'focusable' in settings && ! ( isFunction( settings.focusable ) || isBoolean( settings.focusable ) ) ) {


Can you explain the purpose of this focusable property? It's not clear to me.

We'll also want to include relevant documentation updates in docs/

If at all possible, it would be ideal to not impose this implementation detail onto block authors.

Sure!
Basically the problem is that the box around a block adds an additional element to the focus cycle that I wanted to remove but at the same time we need some way to focus blocks with no focusable components like the horizontal divider. Also with some blocks like the picture blocks they can be focused before you put the picture in as there is a button but after they have a picture there is nothing within the block to focus so the property needed to support a function.

Currently the property defaults to true so block owners will only have to care about it if their block has nothing focusable. I could also attempt to calculate post-render if there is anything focusable in a block and set the tabIndex of the surrounding div then.

I've thought about this and I think I will change the implementation to do the following:

Initially assume that no child components are focusable and set the state variable focusable to false

Render the block and set the tabIndex of the border based on the focusable state

After the block has rendered then calculate if any of the components within the block can be focused according to the rules here: https://stackoverflow.com/questions/1599660/which-html-elements-can-receive-focus

If the block border is focused and a child element is focusable than transfer focus to the first focusable child

Update the state variable focusable

I now calculate if the block needs a focusable border.

Note that I still need to figure out how to nicely handle selection when tabbing into a block that displays things when the focus property is set.
For example the "button" block has 3 focusable elements:

The button text

A link (only shown when the block is focused)

A button associated with the link (only shown when the block is focused)

This gives inconsistent results when tabbing backwards because the link and link-button are skipped over to select the button-text but they aren't when tabbing forwards because they are displayed once the button text has been selected.

aduth · 2017-08-14T15:40:17Z

editor/modes/visual-editor/block.js

@@ -51,13 +51,28 @@ import {
 	getMultiSelectedBlockUids,
 } from '../../selectors';

-const { BACKSPACE, ESCAPE, DELETE, UP, DOWN, LEFT, RIGHT, ENTER } = keycodes;
+const { BACKSPACE, ESCAPE, DELETE, UP, DOWN, LEFT, RIGHT, ENTER, TAB } = keycodes;
+const F10 = 121;


Should this constant be added to utils/keycodes?

Yes. I could not find the file where that was defined. I will add it.

aduth · 2017-08-14T15:48:59Z

editor/modes/visual-editor/block.js

+}
+
+function isToolbar( target ) {
+	return selectAncestor( target, '.components-toolbar, .editor-block-settings-menu, .editor-block-mover' ) !== null;


A component really ought not traverse the DOM, especially through nodes not immediately rendered by its own render, since it establishes dependencies between the components which are difficult to maintain and debug. It would not be very obvious to add a new entry to this list if we add another toolbar, for example.

Why do we not capture this event at the specific toolbar component(s) instead? If we then need to communicate this to another component, we can employ one of a number of strategies.

Aside: We have element-closest available as a polyfill for Element#closest.

I will read through those links and update my implementation appropriately - as you can probably tell I don't normally work with react.

It's a very tricky problem because the very purpose of what you're implementing is to manage the relationship between descendant components (i.e. moving from blocks into their toolbar). @youknowriad has been exploring some ideas in #2424 (#2421) that at least have an advantage of isolating these behaviors, if not eliminating the DOM querying.

This is also an area where it would be nice to have fully controlled focus state (related to ideas by @iseulde at #771). So instead of VisualEditorBlock finding and calling focus on the element directly, the toolbar dispatches the change in focus to the block and the relevant editable detects and calls focus on itself. This is a larger unsolved problem, and not one we really need to account for here.

aduth · 2017-08-14T15:52:37Z

editor/modes/visual-editor/block.js

@@ -252,7 +284,18 @@ class VisualEditorBlock extends Component {
 	onKeyDown( event ) {
 		const { keyCode, target } = event;

-		this.handleArrowKey( event );
+		//this.handleArrowKey( event );


Commented code should be removed.

Yep, I should probably remove the function it references too. I am still considering if it contains something I needed to implement. I think I may have removed arrow key navigation between blocks which is something that probably needs to be added back.

aduth · 2017-08-14T15:55:01Z

editor/modes/visual-editor/block.js

+			return;
+		}
+		const block = findDOMNode( this.node );
+		const isVisible = ( elem ) => elem && ! ( elem.offsetWidth <= 0 || elem.offsetHeight <= 0 );


What nodes are we worried about here that are 0 width / height?

It's a quick check to see if the element or any ancestors are hidden. It seems that sometimes React hides things rather than re-rendering and actually removing them (at least on Firefox).

aduth · 2017-08-14T15:57:21Z

editor/modes/visual-editor/block.js

+	}
+
+	handleToolbarTabCycle( event ) {
+		const { keyCode, shiftKey, target } = event;


Maybe we can create a separate component which manages groups of toolbars and the keyboard navigation between them? This code doesn't seem particularly relevant to block rendering.

That component could then listen to changes in block focus state.

Yeah, the implementation is currently far from ideal, lots of duplicated code is the main thing irritating me at the moment. I mainly wanted to get some other eyes on it to see if people liked or hated the new keyboard nav.

aduth · 2017-08-14T16:00:18Z

editor/modes/visual-editor/block.js

+		if ( allCycle.length > 1 ) {
+			const nextToolbar = ( targetIndex + 1 >= allCycle.length ) ? allCycle[ 0 ] : allCycle[ targetIndex + 1 ];
+
+			const firstFocusableItem = nextToolbar.querySelector( 'button, *[tabindex]' );


This isn't fully representative of finding first focusable, i.e. would match a disabled button, or tabIndex=-1.

Related effort ongoing in #2323

Good point, I will improve on this.

Note, tabIndex=-1 is exactly what I want to select in this case. I have made the selector avoid disabled buttons.

…ycles

codecov · 2017-08-18T05:54:43Z

Codecov Report

Merging #2392 into master will decrease coverage by 0.42%.
The diff coverage is 7.61%.

@@            Coverage Diff             @@
##           master    #2392      +/-   ##
==========================================
- Coverage   31.59%   31.17%   -0.43%     
==========================================
  Files         175      175              
  Lines        5307     5386      +79     
  Branches      916      943      +27     
==========================================
+ Hits         1677     1679       +2     
- Misses       3080     3131      +51     
- Partials      550      576      +26

Impacted Files	Coverage Δ
blocks/library/gallery/index.js	`33.33% <ø> (ø)`	⬆️
editor/block-settings-menu/index.js	`0% <ø> (ø)`	⬆️
components/toolbar/index.js	`100% <ø> (ø)`	⬆️
blocks/library/table/table-block.js	`8% <ø> (ø)`	⬆️
blocks/library/image/block.js	`0% <ø> (ø)`	⬆️
editor/block-mover/index.js	`0% <ø> (ø)`	⬆️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
blocks/url-input/button.js	`0% <0%> (ø)`	⬆️
editor/block-switcher/index.js	`0% <0%> (ø)`	⬆️
blocks/library/code/index.js	`50% <0%> (-7.15%)`	⬇️
... and 3 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 a9de9d2...0e533c2. Read the comment docs.

aduth · 2017-08-23T13:06:29Z

editor/modes/visual-editor/block.js

+			return;
+		}
+		const forward = keyCode === DOWN || keyCode === RIGHT;
+		const block = findDOMNode( this.node );


this.node should already be an HTMLElement, since it's assigned from the root <div>. You should only need findDOMNode if you're attaching a ref to another component, or when passing this as an argument (which would have the same result).

Ah, I just assumed that all react refs were some wrapper object.

aduth · 2017-08-23T13:12:32Z

editor/modes/visual-editor/block.js

@@ -210,7 +251,13 @@ class VisualEditorBlock extends Component {

 		// Deselect on escape.
 		if ( ESCAPE === keyCode ) {
-			onDeselect();
+			if ( isToolbar( target ) ) {


A few alternative ideas to reaching up through parent ancestry from the event target:

If what we're looking for already exists in the rendered element, we could check whether the target is contained within one of them. Something like:

const isToolbar = [ this.nodes.toolbar, this.nodes.blockSettings, this.nodes.blockMover ].some( ( node ) => node.contains( event.target ) )

Maybe it should be the toolbar, block settings, and block mover which handle the keypress and surface this up to VisualEditorBlock through a prop callback like onEscape.

onFocus could be called anywhere, including the toolbar, block settings, and block mover components, on their own "on escape press" handling

My preference is toward the second: For behaviors which are specific to component(s), it's nice to implement those behaviors within the component itself, and surface them to the parent component semantically through named prop callbacks.

I rejected option 1 as being inefficient.

Another option is for the toolbars to fire custom events instead of a prop callback. The problem with prop callbacks are that the toolbars are not the direct child of the block.

aduth · 2017-08-23T13:15:32Z

editor/modes/visual-editor/block.js


 function FirstChild( { children } ) {
 	const childrenArray = Children.toArray( children );
 	return childrenArray[ 0 ] || null;
 }

+function queryFirstTabbableChild( elem ) {
+	let i = 0;


Unnecessary assignment since the for on the next line will set it to 0 anyways. Could also move entire declaration into the loop:

for ( let i = 0; i < elem.childNodes.length; i++ ) {

Aside: let and const have different scoping implications than var, so conventions around assigning all variables top of function need not necessarily apply.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Scoping_rules_2

Good point, I will fix it.

aduth · 2017-08-23T13:17:49Z

editor/modes/visual-editor/block.js

+function queryFirstTabbableChild( elem ) {
+	let i = 0;
+	for ( i = 0; i < elem.childNodes.length; i++ ) {
+		const tabbableNode = queryFirstTabbable( { context: elem.childNodes[ i ] } );


Do we need this loop at all? Or function for that matter? What's the difference between this and:

const queryFirstTabbableChild = ( elem ) => queryFirstTabbable( { context: elem } );

That includes the element itself in the query so I can't use it like that. I tried passing an array but then I found that it silently discards all but the first element of the array (same with a selector despite using querySelectorAll internally which was quite confusing).

That includes the element itself in the query so I can't use it like that.

Digging through the docs and implementation of ally's queryFirstTabbable, it doesn't seem like this should be the case. Similarly in my testing I find by default the context is not considered:

https://jsbin.com/tubexul/3/edit?html,css,js,output

Seems like this only applies when using either defaultToContext or the underlying queryTabbable's includeContext (default false).

aduth · 2017-08-23T13:26:14Z

editor/block-switcher/index.js

-				}
-			</div>
+			<Toolbar>
+				<li>


Aside: Ideally a consumer of the Toolbar component should not care that it renders itself as a ul, and could pass children without the li. In the implementation of the Toolbar component, children can be achieved with React.Children.map:

<ul> { React.Children.map( ( child, i ) => ( <li key={ i }>{ child }</li> ) ) } </ul>

Mentioning as aside since it's not really relevant to the scope of your pull request.

It would make using the Toolbar with children slightly easier (and I have considered it), however it would also remove some control from the caller (ie to add classes to some li elements).

however it would also remove some control from the caller (ie to add classes to some li elements).

To me, this is not control we want to allow, or at least not through exposing li directly. There are a few alternatives, such as a prop on Toolbar, a wrapper for each children like ToolbarItem, a wrapper of the item itself (apply class to a div parent of DropdownMenu), or more advanced examples like function as child components.

Ultimately, I don't think we know enough about the actual use cases to need to concern ourselves with this control just yet though.

aduth · 2017-08-23T13:27:40Z

editor/modes/visual-editor/block.js

+		this.handleToolbarTabCycle( event );
+		this.handleToolbarArrowCycle( event );
+
+		if ( keyCode === F10 && ! ( event.altKey || event.ctrlKey || event.shiftKey || event.metaKey ) ) {


I like how you delegated specific logic to helper functions for handleToolbarTabCycle and handleToolbarArrowCycle. Any reason we shouldn't do the same for the behavior here?

No reason. I will put this in a function to make it more readable.

aduth · 2017-08-23T13:30:05Z

editor/modes/visual-editor/block.js

+			if ( this.props.isTyping ) {
+				this.props.onStopTyping();
+			}
+			this.props.onFocus( this.props.uid, { toolbar: true } );


Curious: Do we need toolbar focus to be a concern of the Redux focus state? I don't feel particularly strongly either way, but it seems like we could achieve the same effect with the component's local state. Unless we're ever moving focus from one block to another and assigning { toolbar: true } in the process (would this be handled well if it were to occur?)

The problem is that the toolbar is hidden most of the time so it must be shown before it can be focused. Since I had to use react to show it it seemed simplest to focus it using the react lifecycle.

aduth · 2017-08-23T13:36:33Z

editor/modes/visual-editor/block.js

+		const settingsMenu = filter( [ block.querySelector( '.editor-block-settings-menu' ) ], isVisible );
+		const moverMenu = filter( [ block.querySelector( '.editor-block-mover' ) ], isVisible );
+		const allCycle = flatMap( [ ...allToolbars, ...settingsMenu, ...moverMenu ], ( toolbar ) => {
+			return filter( Array.from( toolbar.querySelectorAll( '*[tabindex="-1"]:not(:disabled)' ) ) );


What is filter doing here?

If we did need it, I believe it's also capable of handling NodeList (in which case wouldn't need Array.from)

The Array.from came from the other keyhanding code that I deleted (and I see has since been moved into another file). The filter is there because sometimes toolbars are hidden by css media query rules and I figured I'd play it safe and check the focusable items in the toolbar for visibility too.

Oh, nevermind. I see that I forgot the predicate!

I have added the missing predicate to the filter function and removed the Array.from calls.

BoardJames · 2017-08-24T23:39:48Z

I removed or moved code so therefore coverage should go up?

anna-harrison · 2017-08-28T01:01:48Z

Hey @aduth : could you have a quick look at this one and clarify what is needed to merge this?

aduth · 2017-08-28T13:14:39Z

blocks/library/gallery/index.js

@@ -101,6 +101,7 @@ registerBlockType( 'core/gallery', {
 									buttonProps={ {
 										className: 'components-icon-button components-toolbar__control',
 										'aria-label': __( 'Edit Gallery' ),
+										tabIndex: -1,


With these changes, what are the implications on block authors for managing tab index? How do we document this? Can we avoid it?

Is it possible to hijack the tab keypress to avoid the default tabIndex behavior when in the tabbable cycle, so that we don't need buttons to be assigned with tabIndex="-1"?

aduth · 2017-08-28T15:26:06Z

My overarching concerns at this point are:

A block implementer should not need to concern themselves with the implementation details of the focus cycle, as we've started to require new tabIndex props in blocks here
VisualEditorBlock is a nightmare of a component which is already responsible for handling far too many behaviors. The intricacies of toolbar focus should ideally be isolated from block rendering.
DOM traversal via Element#closest escapes concerns of the component itself both in terms of reaching up through the DOM hierarchy and in having knowledge of the implementation details of its child components (VisualEditorBlock should not need to know that .components-toolbar is the class name assigned to <Toolbar />). This can become difficult to maintain in cases where any of these classes change, or new toolbar types are added, or one of these classes becomes assigned to an ancestor node.

BoardJames · 2017-08-29T05:19:01Z

@aduth

A block implementer should not need to concern themselves with the implementation details of the focus cycle

Unfortunately I think that is an impossible goal. There is always going to have to be some marker of things that can be focused and tabIndex is the one that is supported by browsers. Currently I have only modified tabIndex where it was needed to remove things from the tab cycle like buttons (which browsers automatically give an effective tabIndex of 0). I see no way of doing that automatically in all cases (barring a general AI). We might be able to do it automatically for buttons in tool bars but as you allude to in your other comments assuming things about child components is a dangerous thing to do.

VisualEditorBlock is a nightmare of a component which is already responsible for handling far too many behaviors. The intricacies of toolbar focus should ideally be isolated from block rendering.

I consider that the code controlling the block is the most logical place to control focus for the block.

DOM traversal via Element#closest escapes concerns of the component itself both in terms of reaching up through the DOM hierarchy and in having knowledge of the implementation details of its child components (VisualEditorBlock should not need to know that .components-toolbar is the class name assigned to ). This can become difficult to maintain in cases where any of these classes change, or new toolbar types are added, or one of these classes becomes assigned to an ancestor node.

Well I could filter the result returned by Element#closest using Node#contains to ensure that it is within the block but you don't seriously expect that a block would ever be wrapped in a class called 'components-toolbar'?!! Also the only way you are going to avoid using a selector on '.components-toolbar' is by passing out refs to the components which means you'd have to pass in a prop through the block to the toolbars (and you were complaining about giving extra responsibility to block owners before - how are you going to ensure that they do that?!)...

anna-harrison · 2017-10-09T03:27:30Z

@aduth : are you happy to close this one off?

aduth · 2017-10-10T16:49:39Z

The behavior may still be needed per #2031 , but my previously noted concerns still stand. Separately there has been some explorations of alternative toolbar placement like that in #2148 . I noticed that @youknowriad opened #2960 which looks to tackle some of the same tasks as those implemented here.

youknowriad · 2018-01-30T08:49:29Z

A lot has changed and most of the behaviors introduced by this PR already exist now.
Thanks for your work on this PR @EphoxJames I'm closing right now. Let's open separate PRs to tackle the remaining tasks.

tiny-james added 12 commits August 8, 2017 16:49

Fix dropdown menu so it can't be open and have nothing selected

aa66504

Make an open dropdown menu select forward/back with tab/shift+tab

14c9446

Make tabIndex of dropdown menu changeable

8bd904a

Make block switcher use dropdown menu

fdc699b

Remove toolbars and other things from the default tab cycle

7be9165

Merge branch 'master' into try/2031-manual-focus

2f86df0

Use tab to jump between toolbars

13d9cc0

Merge branch 'master' into try/2031-manual-focus

7664de5

Use arrow keys to cycle through toolbar items; Alt+F10 to select toolbar

763e186

Merge branch 'master' into try/2031-manual-focus

a99096e

Select toolbar with F10, deselect with Escape

8cc32a2

Merge branch 'master' into try/2031-manual-focus

c05dc9c

BoardJames requested review from youknowriad and afercia August 14, 2017 07:04

BoardJames mentioned this pull request Aug 14, 2017

Keyboard interaction: standardize the blocks behavior #2031

Closed

Added mover to the toolbar cycle

394371c

youknowriad requested review from aduth and ellatrix August 14, 2017 09:31

aduth reviewed Aug 14, 2017

View reviewed changes

tiny-james added 4 commits August 15, 2017 10:39

Add F10 to keycodes

829af3f

Take disabled buttons into account when creating toolbar navigation c…

c210a69

…ycles

Calculate if a block needs a focusable border

2770127

Merge branch 'master' into try/2031-manual-focus

fdc1120

tg-ephox mentioned this pull request Aug 17, 2017

Arrow navigation broken in Firefox #2124

Closed

tiny-james added 4 commits August 18, 2017 15:40

Remove unused code

5ae9a14

Remove unused imports to fix lint

6574e4b

Updated test to match new behaviour of tab key in drop-down menu

9dd13cf

Merge branch 'master' into try/2031-manual-focus

c70d4f8

BoardJames mentioned this pull request Aug 21, 2017

Pressing backspace doesn't delete an empty freeform block #1273

Closed

tiny-james added 8 commits August 21, 2017 15:14

Merge branch 'master' into try/2031-manual-focus

36bb0d1

Fix incorrect merge

7924dd7

Merge branch 'master' into try/2031-manual-focus

3432da5

Fix the code block so it will take focus on tab

b1f5896

Allow F10 to work to select standard buttons in blocks without toolbars

b31ffee

try/2031: Use Element.closest instead of hand rolled version

16021fa

try/2031: Allow hiding UI on ESCAPE before deselecting

aa77c25

Merge branch 'master' into try/2031-manual-focus

033d0f3

aduth reviewed Aug 23, 2017

View reviewed changes

tiny-james added 4 commits August 24, 2017 17:42

try/2031: Move toolbar selection code into its own function

69f5011

try/2031: Move let into more specific scope

84b183c

try/2031: Removed unneeded Array.from; Added a missing filter predicate

1facd3c

Removed unneeded uses of findDOMNode

c8f7363

aduth reviewed Aug 28, 2017

View reviewed changes

tiny-james added 2 commits August 29, 2017 13:29

Merge branch 'master' into try/2031-manual-focus

f3af11c

Fix missed dependency

0e533c2

aduth mentioned this pull request Aug 29, 2017

Framework: Extract arrow navigation to its own component #2424

Merged

anna-harrison assigned BoardJames Oct 6, 2017

youknowriad closed this Jan 30, 2018

gziolo deleted the try/2031-manual-focus branch May 7, 2018 11:15

Try/2031 manual focus #2392

Try/2031 manual focus #2392

Conversation

BoardJames commented Aug 14, 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

BoardJames Aug 15, 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

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

codecov bot commented Aug 18, 2017 • edited Loading

Codecov Report

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

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

BoardJames commented Aug 24, 2017 • edited by anna-harrison Loading

anna-harrison commented Aug 28, 2017

aduth Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Aug 28, 2017

BoardJames commented Aug 29, 2017

anna-harrison commented Oct 9, 2017

aduth commented Oct 10, 2017

youknowriad commented Jan 30, 2018

BoardJames commented Aug 14, 2017 •

edited

Loading

BoardJames Aug 15, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading

BoardJames commented Aug 24, 2017 •

edited by anna-harrison

Loading

aduth Aug 28, 2017 •

edited

Loading