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

CustomSelectControl: refactor with ariakit #55023

Open
52 of 85 tasks
brookewp opened this issue Oct 3, 2023 · 11 comments
Open
52 of 85 tasks

CustomSelectControl: refactor with ariakit #55023

brookewp opened this issue Oct 3, 2023 · 11 comments
Assignees
Labels
[Package] Components /packages/components [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@brookewp
Copy link
Contributor

brookewp commented Oct 3, 2023

Context

Continuing with the exploration started by @ciampo in #41466, this issue will track the migration of CustomSelectControl to Ariakit.

Legacy: #55234
New version: #55790

Initial steps

  • Analyze the legacy component, learn how it works, take note of its features
  • Study Ariakit's Select component
  • Look into ARIA roles
  • Consider using WordPressComponentsProps CustomSelect: Add WordPressComponentsProps #56998
  • Create a new, temporary component which should implement:
    • children (and a CustomSelectControlV2.Item component) to allow its consumers to specify select options
    • basic functionality (value / onChange / defaultValue / label / size props)
    • styles that match the legacy component
    • Storybook docs and examples (maybe one with post authors)
  • Refine README, types stories, etc CustomSelect: Adapt component for legacy props #57902

V2 Legacy Wrapper

Completed in #57902

V2

To discuss

  • Review API surface:
    • Based on how much we expect V2 default to diverge from V2 legacy wrapper, does it make sense to keep a common underlying implementation, since it comes at the cost of higher complexity in the internal implementation
      • if so, let's see if there is a better way to allow different behavior / styles between the two versions (we currently use React context + passing props to Styled components)
    • Should the components forward refs? If so, should they expose multiple refs? (ie. to the label, the button, and the popover)
    • How to differentiate between props for wrapper vs props for trigger? Should we have a trigger render prop, like we do for DropdownMenuV2 ? (see CustomSelectControlV2: Add root element wrapper #62803 (comment)))
  • Popover behavior and APIS:
    • Should the popover be modal ? Regardless, should it always prevent body scroll?
    • Where should the popover render in the DOM ? Always inline? Always portal-ed ? If so, in which slot (see Popover-based UI: do not render in Popover.Slot by default #56482)? Should we expose any props?
      • This should influence / should be influenced by DropdownMenuV2 too
  • Consider cloning + splitting V1 and V2 implementations, so that V2 can diverge without affecting V1
  • Do we need an isMultiple prop? (especially when neither value nor defaultValue are specified, but also for TS ergonomics)
  • Discuss whether it makes sense to tweak the size of each item based on the pointer type (smaller items for fine pointer control, larger items for coarser pointer)
  • Discuss best approach to the select popover height (including the maximum and minimum heights) (see Improve CustomSelectControl's usability around max height #49110)
  • Discuss best approach to the select popover flip behavior (especially in the light of CustomSelectControl V2 popover renders below position: sticky elements #63180)
  • Consider adding new features (like exporting more subcomponents, separators, group and group labels, etc)
    • Including new Select-related components recently added to Ariakit
  • Ask design for specs/review
    • Should it have more specific prefix/suffix structure, similarly to the new DropdownMenuItem ?
    • Checkmark/SelectItemCheck
      • should it be optional?
      • should it be instead exported as a subcomponent, and used by consumers when needed?
      • placement:

        Should the design for CustomSelectItem be inspired by the work? Specifically; placing the checkmark icon on the left?

    • review line height (see comment)

To implement

To check before releasing

  • Public-facing docs (Types, Storybook examples, READMEs)
  • Export the component as private API, start experimenting in the editor
  • Check with accessibility folks on this and for overall feedback
@brookewp brookewp added [Package] Components /packages/components [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. labels Oct 3, 2023
@brookewp brookewp self-assigned this Oct 3, 2023
@brookewp brookewp changed the title Components: refactorselect/combobox components with ariakit Components: refactor select/combobox components with ariakit Oct 3, 2023
@brookewp
Copy link
Contributor Author

brookewp commented Nov 27, 2023

@andrewhayward @afercia @alexstine

Hey folks! We're looking into improving the CustomSelectControl component. To start, we've created a basic experimental component based on Ariakit's Select.

Before getting too far into this, we wanted to check in with you all to hear your thoughts on this. Do you have any concerns with the existing CustomSelectControl or with Ariakit's Select? How does the new version of the new component feel so far?


Note

When testing the new version, the select's popover will appear to be misaligned. You can use the shortcut S to toggle the sidebar or open the story in its own window to see the correct styling.

@afercia
Copy link
Contributor

afercia commented Nov 28, 2023

@brookewp thanks for the ping.

Do you have any concerns with the existing CustomSelectControl

The only concern I have with the existing CustomSelectControl is that it seems to use a wrong ARIA role of listbox. The listbox role is more similar to a HTML select element with size attribute greater than one. Instead, what we want is to emulate a HTML select element with size one.

The Ariakit's Select implementation seems more correct to me, at a first glance. It uses a combobox role which is more appropriate. However, the combobox role has a few variants and several optional behaviors. it's a bit complex to understand. It's important to get it right since the start. Reference with examples in the ARIA Authoring Practices (APG): https://www.w3.org/WAI/ARIA/apg/patterns/combobox/

Some comboboxes allow users to type and edit text in the combobox and others do not.

To emulate a HTML select element
When a combobox does not support text input, it is referred to as select-only, meaning the only way users can set its value is by selecting a value in the popup. For example, in some browsers, an HTML select element with size="1" is presented to assistive technologies as a combobox.

This is the behavior we'd want to use for a HTML select-like component. All the properties to be used and intereaction are detailed in the APG. Example available here: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only

Input with suggestions
Instead, when a combobox supports text input, it is referred to as editable. An editable combobox may either allow users to input any arbitrary value, or it may restrict its value to a discrete set of allowed values, in which case typing input serves to filter suggestions presented in the popup

This is the pattern we'd want to use for ComboboxControl and FormFieldToken, which is more commonly called 'autocompleter with suggestions'.

Initial testing

Quickly testing with Safari and VoiceOver:

  • The default example seems to work okay to me.
  • The Multi-Select example dosn't work correctly: the announcement of the selected options is wrong, at least with Safari and VoiceOver.
  • The Controlled example: same, the announcement of the selected option is wrong.

@afercia afercia closed this as completed Nov 28, 2023
@ciampo
Copy link
Contributor

ciampo commented Nov 28, 2023

Thank you for looking in to this, @afercia ! And also for sharing some very useful links to the relevant parts of the specs.

The Ariakit's Select implementation seems more correct to me, at a first glance.

Thank you! That's good to know, especially since the new version of the component that Brooke has been working on is indeed based on the ariakit Select component family.

Quickly testing with Safari and VoiceOver:

  • The default example seems to work okay to me.
  • The Multi-Select example dosn't work correctly: the announcement of the selected options is wrong, at least with Safari and VoiceOver.
  • The Controlled example: same, the announcement of the selected option is wrong.

Could you be more specific about what exactly is wrong vs the expected behaviour?

I'm also going to re-open this issue, since it was meant to be a tracking issue for @brookewp's ongoing work on those components.

@ciampo ciampo reopened this Nov 28, 2023
@afercia
Copy link
Contributor

afercia commented Nov 30, 2023

I'm also going to re-open this issu

Oh sorry I think I closed it not intentionally, somehow.

Could you be more specific about what exactly is wrong vs the expected behaviour?

Well simply testing it with a screen reader clearly shows the bugs so I'm assuming this hasn't been tested properly.

In both the Multi-Select and the Controlled example, the 'selected' announcement is wrong. I think it's because of incorrect usage of aria-selected but I haven't looked into it in depth. I think this originates from some confusion about `aria-selected' as this attribute in some ARIA widgets configurations is meant to indicate the actually selected option while in other cases is meant to indicate the highlighted option for example while navigating with the arrow keys through a list of suggestions. More confuzion may arise when multi-selection comes into plau. The correct usage is well detailed in the ARIA Authoring Practices but it may be source of confusion.
I think it's an ariakit bug and should be fixed upstream. But, so far, we can't use the Multi-Select and the Controlled configurations.

I'm testing with:

  • macOS Sonoma 14.1.1
  • Safari Version 17.1 (19616.2.9.11.7)
  • VoiceOer Version 10 (919.1)

Multi-Select

Initial announcement of the two selected options looks correct:

Screenshot 2023-11-30 at 09 59 09

But, as soon as arrow keys are used to move to the other options, the announcement is wrong: 'flamingo pink' is not selected but it is announced as 'added to selection'.

Screenshot 2023-11-30 at 09 59 25

or, it announces that a selection has bee 'replaces' while it is not true:

Screenshot 2023-11-30 at 10 26 04

And again, moving through the options it announces 'removed from selection' or 'added to selection' when it's not true.

Controlled

This is a bit uncomfortable to test as there are a few containers with CSS overflow property that cut-off the dropdown. I removed those CSS using the dev tools inspector properties for testing purposes.

Again, navigating through the options, it may announce an option as 'selected' while it is not true:

Screenshot 2023-11-30 at 10 04 08

@diegohaz
Copy link
Member

Could you be more specific about what exactly is wrong vs the expected behaviour?

Well simply testing it with a screen reader clearly shows the bugs so I'm assuming this hasn't been tested properly.

In both the Multi-Select and the Controlled example, the 'selected' announcement is wrong. I think it's because of incorrect usage of aria-selected but I haven't looked into it in depth. I think this originates from some confusion about `aria-selected' as this attribute in some ARIA widgets configurations is meant to indicate the actually selected option while in other cases is meant to indicate the highlighted option for example while navigating with the arrow keys through a list of suggestions. More confuzion may arise when multi-selection comes into plau. The correct usage is well detailed in the ARIA Authoring Practices but it may be source of confusion. I think it's an ariakit bug and should be fixed upstream. But, so far, we can't use the Multi-Select and the Controlled configurations.

@afercia, thanks for testing. There's a bug in WebKit/Safari. I can reproduce the same behavior on the APG Multi-Select Listbox example.

Safari has known issues with aria-activedescendant. It appears the issue is being addressed for combobox widgets in the upcoming version. So, we can expect improvements in the near future.

For now, the aria-activedescendant can be disabled for the Ariakit Select component by setting the virtualFocus prop to false. This will enable the use of the roving tabindex approach, which seems to function well on Safari.

@ciampo
Copy link
Contributor

ciampo commented Nov 30, 2023

Thank you Diego for the reply!

For now, the aria-activedescendant can be disabled for the Ariakit Select component by setting the virtualFocus prop to false. This will enable the use of the roving tabindex approach, which seems to function well on Safari.

Do you suggest, for now, always setting virtualFocus: false to provide a better, more consistent experience across browsers?

@alexstine
Copy link
Contributor

The Safari bug is a really bad one. It also affected the block autocompleter.

#54902

Safari needs to keep support for ARIA attributes, that's the least Apple could do.

@afercia
Copy link
Contributor

afercia commented Dec 1, 2023

Safari has known issues with aria-activedescendant.

Yes that's a known problem but I don't think it's the only one.

Latest Safari version seems to made things even worse. However, we already went through listboxes, comboboxes, autotompleters, aria-activedescendant, and 'aria-selected` a few times. For example, we had an issue with the UrlInput in #47147 and we fixed it in #47148. At that time, the UrlInput worked with Safari and VoiceOver. Instead, now it doesn't work. Other autocompleters don't work with Safari / VoiceOver, for example the Command center, see #52930.
Although the combobox with autocompleter is a different pattern than a Listbox, I suspect the root problem is the same.
These ARIA patterns are extremely delicate and even a small detail could make a difference. Even the markup structure, like unexpected wrappers could have an impact.

Still, it's important to note that the 'Add new tag' combobox with suggestions does work with Safari / Voiceover. It does use
aria-activedescendant, it does use aria-selected, and it uses the old ARIA 1.0 pattern with arira-owns instead of aria-controls. It does not use the speak workaround implemented in #54902. It just works out of the box.

Since this combobox + listbox does work, I'd suggest to investigate in depth why it works and why the other ones don't. There must be something that we are missing (and I couldn't find it out so far).

Screenshot of the Tag suggestions working with Safari / Voiceover:

Screenshot 2023-12-01 at 10 12 11

Tested with:
macOS Sonoma 14.1.1
Safari Version 17.1 (19616.2.9.11.7)
VoiceOver Version 10 (919.1)

@ciampo
Copy link
Contributor

ciampo commented Dec 1, 2023

it's important to note that the 'Add new tag' combobox with suggestions does work with Safari / Voiceover

That would be the FormTokenField component, which you can see in isolation in Storybook

@alexstine
Copy link
Contributor

I believe it works likely because the controlled descendant appears directly after the input in the DOM.

@afercia
Copy link
Contributor

afercia commented Dec 4, 2023

I believe it works likely because the controlled descendant appears directly after the input in the DOM.

Yes it is possible. Also, I seem to recall Safari / VoiceOver don't work at all with options nested within groups.

@mirka mirka changed the title Components: refactor select/combobox components with ariakit CustomSelectControl: refactor with ariakit Apr 25, 2024
@ciampo ciampo self-assigned this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
Status: In progress (owned) ⏳
Development

No branches or pull requests

6 participants