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

[Data Views] Don't fully disable buttons that will change state based on user action #56396

Closed
andrewhayward opened this issue Nov 21, 2023 · 10 comments · Fixed by #56422
Closed
Assignees
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@andrewhayward
Copy link
Contributor

andrewhayward commented Nov 21, 2023

What problem does this address?

As per WCAG SC 3.2.2 (On input):

Changing the setting of any user interface component does not automatically cause a change of context unless the user has been advised of the behavior before using the component.

A 'change of context' is defined as something "that, if made without user awareness, can disorient users who are not able to view the entire page simultaneously". This includes focus.

Disabled controls cannot natively have focus. This means that if the act of interacting with a control (for example pressing a button) disables it, then focus will move somewhere else. Without careful focus management, this can be anywhere, and usually nowhere related to the task at hand.

There are several buttons used by data views that fall into this trap.

A set of pagination controls, with disabled buttons for the first page, previous page, next page, and last page, all circled in red.

For example, when the "first page" button in the pagination controls is pressed, the user is taken to the first page of results, and the button is disabled, but it is not clear where focus moves to. The same also applies to the "previous", "next" and "last" buttons.

What is your proposed solution?

Rather than set disabled on these buttons, we should instead use aria-disabled. This will leave the buttons focusable, but announced as disabled to assistive technologies.

The most appropriate way to do this is by setting both disabled and __experimentalIsFocusable on the Button component.

Note

There is a problem with focus rings on some focusable disabled buttons, but this is noted and being fixed.

A disabled button, titled 'Reset', circled in red.

This means that the "reset filters" button, for example, visually appears to have this focus problem, but in fact does not.

@andrewhayward andrewhayward added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Nov 21, 2023
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 22, 2023
@ntsekouras
Copy link
Contributor

Thanks for the issue! I opened a quick PR to fix this, but I'm wondering if there are cases where we don't need this for Buttons, since it's the best a11y practice. Should this be the default behavior for Button component? 🤔 --cc @ciampo

@ciampo
Copy link
Contributor

ciampo commented Nov 23, 2023

We could take this into account, yeah. @andrewhayward @afercia , do you see any potential drawbacks if the Button component had the __experimentalIsFocusable prop true by default?

@afercia
Copy link
Contributor

afercia commented Nov 27, 2023

Late to the party. This is very similar to what happens in the Patterns explorer modal dialog, see #55110 / #56162

do you see any potential drawbacks if the Button component had the __experimentalIsFocusable prop true by default?

Not sure that would be ideal. I'll create a separate issue or maybe a discussion.

As an aside, there are two more problems to consider for this pagination:

  • The number filed triggers an unexpected change of context because it uses onChange. As soon as the number value is changed via keyboard, the view updates. That's a context change that needs to be avoided and onChange in these cases is an accessibility anti=pattern that needs to be banned.
  • Both the data views and the Patterns axplorer have a Pagination component. They are very similar and basically they are code duplicaiton. They should abstracted to a separate, reusable, component.

@ciampo
Copy link
Contributor

ciampo commented Nov 28, 2023

  • The number filed triggers an unexpected change of context because it uses onChange. As soon as the number value is changed via keyboard, the view updates. That's a context change that needs to be avoided and onChange in these cases is an accessibility anti=pattern that needs to be banned.

Just to make sure folks can look into improving this, what would be a better solution? Could we use an aria-live region to notify users about what updated?

@afercia
Copy link
Contributor

afercia commented Nov 30, 2023

Just to make sure folks can look into improving this, what would be a better solution? Could we use an aria-live region to notify users about what updated?

I don't think that would solve the issue. It's not just about screen readers. It's general interaction: changing the value should never trivver a page update. Say I want to go from page 1 to page 10. That would trigger 10 page updates. As a user, that's not what I want.

This fields should not use onChange. It should allow users to change the value and submit.
We'd need to replicat ethe same experience and interaction as it is used in the core admin pagination, where pagination triggers on submit.

@andrewhayward
Copy link
Contributor Author

andrewhayward commented Nov 30, 2023

The number filed triggers an unexpected change of context because it uses onChange. As soon as the number value is changed via keyboard, the view updates.

Strictly speaking, onChange doesn't fire until the user hits enter, or the field loses focus. Which, to me, would be sufficient in this case to indicate a user-initiated and intended action.

That being said, it looks like we're actually using onInput (or at least replicating it), and the data is being reloaded with every single keypress, which I agree is not a good experience.

That's a context change that needs to be avoided and onChange in these cases is an accessibility anti=pattern that needs to be banned.

I'm not sure as I entirely agree with this. As per the spec...

Tip

A change of content is not always a change of context. Changes in content, such as an expanding outline, dynamic menu, or a tab control do not necessarily change the context, unless they also change one of the above (e.g., focus).

Important

Opening a new window, moving focus to a different component, going to a new page (including anything that would look to a user as if they had moved to a new page) or significantly re-arranging the content of a page are examples of changes of context.

From my perspective, we're not doing anything that would qualify as a change in context here, though I'm happy to hear any thoughts on why we might be. We're not reloading or navigating elsewhere, focus is remaining correctly, and the content is not being significantly rearranged (to follow the examples given).

The only part of the page that changes is the data table, which, honestly, I would entirely expect on a page transition. The rest of the supporting UI doesn't change at all. I don't think the change in content here changes the meaning of the page, or its context.

@afercia
Copy link
Contributor

afercia commented Dec 4, 2023

Strictly speaking, onChange doesn't fire until the user hits enter, or the field loses focus. Which, to me, would be sufficient in this case to indicate a user-initiated and intended action.

That being said, it looks like we're actually using onInput

Oh yes, I think I had in mind the onChange event for select elements, thanks for correcting me.

From my perspective, we're not doing anything that would qualify as a change in context here

Well... in the spec you quoted there is a relevant part. It should link to the WCAG 2.2 though, as that's the current spec.

going to a new page (including anything that would look to a user as if they had moved to a new page) or significantly re-arranging the content of a page

To me the parts

  • 'including anything that would look to a user as if they had moved to a new page'
  • 'or significantly re-arranging the content of a page'

fully qualify updating the table as a change of context.

The only part of the page that changes is the data table, which, honestly, I would entirely expect on a page transition. The rest of the supporting UI doesn't change at all. I don't think the change in content here changes the meaning of the page, or its context.

I kindly disagree. The 'only' part that changes is actualy the main content of the page. All the rest is the UI frame, which is not relevant to the information provided by the page.

I don't think the change in content here changes the meaning of the page

Hum, it does change it. It's a new page, with new data presented to the user. I would agree the structure of the page does not change. The actual data changes though.

I don't think such a change would be accessible for, for example:

  • screen reader users
  • low vision users
  • users with cognitive impairments

@joedolson
Copy link
Contributor

First of all, I don't think that a field losing focus should be interpreted as user intent to change context. It can just as easily be a user changing their mind or exploring the page to get more information before they commit to a page change. If this change occurs on a focus loss, the user effectively has no way to leave the field without updating the results.

Even if resetting the value to the current page would avoid updating results, that depends on the user a) knowing what page they were on when they started changing the value and b) requires the user to lose the value they had typed in. Both of these are problems for accessibility; in the first case, screen readers may need to explore the page to identify the current page and in the second case, users with memory or cognitive impairments may struggle to adjust state.

As far as the actual definition of a context change, I agree that this is fuzzy. However, I would argue that going to a new page of results is not just something like a user going to a new page, it is literally a new page. All of the relevant content of the page has now changed.

Most importantly, to me, a change that happens when focus moves is unexpected, and I'm uncomfortable with any change that isn't explicitly requested by a user. Users shouldn't expect moving focus to result in content changing.

@alexstine
Copy link
Contributor

Yeah, this is not a good interaction. I understand it from a matter of web development but this doesn't make sense for keyboard users. The only way a pagination input like this works is with a submit button. E.g.

<label for="input1">Go to page</label>
<input type="number" id="input1" min="1" max="50" value="1" />
<input type="submit" value="Go to page" />

Expecting someone to know there is an event listener on a field is the very problem with front-end development today. Not saying anyone specific to WordPress, just in general. Actions should be explicit, not guesses.

Thanks.

@afercia
Copy link
Contributor

afercia commented Dec 13, 2023

I'm uncomfortable with any change that isn't explicitly requested by a user

This. Thanks Joe and Alex.

For history: prior Trac ticket about the use of onchange event. Although related to <select> elements, the root problem is the same: https://core.trac.wordpress.org/ticket/40925

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants