ui icon indicating copy to clipboard operation
ui copied to clipboard

feat(select accessibility): add <SingleSelectA11y/> component

Open Mohammer5 opened this issue 1 year ago • 6 comments

Implements LIBS-559

Storybook demos here.


Description

Adds an accessible version of the single select component. This component is significantly different from the existing single select component in some aspects, so I decided to create a new component rather than modifying the old one.

In order to make this component accessible:

  • The selected value is [role="combobox"]
    • The combobox has aria-placeholder, aria-labelledby & aria-expanded
    • The arrow inside the combobox is a button
    • Clicking the button will focus the combobox
    • The combobox points to the listbox via aria-controls={listBoxId} and aria-haspopup="listbox"
  • The menu is [role="listbox"]
    • The listbox notifies the user about changes via aria-live and aria-busy
  • Options are button elements with [role="option"]
  • Options have aria-selected, aria-disabled & aria-label
  • The search input has a aria-label when a filterLabel has been provided

Style-wise there should be no difference between the existing component and the new one. There are some differences in which props exist and which props are expected:

The new component

  • expects a name prop.
    The HTML element that displays the selected value (combobox) needs to point to the element that has the options (listbox), which is done by using id attributes. Instead of creating one randomly, I decided to make this prop requried.
  • expects an options array rather than children.
    This makes it easier to access the options without having to go through children. In order to support custom options, the options can have a component prop, which will override the default option style while the accessible option wrapper will stay intact
  • accepts a selected prop which is an option ({ value: string, label: string }).
    This should help providing the selected value and its label while still loading the options. This eliminates an issue we've had in the past with other components where the selected option would have to be added to the options array, regardless of whether it should be displayed or not
  • requires the clearText prop to be present when the select is clearable.
    This way we can have an aria-label on the clear button.
  • has a filter, which is now controlled by the consumer (filterValue & onFilterChange).
    Consumers can now use the filter to load more options or control the options list in any other way
  • has a different set of data-test properties

Breaking changes (disregarding prop changes) if we'd replace the existing component

  • The component can't be given an arbitrary set of children. It doesn't accept any children whatsoever
  • Filtering doesn't work out of the box

Thinks I want to discuss

  • [ ] The new component has A11y in its name. I don't think we should replace the old component but deprecate it to give developers time to transition to the accessible variants. But how should we call the new one?
  • [ ] The new component accepts an autoFocus prop, which was called initialFocus before. I didn't really think about the name until I realized it deviates. I suppose I should just use the old name?
    autofocus is the name of the actual HTML attribute
  • [x] When using custom options, every option in the array has to have a component property. Would it make sense to introduce a optionComponent prop on the select which would apply to all options unless an individual option has a component property?
  • [ ] The listbox uses aria-live and aria-busy to update the user about changes. The aria-live attribute can have different values: off, polite & assertive. I think assertive could be better, but depends on the use-case I suppose. Do we want the consumer to be able to control this value? And if yes, what should the default value be?
  • [ ] The transfer component has areas in which the app can place anything.
    I think it could be useful for the transfer to do the same (above and below the options (and the listbox HTML element) in the menu). We could even get rid of the search functionality here and let apps handle that, making the component leaner and reducing the maintenance burden in the app.

Open TODOs

  • [x] Keyboard interactions
  • [x] Loader at the bottom of the list is missing
  • [x] Handle disabled states when handling keyboard inputs
  • [ ] Padding needs to be added to the filter field
    • The commented out padding causes the intersection observer to not detect the last option (when adding the styles back) as having become visible when scrolling down. The issue is described in this stackoverflow article, although in our case it's not about a child but about a sibling element. I already tried using an inner div and assign the padding to that element, but that caused the same problem...
    • I think this should not prevent us from merging the PR, as this issue doesn't impede functionality

Checklist

  • [x] API docs are generated
  • [ ] Types added / updated
  • [x] Tests were added
  • [x] Storybook demos were added

Code explanations

Component implementation:

https://github.com/user-attachments/assets/628c5f53-f55c-4fec-a7a4-a7550dafff4e

Test implementation:

https://github.com/user-attachments/assets/bd1ec320-da01-4343-910c-0f647b75bc4a

Documentation:

https://github.com/user-attachments/assets/483e9f4e-6ead-4ccb-bd4b-1ba1e3c8aba3

Mohammer5 avatar Oct 16 '24 02:10 Mohammer5

🚀 Deployed on https://pr-1620--dhis2-ui.netlify.app

dhis2-bot avatar Oct 16 '24 02:10 dhis2-bot

What exactly are the breaking changes compared to the previous input besides new props and features? Are certain functionalities no longer working?

I would say a breaking change release would be better than introducing a new component, and if both components should be kept in the package maybe the old one should be renamed so people have the option to upgrade with just a name change?

I am also missing some documentation about this in the docs/docs directory.

Topener avatar Oct 16 '24 14:10 Topener

@Topener

What exactly are the breaking changes compared to the previous input besides new props and features? Are certain functionalities no longer working?

The existing component accepts children, which means you can pass anything to the component and it will be rendered in the menu, which you can see here. This means that apps that rely on this feature can't do the same with the new component.

Other potential breaking changes relate to custom options: The options in the new component uses a button element and customization can only be done within the button. If apps were putting block elements or several buttons into a single option, this wouldn't be possible anymore.


I would say a breaking change release would be better than introducing a new component, and if both components should be kept in the package maybe the old one should be renamed so people have the option to upgrade with just a name change?

I can elaborate my reasoning for introducing an additional component with a new name: I'm mostly worried about the impact on devs when they want to upgrade UI for other reasons. Say we introduce a breaking change but the teams don't have time at that moment to transition, then release another feature that is unrelated to the select component and one of the apps needs that change. If the app has many selects, it'd be a lot of work. Releasing two components, deprecating the old one and keeping its name for the time-being would allow devs a transition time.

Of course we could force apps to use the new component and be more accessible asap. This mostly depends on how we want to move forward organization-wide. My stance is to be careful which breaking changes in essential components and give developers enough time to transition from one component to another without stopping them from upgrading the library when they need other changes.


I am also missing some documentation about this in the docs/docs directory.

Yes, this is still draft and the docs checkbox isn't checked. The goal is to discuss the general direction now that I have something I can show.

Mohammer5 avatar Oct 17 '24 02:10 Mohammer5

Alright I get the reasoning! I'm not the one to decide that but that would be my recommendation. Releasing a new major version also allows for breaking changes and I think that is the best way forward as it would be a new best practice. It also allows people to stay on an older version if they don't have time to upgrade. We'll just have to document the breaking change properly, not only in the releasenotes but also in the /docs/docs directory with the component

Perhaps @amcgee can share his opinion on the matter.

Topener avatar Oct 17 '24 15:10 Topener

Quality Gate Failed Quality Gate failed

Failed conditions
44 New issues
2 New Bugs (required ≤ 0)
5 New Critical Issues (required ≤ 0)
B Reliability Rating on New Code (required ≥ A)
42 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

sonarqubecloud[bot] avatar Nov 28 '24 08:11 sonarqubecloud[bot]

Quality Gate Failed Quality Gate failed

Failed conditions
48 New issues
B Reliability Rating on New Code (required ≥ A)
5 New Critical Issues (required ≤ 0)
46 New Code Smells (required ≤ 0)
2 New Bugs (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

sonarqubecloud[bot] avatar Nov 20 '25 14:11 sonarqubecloud[bot]

thanks @Mohammer5 for the great work here

kabaros avatar Dec 03 '25 13:12 kabaros

:tada: This PR is included in version 10.10.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

dhis2-bot avatar Dec 03 '25 14:12 dhis2-bot