Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard accessibility: Add a "Skip link" to jump back from the inspector to the selected block #5856

Merged
merged 6 commits into from
Apr 11, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Mar 28, 2018

This PR adds a way for keyboard and assistive technologies users to jump back to the currently selected block.

Users can already use keyboard shortcuts to navigate the editor regions, see #3084 and screen reader users can use ARIA landmarks, see #2380. These tools can be used to jump from the block being edited to the Sidebar but there's no way to jump back from the sidebar to the block being edited.

This PR adds a reusable component that mimics the "skip links" already used in core. Actually, it's a button that gets revealed just when it receives keyboard focus and, when pressed, moves focus back to the currently selected block.

Related:

For now, it's placed in the block inspector in the sidebar (in the bottom right corner of the screen) but it could be useful also in the "post settings". I could see it also reused in the header toolbar and in the publishing flow, so I guess the best option would be to consider it as a generic component. In this regard, I'm not sure where the new files should be placed. For now they're in the editor directory. Any feedback welcome.

Screenshot:

screen shot 2018-03-28 at 21 31 03

Design-wise, I've made it look like the current "skip links" in core, any feedback welcome.

When a better, customizable, shortcut for navigateRegions will be available, see:
Consider a mechanism to customize shortcuts, e.g. Ctrl + backtick #3218
there will be a way for all keyboard users to jump from the selected block to the sidebar and back from the sidebar to the selected block, thus fixing #1182

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Mar 28, 2018
@mtias mtias added the Needs Design Feedback Needs general design feedback. label Apr 2, 2018
@afercia afercia changed the title [WIP] Keyboard accessibility: Add a "Skip link" to jump back from the inspector to the selected block Keyboard accessibility: Add a "Skip link" to jump back from the inspector to the selected block Apr 3, 2018
@afercia afercia requested review from aduth and karmatosed April 3, 2018 16:38
@karmatosed
Copy link
Member

Design-wise, I've made it look like the current "skip links" in core, any feedback welcome.

Whilst I would like to change those, I think keeping it the same is sensible. Changing to me seems like we'd cause friction where none is needed.

Position wise it seems to make sense tabbing through. Is it usual to have such skips in different places on screens? It totally may be, which is why asking for insights.

Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design wise this is just like core so giving it approval for now.

@@ -0,0 +1,27 @@
.skip-to-selected-block {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class name does not adhere to the Gutenberg naming conventions:

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#naming

It should be corrected to .editor-skip-to-selected-block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I've used editor- initially then removed waiting for a feedback about the placement of the whole thing,

}
}

.edit-post-sidebar .skip-to-selected-block:focus {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit-post is a superset of (extends) the underlying editor, so there should not be awareness of that context from here. These styles, if needed, should be applied within edit-post/components/sidebar/style.scss (also better reflecting that they're specific to the edit-post sidebar, for someone looking for those styles).

@@ -16,6 +16,10 @@ export function getBlockDOMNode( uid ) {
return document.querySelector( '[data-block="' + uid + '"]' );
}

export function getBlockWrapperDOMNode( uid ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a JSDoc block here describing the intended behavior, arguments, and return value? That we describe this as expected to return an element with tabIndex might reveal that it could potentially benefit from a more appropriate name: getBlockTabbableWrapper ? I don't feel strongly; proposed name is fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, forgot to do that.

@@ -0,0 +1,37 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: If "Skip To" buttons are expected to be commonplace, could make for a good base components component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean moving it to the /components directory?

Copy link
Member

@aduth aduth Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, maybe not SkipToSelectedBlock as-is, since it an implementation specific to block usage, but a SkipTo, which SkipToSelectedBlock can extend (or just inline SkipTo where we're using SkipToSelectedBlock).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK. Hm at the moment I don't see the need for other "Skip To"s, the ones in core and this one should suffice, for now. We can always iterate later, if need be?


export default withSelect( ( select ) => {
return {
selectedBlock: select( 'core/editor' ).getSelectedBlock(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSelectedBlock can be a computationally-intensive selector. I wish we had a simpler way to get access to the direct UID value of the singular selected block. We do have getBlockSelectionStart, but this also returns a value for a multi-selection. Which actually raises another point: do we care if it's a singular block selection? Seems like we'd want to return to the first UID of the set of selected blocks in either case.

@@ -55,6 +55,8 @@ $z-layers: (
'.components-popover.is-bottom': 99990,

'.components-autocomplete__results': 1000000,

'.skip-to-selected-block': 100000,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why so high? In fact, even without z-index at all, it appears to layer as I'd expect.

FWIW, this isn't the same as putting the button on equal footing as .edit-post-sidebar, since it's already within the stacking context of the sidebar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For re-use in other places e.g. header toolbar, publishing flow, etc. I guess it should have a high z-index.

};

return (
selectedBlock && uid &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectedBlock will never be falsey here. You already have a condition above for ! selectedBlock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectedBlock is falsey initially and switching (even with mouse) the sidebar to the block inspector renders the skip link (the render method must return "something")

@afercia
Copy link
Contributor Author

afercia commented Apr 3, 2018

Is it usual to have such skips in different places on screens? It totally may be, which is why asking for insights.

I don't have an answer, I think this is the first time I see a "skip link" to actually jump back :) Will ask the a11y team opinion.

@rianrietveld
Copy link
Member

What's in a word: skip link / jump back. It's all relative to how some sees/uses the page I guess.
Keeping the wording uniform may be the clearest for users.
So "Skip to selected block" seems ok to me. Nice work Andrea :-)

@afercia
Copy link
Contributor Author

afercia commented Apr 4, 2018

I think I've addressed most of the points, pending feedback on #5856 (comment)

For now, it's placed only in the block inspector. I've tested it also with multi-selection and thanks to getBlockSelectionStart() it would work fine. Worth noting the actual returned UID is the one of the block that gets selected first. For example, when multi-selecting starting from the last block and moving the mouse up, the UID is the one of the last block:

screen shot 2018-04-04 at 17 35 30

@afercia afercia removed the Needs Design Feedback Needs general design feedback. label Apr 4, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@afercia afercia force-pushed the add/skip-to-selected-block branch from eb20646 to 978b412 Compare April 11, 2018 09:05
@afercia
Copy link
Contributor Author

afercia commented Apr 11, 2018

Rebased to solve conflict.

@afercia afercia added this to the 2.7 milestone Apr 11, 2018
@afercia afercia merged commit b83d659 into master Apr 11, 2018
@afercia afercia deleted the add/skip-to-selected-block branch April 11, 2018 09:27
@mtias
Copy link
Member

mtias commented Apr 18, 2018

Nice one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants