Use inert attribute instead of useDisabled #44865
Conversation
youknowriad
requested review from
spacedmonkey,
ajitbohra,
ZebulanStanphill and
michalczaplinski
as code owners
youknowriad
added
the
[Type] Bug
|
Size Change: -118 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
youknowriad
requested review from
gziolo,
ntwb,
nerrad and
ryanwelcher
as code owners
|
I'm planning to use the inert attribute to be able to implement the "readonly" view of #44770 without causing a remount of overything. |
|
TIL about Do you know if assistive tech can access text in inert elements? |
|
@Mamaduka No they can't on purpose. |
| /** A variable keeping track of the previous updates in order to restore them. */ | ||
| const updates: Function[] = []; | ||
| const disable = () => { | ||
| node.childNodes.forEach( ( child ) => { |
There was a problem hiding this comment.
This only seems to check the direct descents of a node. Should we recursively for each child node also and the inert attribute?
There was a problem hiding this comment.
No, inert is recursive by default, the only reason we can't just apply inert at the top level is because we don't want to disabled the top level entirely (that's how the hook used to work and we can't change it).
| await waitFor( () => { | ||
| const button = screen.getByText( 'Button' ); | ||
| expect( button.hasAttribute( 'disabled' ) ).toBe( true ); | ||
| } ); |
There was a problem hiding this comment.
Testing that the preview is disabled seemed like a meaningful test — instead of deleting it, we could have replaced it with user actions? (ie. user tries to interact with block preview, nothing happens)
| const input = screen.getByRole( 'textbox' ); | ||
| const contentEditable = screen.getByTitle( 'edit my content' ); | ||
| expect( input ).toBeDisabled(); | ||
| expect( contentEditable ).toHaveAttribute( 'contenteditable', 'false' ); | ||
| expect( contentEditable ).not.toHaveAttribute( 'tabindex' ); | ||
| expect( contentEditable ).not.toHaveAttribute( 'disabled' ); |
There was a problem hiding this comment.
Same here — instead of testing implementation details, we could write tests from the point of view of the user: "trying to interact with an element, nothing happens".
There was a problem hiding this comment.
For me these were testing internal implementation anyway and not good tests so that's why I removed them. That said, I agree that "trying to interact" is a better test here.
There was a problem hiding this comment.
Agreed! The previous tests were definitely testing implementation details too
| expect( contentEditable ).not.toHaveAttribute( 'tabindex' ); | ||
| expect( contentEditable ).not.toHaveAttribute( 'disabled' ); | ||
| // @ts-ignore | ||
| expect( container.firstChild.hasAttribute( 'inert' ) ).toBe( true ); |
There was a problem hiding this comment.
Instead of adding @ts-ignore, this could have been fixed by adding an optional chaining to container.firstChild?.hasAttribute( 'inert' ).
(this line has since been refactored to use the toHaveAttribute assertion)
There was a problem hiding this comment.
@youknowriad Please see my comments below. I fear using inert has caused quite a few accessibility regressions. I am not sure why for example you added it to the table of contents block, this made the block content that sighted users can see totally inaccessible to keyboard users on the back-end. As noted by @Mamaduka, further research probably should have happened here before shipping this PR.
| className="wp-block-comments__legacy-placeholder" | ||
| ref={ disabledRef } | ||
| > | ||
| <div className="wp-block-comments__legacy-placeholder" inert="true"> |
There was a problem hiding this comment.
Why exactly are we disabling this?
There was a problem hiding this comment.
This is just mirrors what existed before the PR. Basically we had a useDisabled hook that poly fills what "inert" does. So I'm not sure we changed anything in behavior here. I believe these "inert" within components are there because these blocks are "interactive", people can interact with them in the frontend but they are not meant to be interacted with in the backend (or navigated within). I'm not entirely sure that we really want to keep this visible for screen reader users, they are just mean to offer a visual representation of what the block will look like in the frontend but for interactions purposes, the intent is to hide these entirely.
If we really want to leave a "trace", we may add a label maybe or something like that?
There was a problem hiding this comment.
@youknowriad That's the problem. We are giving sighted people information keyboard/screen reader users don't have access to. If the content on the back-end is really not important, let's add display: none; to it so it is inaccessible to everyone. Additional context make sense?
Thanks.
There was a problem hiding this comment.
I think I see what you're coming from but the reality is that the editor is also meant to be WYSIWYG and that concept doesn't really translate into screen readers. Sighted users will see something different visually between the backend and frontend and consider this a visual bug.
In terms of interactions, I agree, both sighted and screen reader users should have the exact same thing but the visuals are important for sighted users.
Would it help if we add a label or something saying "Visual Preview of block/whatever we're previewing" or something like that?
Also, this is independent of the use of inert, it has always been this way in the editor.
There was a problem hiding this comment.
Also, this is independent of the use of inert, it has always been this way in the editor.
That's got to change but I guess it can go into an issue for further discussion.
Labels don't help here because the preview itself is still not communicated equally.
Why is it not possible to disable interactions with blocks in an accessible way? #54322 accomplishes just that. People with vision can't interact with the block and screen readers can still see the block content but cannot interact with the block.
We've got to quit caring so much about this front-end vs. back-end. Honestly, do you think anyone at all would notice the usage of inert vs. aria-disabled with event listeners to disable a control? Absolutely not. It just makes it more work for developers and that I fear is the real problem here.
There was a problem hiding this comment.
Below example shows how this can still remain accessible but preventing the form from actually working.
<html>
<head>
<title>Test</title>
</head>
<body>
<h1>With inert</h1>
<form class="form1" action="https://www.google.com" method="GET" inert>
<input type="search" value="" name="search1" aria-label="Search 1" placeholder="Search 1" />
<input type="submit" value="Search 1" />
</form>
<br />
<h1>Accessible way but more work</h1>
<form class="form2" action="https://www.google.com" method="GET">
<input type="search" value="" name="search2" aria-label="Search 2" placeholder="Search 2" aria-disabled="true" />
<input type="submit" value="Search 2" aria-disabled="true" />
</form>
<script type="text/javascript">
const form2 = document.querySelector('.form2');
form2.addEventListener('submit', function(e) {
e.preventDefault();
} );
</script>
</body>
</html>
There was a problem hiding this comment.
I've created an issue where can record discussions and debate alternatives:
There was a problem hiding this comment.
Now that I think about it, all of our usage of "inert" can be seen as "screenshots". In that sense, I think maybe the solution going forward is to use a role="img" on these elements with a label. When you show a preview of a block or a template, it's just like an image.
What and Why?
The idea of this PR is that there's a new HTML attribute that was specifically added to address the need of our useDisabled/Disabled hooks/components. So let's just refactor this to use the attribute under the hood and rely on a polyfill for browsers that don't support it yet. The main reason for the switch is that our current useDisabled is too heavy and is buggy in some situations (switching back and forth from disabled to enabled can cause some issues sometimes)
Testing Instructions
1- Check that disabled content (block preview, some widget blocks like archives as a dropdown...) are disabled properly
Todo