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

Editable blocks have inconsistent ARIA role: sometimes textbox, sometimes document #42158

Open
afercia opened this issue Jul 5, 2022 · 20 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 5, 2022

Description

Blocks that have an editable area use the RichText component that renders an HTML element with a contenteditable HTML attribute. This provide users with an area where they can enter text.

Semantically, these editable areas need to be exposed to assistive technology as editable 'fields'.

In terms of ARIA roles and attributes:

  • role="textbox" is the equivalent of a HTML text input field.
  • role="textbox" aria-multiline="true" is the equivalent of a HTML textarea and should be used for most of these editable areas.

The RichText component defaults to role="textbox" and that's correct.

However, with the introduction of useBlockWrapperProps in #23034 (later renamed to useBlockProps) things have changed and now editable areas have inconsistent ARIA roles.

Note that, initially, useBlockWrapperProps used to pass an ARIA role="group" even though I think the group role was added even before in #19701. That was incorrect and was fixed in #33627 by changing the group role to document. However, seems to me that fixed the symptom rather than the root cause. Editable areas should always have a textbox semantics.

Seems to me the root cause is that useBlockProps is used inconsistently. Sometimes, it's used on the RichText wrapper. Sometimes, it's used directly on the RichText component itself. In this last case, the role="document" provided by useBlockProps overrides the RichText role textbox.

I read along the discussion on #29526 and #33627 and I see the fix provided there was discussed and tested by the accessibility team. I'd like to hear from the team and have some discussion but I'd tend to think all the editable areas should have a consistent ARIA role of textbox.

Step-by-step reproduction instructions

  • Create a new post and add the block listed below.
  • Inspect the source in your browser's dev tools and observe the ARIA role applied to the editable element: the one with the contenteditable attribute:
  • Paragraph: has a role="document"
  • Heading: has a role="document"
  • Preformatted: has a role="document"
  • Quote: the quote field has a role="textbox"
  • Quote: the citation field has a role="textbox"
  • Pullquote: the quote field has a role="textbox"
  • Pullquote: the citation field has a role="textbox"
  • Table: each cell has a role="textbox"

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor labels Jul 5, 2022
@afercia
Copy link
Contributor Author

afercia commented Jul 5, 2022

From a technical side: I'm not sure why useBlockProps provides a default prop role: 'document' to start with.

As noted earlier, seems to me useBlockProps is used in a couple ways:

  • When used on the RichText wrapper, it ends up adding an ARIA role="document to the wrapper, which doesn't make much sense. For example, in the Table block, it adds role="document to the <figure> element that wraps the table.
  • When used directly on the RichText component, for example in the Paragraph block, it ends up overriding the RichText default role="textbox", which is incorrect.

What is the actual purpose of the role: 'document' prop in useBlockProps?

@afercia
Copy link
Contributor Author

afercia commented Jul 6, 2022

I forgot to ping some members of the accessibility team 🙂 @alexstine @joedolson for some insights on previous testing and discussion. Thanks!

@alexstine
Copy link
Contributor

This is a tricky one because I believe role document allows us to place focus on the block wrapper and have screen readers announce it. I tried removing the role once and it produced terrifying results. In the table block for example, it read the figure tag instead of the aria-label that adds context. There is a lot wrong with how we currently do things in Gutenberg and I hope it gets better with time, but I just do not have the bandwidth to take on much at the moment. If you want to open a quick test PR and change the default role in useBlockProps hook, we can see what happens.

@afercia
Copy link
Contributor Author

afercia commented Jul 7, 2022

Thanks @alexstine
A quick way to test this is by testing a Quote block.
In the Quote block there are two editable fields rendered by the RichText component: the Quote text and the Citation text. Both have a role=textbox.
The wrapper is a <blockquote> element with role=document and aria-label Block: Quote. I'm not sure this is entirely correct as it overrides the native blockquote role. Also, not sure a document can be labelled.
Regardless, the two editable RichText are exposed as textbox, which is the point of this issue. Are they announced with the correct semantics with Windows screen readers?

@alexstine
Copy link
Contributor

@afercia

Regardless, the two editable RichText are exposed as textbox, which is the point of this issue. Are they announced with the correct semantics with Windows screen readers?

Doubt it but cannot say for sure. I can test this weekend if you need but I never really made it as far as testing specific blocks. My main focus so far has been trying to improve the blockers around the editor as a whole. E.g. functionality that effects all blocks and how you interact with the content.

If you think about it though, I am not sure screen readers would be better off announcing blockquote. This seems like it could make the situation worse. It will output a blockquote, but in theory, it is not a blockquote yet. That is how I think about it though.

Thanks.

@afercia
Copy link
Contributor Author

afercia commented Jul 8, 2022

If you think about it though, I am not sure screen readers would be better off announcing blockquote.

@alexstine agree. I think we're mixing up a bit different topics. Sorry if I wasn't clear. The block wrapper and the block editable area are (or better: should be) two different things.

The RichText component renders the actual block editable area. Basically, it's the equivalent of a HTML <input> or <textarea>. As such, it should use correct semantics and labelling. Since it's editable, it should always have a role=textbox. The actual announcement varies depending on the screen reader. VoiceOver announces it as edit text. It should also be properly labelled, to clarify what users are editing.

The block wrapper is a different thing. I explored a bit previous versions of the editor and I see the block DOM structure has changed a lot over time. I think many of these changes originate from the work done to 'lighten' the blocks but the situation now is pretty confused and the HTML of many blocks is inconsistent.

Sometimes block do have a wrapper element. Sometimes don't. To my understanding, in previous versions of the editor, all blocks used to have a wrapper element. This has a impact when it comes to the way ARIA roles and attributes are applied to the blocks markup.

A quick example based on the Heading block:

In WordPress 6.0, the Heading block doesn't have a wrapper element:

<h2 role="document" aria-multiline="true" aria-label="Block: Heading" tabindex="0" contenteditable="true" ...

I don't mind that much about the H2 semantics being overridden. What's wrong here is:

  • It's an editable field but it has a role="document". It should be role="textbox".
  • The labelling in the form of Block colon block-type is the one that used to be set on the block wrapper. Instead, it's now set on the editable field.
  • The aria-multiline="true" is set on a role="document", which is incorrect.
  • Minor: tabindex="0" is redundant because contenteditable already makes an element focusable.

In WordPress 5.3, for example, there was a a wrapper element:

<div tabindex="0" aria-label="Block: Heading" ...
    <h2 role="textbox" aria-multiline="true" contenteditable="true" aria-label="Write heading…" ...
  • The label with the block type was set on the wrapper.
  • The actual editable field had a role="textbox".
  • The editable field labelling was 'Write heading…'

Overall, the current blocks markup is largely inconsistent and I think it does have an impact on assistive technology. To my understanding there are two main issues here:

  • Some blocks do have a wrapper element, some don't.
  • Sometimes useBlockProps is used on the block wrapper, sometimes directly on the RichText component, which impacts the way ARIA roles and attributes are applied to the blocks.

This inconsistency is less than ideal from an accessibility perspective.

@alexstine
Copy link
Contributor

@afercia Oh yes, I understand now. I am not sure why not all blocks have wrappers or why some blocks try to have wrappers/content in one. It makes little sense as an insider looking in, you are correct. @talldan @tellthemachines Do you have any idea why this was changed? Cleaning up HTML would be my best guess?

@afercia Have you tried overriding the prop for a single block or even in RichText? Something like this?

const blockProps = useBlockProps( { role: 'textbox' } );

I am fairly positive you can override that property and if not, you can change it for testing at block-editor/src/components/block-list/use-block-props/index.js I believe. I have done a fair amount of work here and other related called hooks.

Would be interesting to see what happens with correct role usage. In theory, screen readers should still switch in to edit mode so I would be happy with that.

@talldan
Copy link
Contributor

talldan commented Jul 11, 2022

@talldan @tellthemachines Do you have any idea why this was changed? Cleaning up HTML would be my best guess?

Pretty much. There has been an effort to make the block HTML in the (backend) editor match the block HTML on the frontend of a site. On the frontend, a paragraph block will only output a paragraph element, so the editor now does the same. It makes it possible for a block in the editor to more closely match the frontend styling.

I can't recall exactly how it worked before in the editor, possibly there were lots of div wrappers.

I think this is the PR that changed things - #23034.

@afercia
Copy link
Contributor Author

afercia commented Jul 11, 2022

@alexstine thanks for the hint. Not sure overriding the prop would be the best option as it would require to fix the role on a case by case basis, only when useBlockProps is used directly on RichText.

@talldan thanks for looking into this.

possibly there were lots of div wrappers.

Yep, some block still use div elements or other elements as wrappers. I'm not sure that's the root cause though. It's just that useBlockProps is meant to provide a set of default props to make an element a 'block'. These props include the role=document, which overrides the RichText role prop. Previously, RichText was just using its default role prop.

Update:
I noticed the problem of the overridden props is not limited to the role. Sometimes, useBlockProps overrides also the RichText aria-label. For example. in the Heading block, the aria-label is:

aria-label="Block: Heading"

It should be:

aria-label="Heading text"

The first label is the one that used to be set on the block wrapper. It now overrides the label of the editable RichText.

This has an impact on the way the various block are exposed to assistive technology. Actually, the labelling is pretty inconsistent.

@vcanales
Copy link
Member

Currently, the RichText component considers role an overridable prop.

I'm wondering if there's a need for this to be so; from an accessibility perspective, is there a valid reason for role to be anything other than textbox in the RichText component? Making it permanently textbox is a — perhaps too naive — way to fix consistency on blocks that rely on this component.

@vcanales
Copy link
Member

Furthermore, aria-multiline=true should only be used if role='textbox', but the current logic doesn't make sure that this doesn't happen. This error is caught by axe devTools.

@afercia
Copy link
Contributor Author

afercia commented Feb 14, 2023

To recap the current situation and further clarify my comment on #47276:

  • The RichText component has a default role="textbox" prop that can be overridden.
  • Long time ago, all the blocks used to have a block 'wrapper' element. This element had a role="group", which made sense at that time as it was a sort of 'fieldset' wrapping editable fields.
  • With the introduction of useBlockProps, thw block wrapper was removed for most of the blocks. Not for all blocks. At the same time, the role="group" prop was moved to useBlockProps.
  • After Unable to edit blocks once they've been inserted into content while using a screen reader #29526 and Fix some JAWS bugs #33627, the role="group" prop was changed to role="document" to solve some issues with old JAWS versions. As such, useBlockProps has a default role="document" prop.
  • Some blocks still have a wrapper element (e.g. Pullquote, etc.), some don't (e.g. Paragraph, Heading, etc.).
  • Sometimes, the props from useBlockProps are passed directly to the RichText component, for example in the Paragraph block. As such, the contenteditable element of the Paragraph block has a role="document"
  • Sometimes, the props from useBlockProps are passed to the block wrapper instead, for example in the Pullquote block. As such, the wrapper has a role="document' while the contenteditable element has the default role="textbox".

Basically, the presence of the wrapper element and the usage of useBlockProps are greatly inconsistent across blocks. This inconsistency has a deep impact on assistive technologies and affects also other aria attributes and labelling.

@afercia
Copy link
Contributor Author

afercia commented Feb 14, 2023

A good way to test the inconsistencies across blocks would be comparing the way various screen readers behave on the Paragraph block and on the Quote (or Pullquote) block.

To me, a contenteditable element should always have a `role="textbox'.

Feedback from assistive technology makers would be very very welcome ❤️ 🙏 to make sure that wouldn't introduce weird behaviors for old versions. See the discussion on #29526
Cc @ggordon-vispero, @michaelDCurran, @seanbudd

@alexstine
Copy link
Contributor

I wanted to revive this discussion as I am currently playing around in a PR where I am looking at removing role="document" all together and letting a blocks implicit ARIA roles take over. I think trying to define a role for every block is too difficult globally so it should be up to developers to ensure their blocks are accessible.

As far as blocks with wrappers and without wrappers, apparently this was done to match the front-end visually and will likely not change so we've got to make the best of it.

@salvatoremark
Copy link

@alexstine, do we know where this issue stands today? I tried overwriting the role="document" attribute where it was suggested:

useBlockProps({ role: "presentation" })

But apparently, that feature hasn't been implemented yet? Setting blocks to default to role="document" might be ok as long was we can overwrite the value in useBlockProps().

@alexstine
Copy link
Contributor

@salvatoremark No movement at this point. BTW, why are you adding the presentation role? That will remove all semantics for assistive tech. I can understand the ability to override the role but not with presentation.

@salvatoremark
Copy link

salvatoremark commented Apr 5, 2024 via email

@joedolson
Copy link
Contributor

Can you provide more detail on how this is decorative, @salvatoremark ? I'd argue that it's not possible for any block to be decorative in the admin; if an editor is working with the document, they always need to be able to identify the block in a meaningful way. It's similar to how images in the media library cannot be decorative, because the purpose of the media library is to display the images in the library.

@salvatoremark
Copy link

salvatoremark commented Apr 7, 2024 via email

@alexstine
Copy link
Contributor

Not that I know of. You might be able to do it with index.php file but I do not think you can in save.js. Would need someone else to confirm.

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). [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants