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

fix(chips): use built-in radiogroup / checkbox behavior for a11y #890

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

jonaskuske
Copy link
Contributor

@jonaskuske jonaskuske commented Jul 16, 2024

Chips are composed of a checkbox/radio input and a label. As of now, the input is hidden entirely from screenreaders via display: none and the label has role=button, aria-hidden=true and is focusable via tabindex=0, though the latter is only true in the core examples, not in db-ui/elements*.

Problems:

  • In db-ui/elements, chips are not focusable at all! The input is hidden and the label is not focusable
  • In db-ui/core, chip labels can be focused. But they can't be interacted with! They have role=button, but no keydown event handlers
  • The labels (that act as buttons) have no explicit focus styles – though the user agent might automatically apply some (in Edge: a black outline)
  • When a screenreader is active, interaction can work. Windows Narrator seems to dispatch click despite the missing keyboard event handlers. But I'm not sure if all screenreaders do this (I only tested with Windows Narrator), and also this means interaction via keyboard is inconsistent and works with an active screenreader, but doesn't work without one
  • There's no selected state and no information about the different behavior, I don't know which options are selected and whether selecting one chip de-selects the others

Suggestion

I'd suggest to keep radiogroup (interactiontype=selection) or group (interactiontype=filter) semantics for a set of chips, which is achieved by keeping the input in the a11y tree as the focusable element (only hide it visually) and using the label for the visual style. This way:

  • chips (their inputs) are focusable in both core and elements
  • there is consistent visual feedback when a chip is focused
  • keyboard navigation (via arrow keys for selection/radiogroup usage, via tab for filter/checkbox usage) works with and without an active screenreader
  • screenreaders can provide more useful information for each chip, like its checked state, the control type or something like "Food & Drinks, element 4 of 7"

Recommended usage:

<div role="radiogroup" aria-labelledby="selection-label">
  <span id="selection-label">Select a drink:</span>
  <db-chip interactiontype="selection" name="drink-select" selected>Water</db-chip>
  <db-chip interactiontype="selection" name="drink-select">Wine</db-chip>
  <db-chip interactiontype="selection" name="drink-select">Juice</db-chip>
</div>
<div role="group" aria-labelledby="filter-label">
  <span id="filter-label">Filter drinks:</span>
  <db-chip interactiontype="filter" name="drink-filter">Water</db-chip>
  <db-chip interactiontype="filter" name="drink-filter">Wine</db-chip>
  <db-chip interactiontype="filter" name="drink-filter">Juice</db-chip>
</div>

Downsides:

Filter and selection chips visually look the same, but are controlled differently: filter chips are a group of checkboxes, meaning the chips can be individually navigated to through Tab and then activated through space/enter, and selection chips are a radiogroup, where the whole group is navigated to through Tab, and then you can switch between the options with the arrow keys. But that's just using built-in radiogroup and checkbox semantics and is announced as such, so I think that's okay?

Alternatives considered:

  1. Implement the set of chips as proper listbox with custom event handlers, aria-selected state etc.
  2. Keep current behavior of chips acting as buttons, but add consistent focus styles and a keydown handler triggering this.click() and make label[role=button"] focusable in db-ui/elements, too

The first is a lot of effort and with the second the elements still lack selected state and other relevant info.

fix #694
fix #13

Related: db-ui/elements#2659

@mfranzke
Copy link
Member

@jonaskuske great catch, and thanks a lot especially for your comprehensive research and explanations as well as providing the results in code as well already. kudos to you !!

@mfranzke
Copy link
Member

mfranzke commented Jul 18, 2024

@jonaskuske what do you think about using fieldset & legend instead of div role="radiogroup" or div role="group"? This might be another advantage and we could get rid of the aria-labelledby as well. I could add those aspects to the documentation than.

@jonaskuske
Copy link
Contributor Author

@mfranzke <fieldset> + <legend> would of course be preferable and is what I attempted at first, but then opted for the role + aria-labelledby approach due to styling concerns because I want to display the group label in line with the row of chips and am probably not the only one at that. Not being able to simply do display: flex on the fieldset is super counter-intuitive.

Apparently using float: left on the legend works in all browsers, but that feels a bit iffy and devs will probably continue to stumble into this and wonder why a simple display: flex on the fieldset has no effect.

The fix is to set display: contents on the legend, then it will participate in its parents flex layout. Proposed here and indeed added to the spec: https://thatemil.com/blog/2017/07/01/fixing-fieldsets/ (author added an update at the bottom). It works in Chrome, can't test in other browsers. If it's supported cross-browser too nowadays, the <fieldset> + <legend> approach is fine imo, though still a bit of a footgun.

@mfranzke mfranzke added the changes needed Our consumers would need to make some changes in their codebase. label Jul 25, 2024
@mfranzke mfranzke self-assigned this Jul 29, 2024
@mfranzke mfranzke changed the base branch from main to release-candidate August 2, 2024 13:14
@mfranzke mfranzke merged commit a80f7e5 into db-ui:release-candidate Aug 6, 2024
12 checks passed
@mfranzke mfranzke mentioned this pull request Aug 6, 2024
mfranzke added a commit that referenced this pull request Aug 6, 2024
* fix(chips): add focus styles, make input focusable & expose label

* chore(chips): remove html lint ignores, add label for single-chip entry in patternlab

* Update chip.scss

---------

Co-authored-by: Maximilian Franzke <787658+mfranzke@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working changes needed Our consumers would need to make some changes in their codebase. community feedback
Projects
None yet
2 participants