Add keyboard navigation to the inserter. #578

BE-Webdesign · 2017-05-01T05:46:33Z

Fixes #515. This PR adds keyboard navigation to the inserter. This introduces RxJS as a library, which I feel could be very useful for other aspects of this project. Accessibility review would be awesome @afercia.

WIP: Need to fix testing errors which appear to be from the build, the PR should work. Need to fix weird RxJS issue. Also rebase.

Open questions:

Is it worth adding another dependency for RxJS? I think for a heavily active user interface like the editor we will probably want to add something like RxJS to help manage some of the interaction going on within the editor. This does however add more complexity for the project as RxJS is probably not widely used in the WordPress ecosystem compared to React and Redux.

Testing instructions:

Verify that after toggling an inserter menu open you can navigate through the block types buttons via keyboard shortcuts ( right arrow, down arrow, and tab move forward; left arrow, up arrow, and shift + tab move backward, through the list. ) As you cycle through the block types and the search field verify that the focus status is being properly updated. Press the escape key for toggling the inserter. When toggling the inserter the toggle button for that inserter should recieve focus.

Feedback

How can I improve my PRs?

afercia · 2017-05-01T15:41:35Z

@BE-Webdesign from an accessibility perspective, works great 🙂 Tested with Safari 10 + VoiceOver for now, should be tested at least also on Win Firefox + NVDA and IE 11 + JAWS.

I've made a quick video, after I've merged this PR with #527 which adds ARIA roles, properties, etc. See https://cloudup.com/cYMLChL68d4

The only thing I'd recommend to change is to don't move focus to the menu when it opens. Otherwise, the information about the expanded state you can see in the video would be lost. I think it's far better to keep the focus on the toggle button and let users continue the navigation following the natural tab order. Luckily, the menu gets injected immediately after the toggle.

Minor things I've noticed, you probably are already aware of:

getting this in my console: Warning: Unknown prop buttonRef on <button> tag. Remove this prop from the element.
the inserter should close also when clicking anywhere outside of it, I guess this should go in a separate issue?
I'd leave out the focus style, which is good for testing but maybe better to address it as part of a general issue for all the controls, see Focus style for any keyboard operable control #574

aduth · 2017-05-01T15:55:57Z

Is it worth adding another dependency for RxJS?

I'll admit to not being an expert on RxJS, or observables generally. It's worrying to me in its size (it quite literally doubles the production build), but more importantly in that thinking in observables is a departure for most JavaScript developers, and quite intimidating for the uninitiated (myself included). Not a blocker of course, but the "another thing to learn" factor is a real consideration, and I can't help but find it to be a large leap in the case of observables. Of course, the same can be said about Redux data patterns, so if it can be shown that there's a net simplification, I could be in favor of it.

I think for a heavily active user interface like the editor we will probably want to add something like RxJS to help manage some of the interaction going on within the editor.

Is there something about the current state management that you think won't be able to scale to manage the state of complex editor interactions?

BE-Webdesign · 2017-05-01T16:09:26Z

@afercia

I've made a quick video, after I've merged this PR with #527

I will check that out, thank you for making it.

The only thing I'd recommend to change is to don't move focus to the menu when it opens.

So keep the focus on the toggle button, until one of the navigation keys is pressed? Right now this is changing the focus to the elements as you move through them, which requires a lot of hoop jumping with refs etc. would it be better to just leave the focus on the inserter button the whole time? I am not fully clear what to do here.

getting this in my console: Warning: Unknown propbuttonRefon tag. Remove this prop from the element.

Yeah, maybe a seperate inserter button component will need to be made, the problem is that I couldn't figure out a better way of grabbing the ref to focus so I have a created a prop that might not always be there.

I'd leave out the focus style, which is good for testing but maybe better to address it as part of a general issue for all the controls, see #574

Take out the style?

BE-Webdesign · 2017-05-01T16:42:12Z

@aduth

It's worrying to me in its size (it quite literally doubles the production build)

Yeah RxJS is huge, luckily it has a number of ways to reduce the build size; down to selecting what methods you need etc. Right now, for convenience, I am adding the whole library, I will change this to be much much much smaller.

and quite intimidating for the uninitiated (myself included)

Don't be intimidated, everyone here is excellent and fantastically skilled. RxJS is just a tool no need to be intimidated. I think you may find Observables very appealing for a number of things on Gutenberg, and it is an excellent learning opportunity. If anyone would like to do a quick Skype/Hangout I would be glad to go over Observables and explain what they can help with on Gutenberg.

Not a blocker of course, but the "another thing to learn" factor is a real consideration, and I can't help but find it to be a large leap in the case of observables

I am aware and glad you a bringing it to light. I feel adding this keyboard navigation behavior without Observables would be really hard, so if you see a better alternative, I would love to know ( You know a lot more about React than I do, so maybe you see something I don't ). Working with Observables is fun once you get the hang of it and understand what problems they simplify, much like promises, but like React & Redux, it can be a 🐰 🕳 of complexity deep in the codebase.

I can't help but find it to be a large leap in the case of observables

Yeah the teaching materials out there are not great, I think I could explain them pretty well in a way that is more easily understood; or hopefully I can, maybe I am bad at explaining too 😄

Is there something about the current state management that you think won't be able to scale to manage the state of complex editor interactions?

Yes, anywhere you are adding setTimeout()s in the React Components, to have better event delegation, you would probably find working with RxJS amazing. React does not have great event delegation nor should it. RxJS Observables basically serve as a way to have ease of control over event delegation, which will make adding accessibility features a lot more manageable, and improve the overall user experience to be less glitchy. Right now we have a hidden bug around selecting blocks, that can most likely be solved by implementing RxJS. There are tons of places where we can use RxJS to simplify what we already have running, and as we approach the more complex actions in the editor, RxJS could step in to ease the implementation of those. To me when you are using RxJS, Redux, and React together you create a very compelling way to develop front end stuff and the beauty is you have the power to pick and choose what library you want to manage certain things and how you want them to be managed.

If contributor tools are ever developed, RxJS should probably be auto include for something like that, as it really shines with networking requests, i/o, and how that impacts the UI, but that is just a potential future use case. There are many many use cases to use it for in the editor currently and coming in the near future.

afercia · 2017-05-01T18:03:46Z

@BE-Webdesign yep, I meant to keep the focus on the toggle button and just remove:

// Set the component menu to have focus in DOM.
this.menuNode.focus();

When the menu opens, users can just tab into it and from that moment on, the magic happens 🙂

BE-Webdesign · 2017-05-01T18:19:46Z

Updated the build, will be back later to make the other changes.

BE-Webdesign · 2017-05-01T23:10:01Z

I thought of some other great benefits to RxJS, but by no means am I trying to shove it down anyone's throat. Observables can help us with managing the saving and auto-saving drafting etc of posts over the network and also make offline handling wayyyyy easier. It could improve upon the already existing heartbeat api and reduce that complexity as well.

BE-Webdesign · 2017-05-02T00:34:57Z

Okay this is ready for review now. I would love the opportunity to try and pitch RxJS, but I am not overly opinionated, so feel free to poo poo this idea.

BE-Webdesign · 2017-05-02T03:29:52Z

If we nix RxJS, I think other solutions could be to create a higher order component, or just replace what RxJS is doing with vanilla event handlers, but then you lose all of the RxJS goodness by doing the latter.

youknowriad · 2017-05-02T10:35:03Z

editor/components/inserter/menu.js

+
+	componentDidMount() {
+		// Build a stream of keydown events on component mount.
+		this.keyStream$ = Observable.fromEvent( document, 'keydown' );


I'm not totally against rxjs but I have two concerns:

The "another thing to learn" for new comers,

Consistency: Some event handlers (see Editable for example) use simple Event binding while other components like this one uses RxJS.

I don't want to be that person that's always reluctant to changes, but I feel that this PR should be focused on the inserter keyboard interaction using the current way we handle events (simple event bindings), and maybe move the discussion about RxJS into a separate PR/issue.

Sounds good. I can take RxJS out and open a separate issue. The only problem is we might need to do some event normalization around this for better device compatibility. Most of the event handlers we are using are either with TinyMCE and React's normalized event system. This PR is an example where we would need to bind to the document to listen for keydowns for this component, and was initially why I thought it would be appropriate to use RxJS. I will see if I can get React's events to do the same thing, but it might take me a while :).

I don't want to be that person that's always reluctant to changes

I think everyone is like that from time to time. It is good in my opinion to be reluctant, maybe we will decide that RxJS is not the right tool for this project, and it will be for the better.

youknowriad · 2017-05-02T10:43:28Z

editor/components/inserter/helpers/index.js

+	return focusables[ nextIndex ];
+};
+
+export const focusNextFocusableRef = ( component ) => {


What's the value of leaving these two functions focusNextFocusableRef, focusPreviousFocusableRef outside of the component?

There is none really, I like them in a separate file. I would be glad to put it in one if that is better.

youknowriad · 2017-05-02T10:46:39Z

editor/components/inserter/helpers/index.js

+	return list;
+};
+
+const nextFocusableRef = ( component ) => {


Instead of having a signature like this component => block which is not clear to me because the component should implement a specific interface for the function to work properly. I'd prefer something like ( currentBlock, blocks) => block.

Also, should we test these functions?

Also, should we test these functions?

Yeah.

( currentBlock, blocks) => block.

I will have to look at it. I can't remember, but there was some reason why the component was needed; maybe not. I will look at it again.

youknowriad · 2017-05-02T10:50:31Z

editor/components/inserter/index.js

+					className="editor-inserter__toggle"
+					aria-haspopup="true"
+					id="inserter-toggle"
+					buttonRef={ ( node ) => this.nodes.toggle = node }


Could you clarify why do we need this?

This is to grab the reference for the icon button. I couldn't figure out another way to do this. When the menu closes we need to refocus the inserter button for accessibility. I would definitely like to find something better, but would need help in figuring out something different.

@youknowriad Before I patch this up, do you have a better solution for this?

I'm suspecting the behaviour here won't be ideal after the rebase and the "clickOutside" behaviour. Do we really want to refocus the inserter button if we click outside?

But I'm trying to think what could we do here. Maybe we should transform the IconButton component into a class component instead of a functional component to allow ref on it?

We'll need a focus method implemented in the IconButton also I guess.

IconButton component into a class component instead of a functional component to allow ref on it?

If we do that how to we get the ref to the button element deep in the tree that needs to be focused.

We'll need a focus method implemented in the IconButton also I guess.

This sounds like something that could work a bit better pass in a prop of some sort and track the inserter button focus as part of the Inserter's state? Based on that state the button would be focused or not focused.

Do we really want to refocus the inserter button if we click outside?

The two will be independent, so if you click outside no focus, unless if you hit the toggle. If you close the menu purely with the keyboard, it will focus the toggle button for better accessibility.

afercia · 2017-05-02T14:16:00Z

I really don't know anything about RxJS. For accessibility though, there's the need of some sort of focus-management tools to allow basic stuff like getting the focusable elements of a component, optionally filter them, return the first and the last one, store the element where focus should be moved back to, etc. How to achieve that, I'd leave it to the experts here 🙂

BE-Webdesign · 2017-05-02T14:30:10Z

I really don't know anything about RxJS

Yeah, I will take it out of this PR and open a separate issue.

aduth · 2017-05-02T20:18:56Z

Yes, anywhere you are adding setTimeout()s in the React Components, to have better event delegation, you would probably find working with RxJS amazing.

For this specific example, I agree that setTimeout is often a poor choice, but I usually find it's used as a bandaid of symptoms, and that the solution is not necessarily alternative asynchronous control mechanisms, but rather a better understanding of the synchronous series of steps that we're assuming to occur in the remainder of the current tick (e.g. "waiting" for other event handlers to have been exhausted, "waiting" for a rerender to happen as a result of our changes).

BE-Webdesign · 2017-05-02T21:03:42Z

Yeah RxJS's strong point is in i/o and networking, and complex interactions. I think there will be many complex interactions for this project, by providing a truly top notch user experience.

I am going to take RxJS out of this PR and open a separate ticket, as I agree it doesn't belong and might not be necessary at all to this project. I will probably do a PR showing how our saving flow can be simplified by using RxJS and open new user experience opportunities. Hopefully the ticket clarifies some of the potential benefits if we come across those kinds of problems. I will try and outline the benefits without getting too wordy. I wrote a big response and just deleted it lol, as nobody got time to read that. Hopefully I can explain my ideas better in an issue.

BE-Webdesign · 2017-05-03T00:44:20Z

Okay removed RxJS and consolidated everything into the component. It is now one giant component.
In hindsight RxJS was overkill for this issue, but this probably won't have as consistent behavior across all browsers/devices. It should still work fine. Will open up an issue about RxJS moving forward as I still believe it would be good to use.

Questions still to be answered:

What is a better way to access the inserter toggle button's ref?

youknowriad

I think it's better (more understandable) now, but I wonder why you're relying on the DOM nodes (or refs) to find the next/previous block to focus etc...

In general, in the React world, we prefer to work on the data and leave the DOM node as a side effect of rendering the data (last call)

youknowriad · 2017-05-03T08:47:23Z

editor/components/inserter/menu.js

+		} );
+
+		// Focus the DOM node.
+		this.nodes[ refName ].focus();


Is there a reason why we're leveraging the "focus" to select a node? Is this for accessibility or something?

I guess I'd prefer to avoid the imperative approach and use a className or something.

Keyboard navigation with arrows means focus must be moved to the DOM element, this needs to happen on any "widget" that implements arrow navigation.

This would have been a lot easier if you didn't need to actually focus any of the nodes. It is not pretty, but I don't have a better solution.

youknowriad · 2017-05-03T08:50:22Z

editor/components/inserter/menu.js

+		const focusables = [];
+
+		for ( const refName in nodes ) {
+			if ( this.isShown( filterValue, refName ) && 'search' !== refName ) {


I think we should have a unique isShownBlock method to check for visibility, because we may want to change the way we filter the blocks into a more complex way (keywords, title...). We already talked about moving this filtering function into the blocks module.

youknowriad · 2017-05-03T08:53:11Z

editor/components/inserter/menu.js

+	}
+
+	nextFocusableRef( component ) {
+		const focusables = this.visibleFocusables( component.nodes, component.state.filterValue );


In all these methods, we should not use the nodes as source of truth, we should use the blocks instead and we should not use store the focusedElementRef but store block.blockType instead.

The DOM should be just the last side-effects to trigger (maybe when calling focus if this is required)

It's all just data, but I can use blockTypes, the two are interlocked in this case. What if we add nodes that somehow have nothing to do with blocks in the future that need to be focused through, this functionality all deals with focusable nodes, we aren't touching the dom but merely nodding to its existence and using that as a data source. Anyways going to switch it to blockTypes as it probably makes more sense.

youknowriad · 2017-05-03T08:55:45Z

editor/components/inserter/menu.js

+		}
+
+		// Add search regardless at the end.
+		focusables.push( 'search' );


I assume this is a fallback? I'd prefer if the visibleFocusables or maybe better getVisibleBlocks was data-centric and the focus fallback was not merged with the data

The search field is a focusable for this, and is why I didn't name it blocks. So you will always have the search field in the list of focusables. As you move through the list of focusable elements in the inserter you always want the search field there. Not 100% sure what you want changed.

If you take a look at the Slack "inserter" or the MacOS emoji keyboard which are the inspiration for this feature @jasmussen you'll notice the search input is always focused when we navigate using the arrows.

But Event in the case we make the search input focusable I'd base my logic on the data instead of the dom nodes:

// Compute the block to select based on the state const visibleBlocks = this.getVisibleBlocks(); const blockToSelect = this.getNextBlock( currentBlock, visibleBlocks ); this.setState( { selectedBlock: blockToSelect } ); // Apply the result to the DOM if ( blockToSelect ) { const blockNode = this.nodes[ blockToSelect.blockType ]; blockNode.focus(); } else { this.searchInput.focus(): }

My idea is always the same, compute the block to focus on the data (not the dom nodes) and apply the results to the dom nodes.

The two logic is decoupled from the rendering. that way, we could imagine moving these "state" computations to Redux for example (like you suggested, if it's needed elsewhere)

That looks better to me. Should we try it with the search input always focused and make it slack style with the /? Or for now just leave it as is and change everything relative to the blockTypes?

I personally don't have any preference here. It's probably a design/accessibility decision. I'm ok with leaving this as is for now.

Okay sounds good, thank you for the input as it will be a lot better now!

youknowriad · 2017-05-03T08:58:49Z

editor/components/inserter/menu.js

+		}, {} );
+	}
+
+	bindReferenceNode( nodeName ) {


blockType instead of nodeName?

This is used to also bind the search field. The search field of the inserter is not a blockType so I used nodeName instead is blockType preferred?

Yeah, I know, what I'm proposing is not mixing the refs in the same array, the blocks nodes are in the same array, and the input in its own property.

Sounds good.

youknowriad · 2017-05-03T09:02:27Z

Also, in the UI prototype, https://wordpress.github.io/gutenberg/ Navigating using the arrows up/down don't move to the next/previous block on the list but follows a "visual" direction instead. The algorithm for this is more complex though. I wonder what @jasmussen would like to see here.

BE-Webdesign · 2017-05-03T15:01:22Z

Also, in the UI prototype, https://wordpress.github.io/gutenberg/ Navigating using the arrows up/down don't move to the next/previous block on the list but follows a "visual" direction instead. The algorithm for this is more complex though. I wonder what @jasmussen would like to see here.

From first glance, it seems glitchy as you click an arrow down key you can move diagonally even though there are elements directly below. My guess is that on a keydown or key up press it skips two elements in the stack, so when only one element is in a row the next keydown will skip diagonally. I think what we have right now from an accessability perspective works well. When the menu opens it lists all of the focusable elements in order, as the user clicks the next key they go to the next element in that sequence. nextFocusable() can always be changed as well too. If you want me to do an algorithm that works for that style I can, I don't know how that will be accessability wise, and it will also be dependent on the stylings we have running. (i.e. 2 column vs. 4 column vs. 3 column )

afercia · 2017-05-03T16:15:56Z

I'm suspecting the behaviour here won't be ideal after the rebase and the "clickOutside" behaviour. Do we really want to refocus the inserter button if we click outside?

From an a11y perspective, when clicking, an element that has focus should lose focus and if the click is on a focusable element, that one should get focus. That's the native behavior.
However, when the inserter is open and when pressing Escape to close it, focus should be moved back to the inserter button.

BE-Webdesign · 2017-05-05T13:17:35Z

If the search must be the primary action, then maybe we should re-consider the design here and move the search field to the top. This is something we've already done in WP in some cases, for example when we've moved the comment form textarea to the top.

We can move it to the top and use flexbox order to still make it appear the same way, how does that sound?

Perhaps we can make it so that whenever you manually click (or tab and press enter) the inserter, it behaves as it is in this branch. Whenever you invoke it using the commandline /, you automatically set focus on the search. Would that be a good compromise?

That is a definite possibility and is probably what I would recommend based on afercia's accessibility notes.

jasmussen · 2017-05-05T13:25:14Z

Whether we move the search to the top (when editor bar inserter is clicked) and focus it, or just focus it when invoked from the inline inserter button, I'd think both of those would be pretty good.

afercia · 2017-05-05T17:38:45Z

Perhaps we can make it so that whenever you manually click (or tab and press enter) the inserter, it behaves as it is in this branch. Whenever you invoke it using the commandline /, you automatically set focus on the search. Would that be a good compromise?

Hm sounds to me as a bit forced assumption. Actually there's no way to distinguish who is going to use a pointing device or an input device, not to mention the great variety of alternative devices around. Moving focus programmatically is in the vast majority of the cases just bad for accessibility. It should be done only when there is a single, very specific, task to accomplish. For example, a modal opens with an input field to fill in. This is not the case with the inserter, where there's not one single specific task, there are many. It's basically a composite widget with a menu (that requires arrow navigation) and a search field. I'm still convinced the best option here is to don't move focus and exclude the search field from arrow navigation.

We can move it to the top and use flexbox order to still make it appear the same way, how does that sound?
A specific WCAG guideline states (and for good reasons) that the visual order should match the source order.

jasmussen · 2017-05-05T17:51:56Z

I don't agree that it's a forced assumption, but since it isn't fully implemented yet, I can't demonstrate it for you. Besides, it's not a discussion that should hold this PR from being merged, as this is fine to go without it.

afercia · 2017-05-05T18:07:59Z

Agreed this shouldn't block this PR 🙂 I'll open a separate issue, as moving focus and skipping relevant content is a blocker for accessibility.

BE-Webdesign · 2017-05-05T19:06:13Z

I'm still convinced the best option here is to don't move focus and exclude the search field from arrow navigation.

Great feedback and I want to get this right afercia, but I am slightly confused. If you can never focus the search bar, how can anybody use it strictly via arrow navigation? Can you clarify? Is the way it is currently working on this branch not proper? It is not fully clear what the existing issue is.

don't move focus

To double check you mean don't move focus to the search field automatically, keep focus on the toggle button of the inserter when first opening and closing the inserter menu, like this branch currently does?

afercia · 2017-05-05T19:23:39Z

@BE-Webdesign I mean programmatically.
Apart from the arrow navigation that needs to be there, the tab order and focus behavior should just be native. Keep it simple, don't touch anything 🙂

BE-Webdesign · 2017-05-05T19:26:45Z

Apart from the arrow navigation that needs to be there, the tab order and focus behavior should just be native. Keep it simple, don't touch anything

So no tabindex=-1s on the menuitems? That would preserve the native tab ordering for the menu item buttons and search field. Then the arrow keys could be made programmatic to cycle through the list of menu items matching focus to the selected button and never change focus into the search field, only a tab could get you there. Is that the desired functionality?

BE-Webdesign · 2017-05-05T20:33:11Z

Also if anyone wants to merge this please do so, this can always be iterated on in the future.

afercia · 2017-05-05T20:36:39Z

Will try to be more specific. Sorry, I'm not a native English speaker, just trying to do my best so maybe I was unclear.

Apart from the arrow navigation that needs to be there
this is handled with JavaScript and that's OK. tabindex=-1 is necessary when handling focus with JS especially when using role=menu and role=menuitem. Technically the tabindex attribute should be managed as described here: https://www.w3.org/TR/wai-aria-practices/#kbd_roving_tabindex
but as far as I know keeping it always to -1 is fine

the tab order and focus behavior should just be native. Keep it simple, don't touch anything
what I meant here is when:

tabbing from the inserter toggle button to the menu should be native behavior
tabbing to the search field should be native behavior

BE-Webdesign · 2017-05-05T20:45:23Z

Sorry, I'm not a native English speaker, just trying to do my best so maybe I was unclear.

No problem. These are hard ideas to express regardless of any language barrier; you do an excellent job.

https://www.w3.org/TR/wai-aria-practices/#kbd_roving_tabindex

Thank you for sharing, I will look that over.

tabbing from the inserter toggle button to the menu should be native behavior
tabbing to the search field should be native behavior

I think we are on the same page now, I will revise at some point tomorrow night and hopefully it will meet those standards.

BE-Webdesign · 2017-05-07T02:53:42Z

@afercia,

After reviewing the information you provided and as much a11y information as I could find regarding this type of interaction, I am unable to find anything that would put this as not WCAG 2.0 compliant. Is there something specific about how this branch currently works, that needs to operate differently?

jasmussen · 2017-05-08T09:57:41Z

I see approval here, so I'm merging this. If something still needs to be addressed, we can open separate tickets.

aduth

When tabbing through the inserter items, "Pullquote" is skipped until the very end of the list

aduth · 2017-05-08T15:53:48Z

editor/components/inserter/index.js

@@ -20,6 +20,10 @@ class Inserter extends wp.element.Component {
 	}

 	toggle() {
+		if ( this.state.opened ) {
+			this.toggleNode.focus();


Can you explain the need to force focus back on the button when closing the inserter menu?

My main worry here is the passing the ref callback as a prop. ref alone is undesirable but sometimes necessary escape from the virtual DOM into the "real" DOM within a component; expanding this further in allowing a parent to access its child's DOM compounds fragility.

Accessibility. If someone closes the menu and focus doesn't return to the button it gets confusing to navigate again.

I would have preferred to not write this code, but couldn't think of a better way to do it.

Chiming in to remind these kind of focus management is a basic accessibility requirement. Without that, focus would be lost because we've just removed the previously focused element from the DOM. More info of this, for example, here: https://www.w3.org/TR/wai-aria-practices/#kbd_focus_discernable_predictable

We don't necessarily need to remove the DOM element; we could apply styling conditionally to hide the element, if that helps retain focus. I've not dug too far into it, but perhaps also changing the inserter's root element to be the one that receives focus (instead of separately its button and list children) could help avoid the need for a ref prop.

Hiding it with display: none or visibility: hidden would be the same, since the menu items would be not focusable and this would cause a focus loss. Hiding by other means (off-screen, etc.) it's not what we need because when pressing Escape we want the menu to close and focus be moved to a reasonable place, which is the control that opened the menu.
I'm sorry React is not able to handle this kind of things graciously, that's one of the reasons I say it wasn't designed with accessibility in mind.
Also noting we'll need more of this kind of interaction in other places, see for example the block switcher.

I'm sorry React is not able to handle this kind of things graciously, that's one of the reasons I say it wasn't designed with accessibility in mind.

What does React have to do with focus loss caused by elements being removed or hidden?

@aduth 😑 I mean React doesn't have a native, easy, way to handle things like this one. To my understanding, you all don't like the way you had to implement this focus management, no?

@aduth 😑 I mean React doesn't have a native, easy, way to handle things like this one. To my understanding, you all don't like the way you had to implement this focus management, no?

Regardless of tool, it's an undesirable complexity to programmatically manage focus as we've found to need here.

I really don't have strong opinions about the tool you'll chose to implement this. This is an accessibility requirement though, and needs to be implemented.
I'd also remind everyone that, as I've noted previously a few times, this kind of focus management will be necessary in other places too.

aduth · 2017-05-08T16:02:42Z

editor/components/inserter/menu.js

@@ -12,11 +13,59 @@ import Dashicon from 'components/dashicon';
 class InserterMenu extends wp.element.Component {
 	constructor() {
 		super( ...arguments );
+		this.blockTypes = wp.blocks.getBlocks();
+		this.categories = wp.blocks.getCategories();


Why don't we just call to getBlocks and getCategories directly where they're used, specifically in the render? Even if there's a caching concern, it's usually discouraged to duplicate the source of truth because it's difficult to keep in sync (as noted by @youknowriad).

https://github.com/facebook/react/blob/8cac523/docs/tips/10-props-in-getInitialState-as-anti-pattern.md

aduth · 2017-05-08T16:07:54Z

editor/components/inserter/menu.js

+	onKeyDown( keydown ) {
+		if ( this.isNextKeydown( keydown ) ) {
+			keydown.preventDefault();
+			this.focusNext( this );


Why do we pass this as an argument? this would still be available in the body of the function being called.

Just a hangover from when these were separated out into another file, I can change. I get lazy I guess.

aduth · 2017-05-08T16:09:34Z

editor/components/inserter/menu.js

+	}
+
+	isNextKeydown( keydown ) {
+		return keydown.code === 'ArrowDown'


The code property is only available in Chrome and Firefox:

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code#Browser_compatibility

Even though it's now deprecated, which or keyCode has best compatibility for the browser's we support.

Ah maybe that is the source of some of the a11y problems. Un-normalized events are no fun.

For some reason GitHub won't let me reply to the comment above.

Why don't we just call to getBlocks and getCategories directly where they're used, specifically in the render? Even if there's a caching concern, it's usually discouraged to duplicate the source of truth because it's difficult to keep in sync (as noted by @youknowriad).

I was kind of making an assumption, maybe a bad one, that this will never change across an initialization. What are the use cases in which we would want blockTypes and categories to change on the fly? I would think we want blockTypes and categories to be set on initialization, then any shape of that data can be generated on the fly, which is what is happening here.

aduth · 2017-05-08T16:11:28Z

editor/components/inserter/menu.js

+			return blockTypes[ 0 ].slug;
+		}
+
+		const currentIndex = blockTypes.findIndex( ( blockType ) => currentBlock === blockType.slug );


We don't use the Babel polyfill currently, meaning we don't have access to ES2015 base type instance methods like Array#findIndex, String#startsWith, etc. Static members like Object.assign and Array.from are fair game however.

We could decide to use the polyfill, but it adds non-trivial weight to the build. The developer experience implications could make it worth including.

How about using lodash again?

How about using lodash again?

Yep, that's perfectly appropriate in my book:

https://lodash.com/docs/4.17.4#findIndex

aduth · 2017-05-08T16:14:45Z

editor/components/inserter/menu.js

+		 * Left and right arrow keys need to be handled seperately so that
+		 * default cursor behavior can be handled in the search field.
+		 */
+		if ( this.isArrowRight( keydown ) ) {


I'm not overly concerned about it, but I wonder if it would have really been any less clear to just do a switch statement here instead of extracting out to a number of additional instance methods. Like:

switch ( event.keyCode ) { case 39 /* ArrowRight */: if ( this.state.currentFocus !== 'search' ) { this.focusNext(); } // ... }

Yeah, I like the use of a switch statement, slight optimization, and possibly more easy to read.

aduth · 2017-05-08T16:16:40Z

editor/components/inserter/menu.js

+	}
+
+	findPrevious( currentBlock, blockTypes ) {
+		/**


There's a fair bit of overlap between findNext and findPrevious implementations. I'm wondering if it could be consolidated into a single one, or at least a single base implementation with aliases. Something like:

findByIncrement( increment = 1 ) {

Yeah that could work, it could either be a boon or add more work in the future, but I think this is a good idea.

aduth · 2017-05-08T16:17:27Z

editor/components/inserter/menu.js

+			return;
+		}
+
+		const nextBlock = this.findPrevious( currentBlock, sortedByCategory );


We should treat state as a single source of truth; since findPrevious has access to this.state, there's no need to pass it as an argument.

aduth · 2017-05-08T16:20:30Z

editor/components/inserter/menu.js

+
+	groupByCategory( blockTypes ) {
+		return blockTypes.reduce( ( accumulator, block ) => {
+			// If already an array push block on else add array of block.


We could simplify this somewhat by just creating the array if not exists and using the same push and return behavior.

return blockTypes.reduce( ( accumulator, block ) => { // If already an array push block on else add array of block. if ( ! accumulator[ block.category ] ) { accumulator[ block.category ] = []; } return accumulator[ block.category ].concat( block ); }, {} );

Alternatively, maybe this could be more easily achieved with Lodash's _.groupBy.

I was considering just using lodash as well.

BE-Webdesign · 2017-05-08T17:31:50Z

When tabbing through the inserter items, "Pullquote" is skipped until the very end of the list

#643 is the issue surrounding this. Without a default sort order it is somewhat ambiguous how the blocks are supposed to be sorted, the sort function I added isn't working properly ( wrong predicate after I changed it to be in one function oops ). Currently the order of library/blocks/index.js is what dictates the order, and while the sort function is broken it follows that order exclusively.

jasmussen · 2017-05-09T14:53:37Z

I think we are overthinking this implementation for something that hasn't even been alpha tested, and is likely to undergo lots of UI changes still. There's every chance we will have a tabbed interface, or only a single column instead of two columns.

For that reason, making the alpha version fully accessible is neither a blocker nor a priority. Keeping the code simple, so we can stay agile and respond to test results, however, is a very high priority. I am not saying that to sound dismissive, @afercia, your feedback is always crisp and understandable.

For now, the behavior of the inserter that we want to test is fairly simple: you click the inserter, you click a block, and a block is inserted in the content.

There are a few additional behaviors:

you click the inserter, use arrow keys and enter to insert
you click the inserter at the top, then click the search field to narrow it down
you click the inserter on the side, and focus is automatically set on the search field
you invoke the inserter on a newline with / and focus is automatically set on the search field

Those additional behaviors are also not blockers for the alpha test, and are perfectly fine to address separately.

By hyper-optimizing at this chrono-juncture, we are doing ourselves a disservice with the added complexity. What we need to be doing is keeping things as simple as we possibly can, take notes (tickets) along the way, and once we have an interaction that we feel is 100% solid, we can optimize it.

afercia · 2017-05-09T15:25:40Z

@jasmussen sure, I've already agreed its not a blocker for this PR.

However, if I'm told a required accessibility feature is an undesirable complexity to programmatically manage focus, I'm sorry buy I must remind you this kind of focus management is a required feature and the lack of such a feature will be a blocker later.
I could argue that the undesirable complexity here is actually a limitation of the tool being used. All I can say is I'm sorry React doesn't make this kind of things easier.

aduth · 2017-05-09T17:35:28Z

@afercia It's a fact of the matter that it introduces complexity, and I made no statement questioning whether it's a requirement or not. The fact of the matter is that complexity is introduced by way of manual management of DOM state, a fragile workflow that most modern frameworks discourage but still expose in one way or another.

I questioned your faulting of React as an unfair assessment of what is fundamentally a DOM management concern, and not a limitation of the tool itself. Of course, I'm open to being convinced otherwise if evidence can be shown that it's a problem specific to React, or if alternatives can be suggested which provide better mechanisms for handling this behavior.

mtias · 2017-05-09T19:03:49Z

Please, remember the act of creating something is full of trade-offs. We cannot assume changes have no costs, the same way we should not assume hesitance towards incurring on a cost early on is a disregard for the functionality itself. Stating a specific implementation brings about increased complexity doesn't mean the need for it is being cast aside. There are many crucial things to get right for this project—accessibility being one of them—and we need to build them in pieces that we are confident are improving the whole.

Whatever is considered a requirement for accessibility should be clearly delineated in issues, yet it also fundamentally depends on an agreement on a certain design direction, which precedes the focus on implementing the accessibility aspects of that functionality. Like @jasmussen mentions, we may drop the two-column block presentation, making a lot of the keyboard interactions developed here (thanks to @BE-Webdesign work) partially obsolete. We will have to weight options along the way and we ought to consider each accessibility aspect of a feature on its own merit and difficulties, like returning focus to the inserter button on escaping the menu.

Ultimately, we can do whatever we want with code. I don't see React working against us in any way; if anything, it's abstracting so much of the process of rendering to the DOM that when we do direct manipulation it sticks out prominently.

BE-Webdesign force-pushed the keyboard-inserter-navigation branch from 4aef0bb to 367b8c1 Compare May 1, 2017 05:51

BE-Webdesign force-pushed the keyboard-inserter-navigation branch from 367b8c1 to 59f366a Compare May 1, 2017 18:18

BE-Webdesign force-pushed the keyboard-inserter-navigation branch from 59f366a to c655685 Compare May 2, 2017 00:33

youknowriad reviewed May 2, 2017

View reviewed changes

BE-Webdesign force-pushed the keyboard-inserter-navigation branch from b476bfa to 6f44e52 Compare May 3, 2017 00:30

youknowriad reviewed May 3, 2017

View reviewed changes

mtias added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels May 3, 2017

BE-Webdesign force-pushed the keyboard-inserter-navigation branch from 6f44e52 to 3a7a9bb Compare May 3, 2017 23:03

BE-Webdesign mentioned this pull request May 5, 2017

Prevent scroll bleed in the inserter #656

Closed

afercia mentioned this pull request May 5, 2017

Avoid to move focus programmatically: Inserter menu and everywhere #685

Closed

This was referenced May 6, 2017

Block switcher a11y improvements #695

Closed

Add arrow key navigation to the block switcher #696

Closed

jasmussen merged commit a72b370 into master May 8, 2017

jasmussen deleted the keyboard-inserter-navigation branch May 8, 2017 09:57

aduth reviewed May 8, 2017

View reviewed changes

This was referenced May 10, 2017

Clean up keyboard navigation for the inserter. #715

Merged

Restore Inserter menu focus by tracking opening node #786

Merged

aduth mentioned this pull request May 31, 2017

Add/147 table block (alternate vision) #850

Merged

afercia mentioned this pull request Apr 21, 2018

Unexpected reverse-tab behaviour #1934

Closed

Add keyboard navigation to the inserter. #578

Add keyboard navigation to the inserter. #578

Conversation

BE-Webdesign commented May 1, 2017 • edited Loading

afercia commented May 1, 2017 • edited Loading

aduth commented May 1, 2017

BE-Webdesign commented May 1, 2017

BE-Webdesign commented May 1, 2017 • edited Loading

afercia commented May 1, 2017

BE-Webdesign commented May 1, 2017

BE-Webdesign commented May 1, 2017

BE-Webdesign commented May 2, 2017

BE-Webdesign commented May 2, 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

BE-Webdesign May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign May 2, 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

BE-Webdesign May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented May 2, 2017

BE-Webdesign commented May 2, 2017

aduth commented May 2, 2017

BE-Webdesign commented May 2, 2017

BE-Webdesign commented May 3, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad May 3, 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

youknowriad commented May 3, 2017

BE-Webdesign commented May 3, 2017 • edited Loading

afercia commented May 3, 2017 • edited Loading

BE-Webdesign commented May 5, 2017 • edited Loading

jasmussen commented May 5, 2017

afercia commented May 5, 2017

jasmussen commented May 5, 2017

afercia commented May 5, 2017

BE-Webdesign commented May 5, 2017 • edited Loading

afercia commented May 5, 2017

BE-Webdesign commented May 5, 2017 • edited Loading

BE-Webdesign commented May 5, 2017

afercia commented May 5, 2017

BE-Webdesign commented May 5, 2017

BE-Webdesign commented May 7, 2017

jasmussen commented May 8, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign May 8, 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

BE-Webdesign commented May 1, 2017 •

edited

Loading

afercia commented May 1, 2017 •

edited

Loading

BE-Webdesign commented May 1, 2017 •

edited

Loading

BE-Webdesign May 2, 2017 •

edited

Loading

BE-Webdesign May 2, 2017 •

edited

Loading

BE-Webdesign May 3, 2017 •

edited

Loading

BE-Webdesign commented May 3, 2017 •

edited

Loading

youknowriad May 3, 2017 •

edited

Loading

BE-Webdesign commented May 3, 2017 •

edited

Loading

afercia commented May 3, 2017 •

edited

Loading

BE-Webdesign commented May 5, 2017 •

edited

Loading

BE-Webdesign commented May 5, 2017 •

edited

Loading

BE-Webdesign commented May 5, 2017 •

edited

Loading

BE-Webdesign May 8, 2017 •

edited

Loading

BE-Webdesign May 8, 2017 •

edited

Loading