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

feat: add combo-box using slotted input #2293

Closed
wants to merge 3 commits into from
Closed

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Aug 3, 2021

Description

This PR adds @vaadin/combo-box base class, Lumo and Material theme, and visual tests.

Fixes #2202

Breaking changes

  • Removed deprecated render method

Note

Some logic is imported from vaadin-combo-box package:

  • ComboBoxDataProviderMixin
  • vaadin-combo-box-dropdown-wrapper
  • Lumo and Material for combo-box dropdown

These will be updated in #2203, as well as JSDoc and missing TS typings.

Type of change

  • Feature

@web-padawan web-padawan added the a11y Accessibility issue label Aug 3, 2021
@web-padawan web-padawan changed the title [WIP] feat: add ComboBox element base class feat: add combo-box using slotted input Aug 4, 2021
@web-padawan web-padawan marked this pull request as ready for review August 6, 2021 10:47
@fhdhsni fhdhsni self-requested a review August 10, 2021 09:39
@web-padawan web-padawan marked this pull request as draft August 10, 2021 12:50
@fhdhsni
Copy link

fhdhsni commented Aug 10, 2021

Some observations when testing the component:

  1. number of available options is not announced when opening the listbox. I'm not sure how it can be fixed.
    Here is an example of a combo-box which announces the number of options. It uses adobe spectrum combo-box component.

  2. It's a good idea to add aria-setsize and aria-posinset since the component supports lazy-loading.

  3. when an option is selected, let's say foobar, it is announced as foobar unselected (VoiceOver chrome). On Safari it doesn't say unselected but it doesn't say foobar selected either, which would be more clear.

  4. When going through the options, it doesn't announce which one is selected/not selected. You can test the previous example for a working case (Chrome announces not selected items, Safari announces the selected item)

  5. The dropdown button should be accessible? it's not at the moment but doesn't seem a big deal. It can provide another way to open the list though. If we decided to make it accessible it should have aria-haspopup="listbox" attribute.

  6. the input should have an aria-activedescendant attribute indicating which option is visually focused.

  7. since home/end keys don't move the visual focus to the first/last option, up-arrow/down-arrow keys should wrap when the visual focus is on the first option or the last option.

  8. aria-owns and aria-controls on the input referring to the listbox.

  9. aria-haspopup="listbox" on the input, although apparently role=combobox adds that by default but I saw it being used in a lot of places in addition to the role=combobox (for example reach-ui). Maybe it's necessary for some screen readers.

  10. listbox should have aria-labelledby/aria-label attributes

@web-padawan
Copy link
Member Author

the input should have an aria-activedescendant attribute indicating which option is visually focused.

This and other related changes will be done as part of #2240

@vursen
Copy link
Contributor

vursen commented Aug 12, 2021

Removed deprecated render method

Removed processTemplates and corresponding tests

While the reason for removing the deprecated render method is pretty much clear, I don't truly understand why to also remove processTemplates().

Did we agree at any moment to stop supporting Polymer Templates even with the template renderer?

@tomivirkki @web-padawan

@tomivirkki
Copy link
Member

Hmm...can't drop support for <template>s yet. The Flow counterparts also still depend on them (TemplateRenderer for example).

@web-padawan
Copy link
Member Author

I will restore the <template> support and corresponding tests.

@web-padawan web-padawan force-pushed the feat/combo-box branch 2 times, most recently from 546ce19 to 6095519 Compare August 16, 2021 13:22
@web-padawan web-padawan marked this pull request as ready for review August 16, 2021 13:43
@sonarcloud
Copy link

sonarcloud bot commented Aug 19, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 37 Code Smells

No Coverage information No Coverage information
8.4% 8.4% Duplication

@web-padawan
Copy link
Member Author

Closing in favor of #2496

@web-padawan web-padawan deleted the feat/combo-box branch April 29, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-implement combo-box using slotted input and new mixins
4 participants