Improve the placeholder instructions accessibility. #45801
Conversation
|
|
|
Size Change: -44 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
@afercia Thanks for the PR but sadly this does not work well for Windows screen readers. I gave this a quick test in Firefox.
- Enable NVDA.
- Add a new post in Firefox.
- Insert an image block.
- Focus is placed on upload button which I believe is why the message is not being read.
role="status"usesaria-live="polite"and because theuseFocusFirstElement()hook fires, it cancels out the polite speech.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/status_role
Any other ideas?
|
@alexstine thanks for testing! Sad it doesn't work for Windows sr. In the latest commit I took a different approach. The instructions text is now passed to
I'd appreciate some quick test. Note: I'm not sure there's a way to avoid the instructions to be read again when transforming a block. For example, when transforming an Image block to a Columns block (which actually wraps the Image within a Column block), it appears the Image block re-mounts and |
| if ( instructions ) { | ||
| debouncedSpeak( instructions ); | ||
| } | ||
| }, [ instructions, debouncedSpeak ] ); |
There was a problem hiding this comment.
Should this run every time the block is selected? As of now, the instructions are only provided on insert.
| }, [ instructions, debouncedSpeak ] ); | |
| }, [ debouncedSpeak ] ); |
There was a problem hiding this comment.
Well, the pre-commit linter forced me to add both props as dependencies. I would have gone with an empty array otherwise.
There was a problem hiding this comment.
With an empty dependency array, if the instructions were to be updated, they would not be announced again — in that sense, having both dependencies looks correct to me?
Regarding debouncedSpeak, the useDebounce hook internally uses useMemo, and therefore the debounceSpeak dependency should not really change unless the speak function changes
There was a problem hiding this comment.
instructions doesn't change as well, at least there are no cases or dynamic instructions so far. So, regardless f whether we use an empty array or not, the announcement is not repeated. Anyways, the pre-commit linter check requires to add both dependencies :) can't commit without adding them.
There was a problem hiding this comment.
@ciampo Is there anything we can use in place of useDebounce() to still have the timeout for the speak() but can announce on every render? I guess we could attach onFocus callback and trigger the announcement that way?
There was a problem hiding this comment.
It sounds like the first step here is to understand the desired behavior — should the announcement be heard or every render? Only when the block is added? Every time the block is focused? Every time the block is focused?
Once there is an agreement there, we can think about what's the best way to implement the desired behaviour
There was a problem hiding this comment.
Ideally we should hear it when the block is focused, because that way when adding placeholders, saving, then going back some time later to add the actual content, the user will know how to interact with the block.
Also worth considering the use case of interacting with an existing template where all the blocks are already in place; the person that fills in the content might not even be the same as the one who adds the blocks.
If the problem with the fieldset/legend combo is that not all placeholders have controls, I wonder if we could come up with a different markup structure for those, and some sort of flag to let the component know when to use either? This has probably been suggested already but nothing else occurs to me 😅
There was a problem hiding this comment.
@tellthemachines I think what we really need is to tie the Placeholder and block selection together. That way the announcement is triggered on block select. I am just not sure how to do this with different components at play. Does the Placeholder render if the block is not selected?
There was a problem hiding this comment.
Placeholder renders whenever a new block is inserted or, after save and reload, if the block hasn't been changed since insertion. It'll render whether the block is focused or not, because it's always meant to be visually present on the page.
There was a problem hiding this comment.
@tellthemachines Think it is possible to add onFocus callback to the wrapping div and just call the speak() function when focus is actually placed inside the block content?
skorasaurus
added
the
[Focus] Accessibility (a11y)
It seems to be that it's normal that the instructions are announced again when you transform blocks, instructions could be different between blocks. |
Yep, but in the example I made above the Image block isn't actually 'transformed'. It gets wrapped within a Columns block. The Image block instructions will be announced again, even though the actual selected block is the Columns one. It's an edge case and we can maybe live with it, I was just wondering if there's a way to avoid the new announcement. |
|
@afercia I don't think there's if we tie the announcement with the component. Even if we tie it to the block instead it's very hard to do as sometimes the "clientId" of the block changes which means there's no way for us to know that it's the same block in all situations. |
@alexstine I'm not sure the Placeholder and instructions are rendered again when editing a block. For example, if you actually set an image on an Image block, save, and then edit again the block, there are no instructions. Even the current fieldset + legend aren't rendered. To my understanding, there's only one case when the instructions are available again and that's when a block is left 'empty' e.g. an Image block with no image. If we opt for making the instructions announced again when editing a block that's totally reasonable but I'm not sure it can rely on the Placeholder component. Maybe I'm missing something: is there any case where editing a block shows the Placeholder with instructions again? |
|
@afercia That is actually a valid point. I am not sure there is a block with content with a placeholder. It would be nice to get confirmation on this before merging though. @getdave @tellthemachines @talldan Any ideas? |
There's none that I know of. Would we still want to announce instructions for blocks that already have content? Might that not be more confusing? |
|
@tellthemachines I was under the impression that |
|
I'm not completely sure I understand the question. The media and text block can have its inner blocks edited while it's still showing a placeholder, but I don't think it counts because I think you're referring to a block that still renders the placeholder after it has been used to add content. In all cases that I'm aware of, the placeholder is removed from the DOM after it's used. |
|
@talldan Yes. There are two issues being discussed.
Thoughts? |
That's a difficult problem to solve, but only announcing on focus seems like a good idea. Not sure what other options there are. It's a shame there isn't a wrapper element that could have an aria-description, but I suppose that would need to be focusable. 🤔 |
|
I'll try to improve this a little more when I catch a free moment. If I get something reasonable, I'll do a single commit so we can all evaluate and revert if needed. 👍 |
|
@afercia Done. I think it works better now. I am using state to detect the first time the component gets focus instead of based off timing alone. Thoughts? |
|
Flaky tests detected in 37bda2d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6253419745
|
| within( placeholder ).getByRole( 'group' ); | ||
| // Test for empty content. When the content is empty, | ||
| // the only way to query the div is with `querySelector` | ||
| // eslint-disable-next-line testing-library/no-node-access |
There was a problem hiding this comment.
Is there a better way or is it okay to ignore?
There was a problem hiding this comment.
Mhh, there isn't an obvious better choice. Maybe @tyxla can think of something here?
There was a problem hiding this comment.
Thanks for the ping @ciampo!
Nope, if we remove the fieldset in favor of a div (which I agree it makes sense), there's no other way to query for the empty div. There are other DOM traversing alternatives, but this one is the most precise.
Therefore, since I agree with removing the fieldset in favor of a div in this use case, I'm fine with the ignore. Making an element inaccessible intentionally is the ideal case for this type of ESLint ignore.
|
@alexstine thanks for the ping. Good idea using the state, it works better now! There's still a problem though. Block with a Placeholder that doesn't contain focusable element won't trigger the
|
|
@ciampo Are these failures related? Looks like maybe not. These E2E logs are too hard for me to read... |
|
I don't think they are related — in any case, I've pushed a couple of commits to solve a merge conflict, which will cause the CI checks to re-run. Let's see if they fail again. |
ciampo
added
Backport to Gutenberg RC
mikachan
removed
the
Backport to WP 6.7 Beta/RC
* Improve the placeholder instructions accessibility. * Use a speak message for the placeholder instructions. * Update test. * Try to only announce message after initial focus. * Fix unit tests. * Announce message on render and add test. * Changelog entry. * Reviewer feedback. Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> * Fix unit tests. * Fix changelog. * Remove duplicate CHANGELOG entry --------- Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com> Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: af78d3f |
mikachan
removed
the
Backport to Gutenberg RC
|
👋 There's a report from the testing for WP 6.4 Beta 1 that may be related to this change:
There's also an error log included: I'm wondering if anyone could help debug this. Thank you 🙇 |
|
Hey @mikachan , reading the stack trace, it looks like the error is coming from this call to the Since there is an Are you able to investigate further on with the person who filed the report? Update: I quickly tried to render a
And I got the exact same message. If that was the case, it means that whoever got this error was using the component incorrectly, since |
|
@ciampo I wonder if we should type check this anyway in the wordpress/a11y package? Make sure we're dealing with type of string? If so, happy to make a quick follow-up. |
|
Hi @ciampo, I'm the guy who reported this in Core Trac. |
|
@TobiasBg Its the |
@alexstine we could, and that would be an extra safety net. At the same time, we do add docs and TypeScript types exactly to help folks to use our components. If we had to always check for every prop to have the correct type at runtime, then we'd need to rewrite most of our components.
Hey @TobiasBg , as @alexstine also mentioned, the |
|
Thanks for the details and explanation, @ciampo! That makes perfect sense! |
The same issue happened to me too. I want to put HTML as instructions (it just has a bulleted list). HTML tags are rendered as-is in the instructions though. I used to use wp.element.createElement(wp.components.Placeholder, {
icon: gridIcon,
label: 'Justified Image Grid',
instructions: wp.element.RawHTML({
children: txt.placeholderInstructions,
}),
});From now on, I either skip the Placeholder component entirely and only output raw HTML, or put everything in the preview prop instead of of |
|
Hey @Firsh ,
HTML is not supported by the The |
| if ( instructions ) { | ||
| speak( instructions ); | ||
| } | ||
| }, [ instructions ] ); |
There was a problem hiding this comment.
On page load, all block with placeholders with instructions are now all "speaking" at once. Is this by intention?
There was a problem hiding this comment.
@ellatrix No, it needs to be fixed.
Fixes #42738
What?
#38366 made sure the placeholder instructions are announced by screen readers. It did so by using a
<fieldset>element with a<legend>element that holds the instructions text.However, that appears to be not ideal for the reasons explained in #42738. I'd like to propose a cleaner approach.
Why?
How?
The only goal of the fieldset and legend added in 38366 was to make the instructions announced by screen readers. It's safe to remove them and provide another way to make screen readers announce the instructions. What this PR does:
role=statusto the instructions so that they're announced just once, when inserting the block.Note: a
role=statusestablishes a live region, that is a region whose content is supposed to be updated. Strictly speaking the usage otrole=statusin this case is slightly unsemantic because the instructions text doesn't update.Testing Instructions
Without a screen reader:
role="status"attribute.Screenshots or screencast