Implement keyboard and focus management #3897

barklund · 2019-12-07T00:27:05Z

Summary

This implements proper semantic and accessible keyboard and focus management.

This is work in progress. Please see individual commit messages for details.

Tabs
Global shortcuts
Local shortcuts
Media list

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

Currently it's only in use for the tabs in the Library. If you focus any tab in the library, pressing tab and shift-tab will move correctly back and forwards between the tabs and pressing enter will switch selection to the focused tab. However, pressing left and right arrows will also navigate to the other tabs (even observing reading direction), but of course not outside the tab list. This should be fully accessible and semantic..

barklund · 2019-12-07T00:38:23Z

Current early implementation only works for tabs:

Note that you can use the mouse to navigate and focus, you can use tab and shift+tab to focus and then enter to select, but you can also use the arrow keys, left and right (observing reading direction).

Using tab allows you to of course tab to other sections of the application, whereas using arrow keys only let you navigate between the tabs and not any further.

Page carousel now also uses navigable and navigablegroup - and you can press delete to delete the currently focused page.

assets/src/edit-story/app/story/actions/useDeletePageByIndex.js

assets/src/edit-story/app/story/storyProvider.js

dvoytenko · 2019-12-10T01:42:35Z

assets/src/edit-story/components/canvas/carrousel/index.js


 	&:hover {
 		background-color: ${ ( { theme } ) => theme.colors.fg.v1 };
 	}
+
+	&:focus, &:active {
+		outline: none;


General believe, i think, is that outlines are good for accessibility. Is there any reason you don't like it here? I think replacing it with border is fine, but sometimes there are other factors. For instance, if UX asks to add an extra pixel to the outline, very little needs to change, vs making border thicker could cause relayout. But other than that, this is not critical.

I'm am open to using either outline or border, no preference. However, the UX called for a specific design with a specific border and I wasn't able to replicate it using outline only. For a11y purposes I think we're safe as long as we do something else to provide highlight.

Can you point me to the precise place in figma with that style. I just want to confirm it once so we don't have to stumble on this each time.

It's shown in the section about deleting a page from the carousel:

Ok. We'd just need to always reserve a border and only manipulate the border-color from transparent to focused color.

dvoytenko · 2019-12-10T01:51:33Z

assets/src/edit-story/components/focusable/focusable.js

+	const setRef = useCombinedRefs( ref, forwardedRef );
+
+	useLayoutEffect( () => {
+		const mt = MouseTrap( ref.current );


Just to clarify: this shouldn't be new MouseTrap, should it?

Mousetrap doesn't use new in their docs, but I do see that WP does in their implementation, that I was inspired by.

I feel that I remember seeing something about new syntax being discouraged, but I can't find it again.

The docs say:

You can also create a new Mousetrap instance if you want to bind a bunch of things to the same element.

Hm, I don't really know what the difference is here (that is, the actual effect of doing either). The current implementation works, but maybe not efficiently?

dvoytenko · 2019-12-10T01:58:37Z

assets/src/edit-story/components/focusable/focusable.js

+
+function Focusable( { shortcuts, hasFocus, isGlobal, element, className, role, tabindex, forwardedRef, children, ...rest } ) {
+	const ref = useRef();
+	const setRef = useCombinedRefs( ref, forwardedRef );


Why not just use React.forwardRef?

I should also use forwardRef, yes (which allows me to use ref={} in the parent component), but I still need some way to merge the two refs, when needing the ref both in the component itself and in a parent component. It can't be done any other way, unfortunately.

A single forwarded ref usually does the trick for me, but sure, if there's need, let's keep.

dvoytenko · 2019-12-10T02:05:36Z

assets/src/edit-story/components/focusable/navigableGroup.js

+		const focus = ( node ) => node && node.focus();
+
+		if ( direction === NavigableGroup.DIRECTION_HORIZONTAL ) {
+			const { forward, backward } = getDirectionalKeysForNode( ref.current );


This makes sense, but to clarify: what does Tab do inside the NavigableGroup?

Pressing tab navigates between focusable elements using browser-native implementation. Just like enter clicks the currently focused element - this is also using browser-native behaviour. I believe that's excellent for a11y.

Ok. I think that's good. In some systems tab jumps between bigger blocks and arrows are used to navigate within the block. But I think for now a simple way is better. This might pop up for e.g. gallery, where there might be unlimited set of things to tab over, which could make tab harder to use to navigate to other elements.

Looking at the tablist implementation at https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html and the one I built for WordPress core (click on share icon on https://wordpress.org/news/2019/12/wordpress-5-3-1-security-and-maintenance-release/embed/), pressing tab jumps to the current tab's content, not to the next tab.

Looking at the tablist implementation at https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html and the one I built for WordPress core (click on share icon on https://wordpress.org/news/2019/12/wordpress-5-3-1-security-and-maintenance-release/embed/), pressing tab jumps to the current tab's content, not to the next tab.

Thanks a lot for those links - very helpful!

Those definitely are different ways of implementing this. The inactive tabs don't participate in tab order at all it seems (tabindex="-1") - only the active tab and the tab content. tab and shift+tab then navigate between the active tab and the tab content in the regular way based off document order.

And they have an example of both automatic and manual tab switching. Very interesting approaches - I will read up on those!

dvoytenko

Few comments, but otherwise LG

miina · 2019-12-19T15:40:56Z

assets/src/edit-story/components/library/tabs/index.js

+const Tabs = styled( NavigableGroup ).attrs( {
+	element: 'nav',
+	role: 'tablist',
+	tabindex: -1,


Should the default focus CSS (outline) also be removed here? (Since it can still focus by clicking)

spacedmonkey · 2020-01-16T12:48:20Z

This PR is over a month out of date. So can merge for develop-stories.

barklund added Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). Group: Stories Editing Stories Stories Editor labels Dec 7, 2019

barklund self-assigned this Dec 7, 2019

googlebot added the cla: yes Signed the Google CLA label Dec 7, 2019

barklund added 2 commits December 9, 2019 16:46

Add navigable functionality to page carousel

7302396

Page carousel now also uses navigable and navigablegroup - and you can press delete to delete the currently focused page.

Merge branch 'develop-stories' into feature/focusable

17a9f95

dvoytenko suggested changes Dec 10, 2019

View reviewed changes

dvoytenko approved these changes Dec 12, 2019

View reviewed changes

barklund requested review from miina, swissspidy, spacedmonkey and dvoytenko December 12, 2019 21:30

miina reviewed Dec 19, 2019

View reviewed changes

This was referenced Jan 7, 2020

Keyboard handling #3826

Closed

Page reordering from carousel #4048

Merged

barklund mentioned this pull request Jan 23, 2020

Allow reordering in page carousel GoogleForCreators/web-stories-wp#59

Closed

spacedmonkey removed their request for review February 13, 2020 12:40

swissspidy closed this Feb 18, 2020

swissspidy deleted the feature/focusable branch February 18, 2020 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement keyboard and focus management #3897

Implement keyboard and focus management #3897

barklund commented Dec 7, 2019

barklund commented Dec 7, 2019 •

edited

Loading

dvoytenko Dec 10, 2019

barklund Dec 11, 2019 •

edited

Loading

dvoytenko Dec 11, 2019

barklund Dec 11, 2019

dvoytenko Dec 12, 2019

dvoytenko Dec 10, 2019

barklund Dec 11, 2019

swissspidy Dec 17, 2019

barklund Dec 17, 2019

dvoytenko Dec 10, 2019

barklund Dec 11, 2019

dvoytenko Dec 12, 2019

dvoytenko Dec 10, 2019

barklund Dec 11, 2019

dvoytenko Dec 11, 2019

swissspidy Dec 17, 2019

barklund Dec 17, 2019

dvoytenko left a comment

miina Dec 19, 2019

spacedmonkey commented Jan 16, 2020

Implement keyboard and focus management #3897

Implement keyboard and focus management #3897

Conversation

barklund commented Dec 7, 2019

Summary

Checklist

barklund commented Dec 7, 2019 • edited Loading

Choose a reason for hiding this comment

barklund Dec 11, 2019 • 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

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacedmonkey commented Jan 16, 2020

barklund commented Dec 7, 2019 •

edited

Loading

barklund Dec 11, 2019 •

edited

Loading