Table of contents block accessibility improvements #54322
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexstine
added
[Focus] Accessibility (a11y)
alexstine
requested review from
ZebulanStanphill,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
alexstine
commented
Sep 8, 2023
| @@ -137,8 +137,11 @@ export default function TableOfContentsEdit( { | |||
| return ( | |||
| <> | |||
| <nav { ...blockProps }> | |||
| <ol inert="true"> | |||
There was a problem hiding this comment.
I'm not sure why no one caught this regression. This makes the content completely inaccessible to keyboard users. Please don't do this unless you have a really good reason to.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/inert
There was a problem hiding this comment.
inert was added in several places in d50e613; we maybe should take a look at each usage.
| <ol> | ||
| <TableOfContentsList | ||
| nestedHeadingList={ headingTree } | ||
| disableLinkActivation={ true } |
There was a problem hiding this comment.
Passing this prop to disable link activation for the editor side.
| }: { | ||
| nestedHeadingList: NestedHeadingData[]; | ||
| disableLinkActivation?: boolean; |
There was a problem hiding this comment.
Is there a better way to type this?
| <a | ||
| className={ ENTRY_CLASS_NAME } | ||
| href={ link } | ||
| aria-disabled={ disableLinkActivation || undefined } |
There was a problem hiding this comment.
This communicates disabled state to screen readers.
| className={ ENTRY_CLASS_NAME } | ||
| href={ link } | ||
| aria-disabled={ disableLinkActivation || undefined } | ||
| onClick={ |
There was a problem hiding this comment.
This prevents click.
| ? ( event ) => event?.preventDefault() | ||
| : undefined | ||
| } | ||
| onContextMenu={ |
There was a problem hiding this comment.
This prevents context menu open in new tab/window.
|
Size Change: +1.64 kB (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
2 tasks
|
The |
|
Note: The Latest Post block has a similar implementation for preventing opening links in the editor (ref: #40593), but it uses Snackbars instead. Which method would be preferred for screen reader users - snackbars or description like we're adding here? I think using one method across the blocks would set better expectations, and we could also recommend it in the future. |
|
@Mamaduka I actually like the snackbar approach because it is less verbose. Maybe this would be a good hook to create? It also makes it more clear visually with the visual notice. Something like |
|
@alexstine, starting with code duplication is fine for now and usually works better than early abstractions. After we have a common pattern, I'm happy to team up and extract it into a reusable hook. Maybe something like this: function Example() {
// `useInertLinkProps` or `useDisabledLinkProps`.
const inertLinkProps = useInertLinkProps();
return <a className="example" { ...inertLinkProps }>Example</a>;
} |
|
Sounds good. I'll work on implementing this code later today. Its already after midnight. 😞 |
|
@Mamaduka Finally got time to implement this. Should be good for another review. |
Mamaduka
reviewed
Sep 19, 2023
Mamaduka
reviewed
Sep 19, 2023
There was a problem hiding this comment.
@alexstine, the new notice fails for the nested headings in ToC. The TableOfContentsList calls itself for rendering nested lists, so you'll need to pass extra props there as well.
Try testing the ToC block with the following heading structure. Clicking on "A sub heading" currently will trigger navigation.
Block code snippet:
<!-- wp:heading -->
<h2 class="wp-block-heading" id="heading-text">Heading text</h2>
<!-- /wp:heading -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading" id="a-sub-heading">A sub heading</h3>
<!-- /wp:heading -->
|
Fixed @Mamaduka. Totally missed the last call. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR fixes some accessibility issues in the table of contents block, front-end and back-end.
Why?
Accessibility is important.
How?
aria-labelto front-end<nav>tag.Testing Instructions
aria-disabled="true"attribute.aria-label="Table of Contents"attribute.Testing Instructions for Keyboard
Verify in the back-end that Up and Down Arrow keys allow you to navigate the block content.
Screenshots or screencast