test(combobox): add accessibility tests #3953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
why is my commit history always goofy :/ |
Westbrook
force-pushed
the
combobox
branch
2 times, most recently
from
086479a to
089e7e5
Compare
najikahalsema
force-pushed
the
combobox-a11y
branch
from
e6bfd12 to
4817d0a
Compare
Tachometer resultsChromeaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
alert-dialog permalink
asset permalink
avatar permalink
badge permalink
button-group permalink
button permalink
card permalink
checkbox permalink
coachmark permalink
color-area permalink
color-handle permalink
color-loupe permalink
color-slider permalink
color-wheel permalink
Firefoxaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
alert-dialog permalink
asset permalink
avatar permalink
badge permalink
button-group permalink
button permalink
card permalink
checkbox permalink
coachmark permalink
color-area permalink
color-handle permalink
color-loupe permalink
color-slider permalink
color-wheel permalink
|
added 7 commits
January 25, 2024 12:01
najikahalsema
force-pushed
the
combobox-a11y
branch
from
76b130e to
137c8b7
Compare
Westbrook
reviewed
Jan 25, 2024
| maxlength=${ifDefined( | ||
| this.maxlength > -1 ? this.maxlength : undefined | ||
| )} | ||
| minlength=${ifDefined( | ||
| this.minlength > -1 ? this.minlength : undefined | ||
| )} | ||
| pattern=${ifDefined(this.pattern)} | ||
| placeholder=${ifDefined(this.placeholder)} |
There was a problem hiding this comment.
Did we intend to remove this here?
There was a problem hiding this comment.
Oh, yeah, I did. There's no placeholder text for the combobox. We didn't even have this.placeholder defined, iirc
There was a problem hiding this comment.
I thought this would apply here, but maybe not. As much a design and accessibility speak against placeholder content, many consumers want control over it, so we likely should include it when possible.
There was a problem hiding this comment.
The point I'm trying to make is that a combobox doesn't have placeholder content to begin with. It's not part of the design spec or the WAI ARIA spec...
There was a problem hiding this comment.
Ah yes, placeholder is a part of the underlying <input type="text"> https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/placeholder, for good or ill people like to use it...
| /AppleWebKit/.test(window.navigator.userAgent) && | ||
| !/Chrome/.test(window.navigator.userAgent); | ||
|
|
||
| const comboboxFixture = async (): Promise<TestableCombobox> => { |
There was a problem hiding this comment.
Is one of the storybook stories sufficient to test against so that we don't need another instance of this?
There was a problem hiding this comment.
I liked that the test fixture only had 4 options, but I guess we could configure the stories to accept the options as an argument in the stories and just feed that into the test. Idk.
There was a problem hiding this comment.
Maybe we make the story template take options as an argument?
This also links up with #3953 (comment) and can be addressed later, though we should keep notes on any tasks of which we allow a deferral.
najikahalsema
commented
Jan 26, 2024
| @@ -14,11 +14,11 @@ import '@spectrum-web-components/combobox/sp-combobox.js'; | |||
| import '@spectrum-web-components/menu/sp-menu-item.js'; | |||
| import { html } from '@spectrum-web-components/base'; | |||
| import { measureFixtureCreation } from '../../../../test/benchmark/helpers.js'; | |||
| import { comboboxOptions } from '../index.js'; | |||
| import { benchmarkOptions } from '../index.js'; | |||
There was a problem hiding this comment.
changed this var name because it felt too generic.
najikahalsema
force-pushed
the
combobox-a11y
branch
from
767e6ea to
711e90a
Compare
Westbrook
pushed a commit
that referenced
this pull request
Jan 31, 2024
* chore: add labels to combobox input * chore: get tests passing * test(combobox): get a11y tests passing * chore: remove unused positionlistbox method * test: get tests passing, change spelling of activeDescendant * chore: missed some descendents * chore: add help text demo and test * ci: update hash * chore: address review comments * chore: abstract shared data to index files * test(combobox): update tests and stories to use legible data * ci: update hash * chore: label menu and rename stories * ci: update hash --------- Co-authored-by: Najika Yoo <naj.halsema@gmail.com>
Westbrook
pushed a commit
that referenced
this pull request
Feb 2, 2024
* feat(combobox): begin working branch for combobox additions * feat(combobox): add size attribute (#3887) * feat(combobox): wip * chore: update sizes and stories * chore: add isoverlayopen decorator to stories --------- Co-authored-by: Westbrook Johnson <westbrook.johnson@gmail.com> Co-authored-by: Najika Yoo <naj.halsema@gmail.com> * chore: add benchmark test for lightdom combobox (#3898) * chore: add benchmark test for lightdom combobox * chore: add object version of benchmark test * chore: rename files --------- Co-authored-by: Najika Yoo <naj.halsema@gmail.com> * test(combobox): update data management tests for current API * test(combobox): get more tests passing and skip tests that will be visited in future work (#3919) * test(combobox): get more tests passing and skip tests that will be visited in future work * ci: update golden images cache * test(combobox): ignore Combobox Item code * chore(combobox): cleanup unused code (#3922) * chore(combobox): cleanup unused code * ci: update golden images cache * fix(combobox): add support for external tooltip elements (#3930) * fix(combobox): add support for external tooltip elements * chore(combobox): remove unused code paths * ci: update golden images cache * docs(combobox): include slot present in API docs * fix(combobox): allow intern Menu to hold a selection when autocomplete === "none" (#3951) * test(combobox): add accessibility tests (#3953) * chore: add labels to combobox input * chore: get tests passing * test(combobox): get a11y tests passing * chore: remove unused positionlistbox method * test: get tests passing, change spelling of activeDescendant * chore: missed some descendents * chore: add help text demo and test * ci: update hash * chore: address review comments * chore: abstract shared data to index files * test(combobox): update tests and stories to use legible data * ci: update hash * chore: label menu and rename stories * ci: update hash --------- Co-authored-by: Najika Yoo <naj.halsema@gmail.com> * test(combobox): fulfil accessibility contract (#3974) * chore: add tooltip to ariadescribedby * test(combobox): add a11y test for tooltip --------- Co-authored-by: Najika Yoo <naj.halsema@gmail.com> * chore(bundle): include combobox * chore(combobox): clean up property availability and types * chore(combobox): clean up property availability and type * refactor(combobox): update ComboboxOption type * ci: update golden images cache * refactor(combobox): simplify typing and correct query location when moving items into viewport * docs: use human useful content in stories * ci: update golden images cache * refactor(combobox): default "autocomplete" to "none" * docs: correct story configuration * docs(combobox): create initial documentation * docs(combobox): apply docs feedback * docs(combobox): use only working examples in live mode * fix(combobox): maintain focus on input element when toggling menu * docs(combobox): improve clarity * ci: update golden images cache * docs(combobox): add story demonstrating controlled-component usage (#3988) * docs(combobox): add story demonstrating controlled-component usage * Update packages/combobox/stories/combobox.stories.ts Co-authored-by: Westbrook Johnson <wesjohns@adobe.com> --------- Co-authored-by: Westbrook Johnson <wesjohns@adobe.com> * fix(combobox): add support for "readonly" and "disabled" * docs(textfield): expand on attribute/property descriptions * fix(combobox): add support for "readonly" and "disabled" * ci: update golden images cache * fix(textfield): prevent outline on :focus-visible elements that are disabled * fix(combobox): correct value to itemText interchange when something is "selected" (#3994) --------- Co-authored-by: Najika Halsema Yoo <44980010+najikahalsema@users.noreply.github.com> Co-authored-by: Najika Yoo <naj.halsema@gmail.com> Co-authored-by: Hunter Loftis <hunter@hunterloftis.com>
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I have made the executive decision that a combobox, even with a tooltip, still needs a label.
To-do:
aria-activedescendentmatches the id of the selected menu item when press ArrowDown from the textfield.Will need to abstract the
nameattribute magic happening in the tests into a helper method at a later date. Someday... but not today.Related issue(s)
Motivation and context
accessibility is cool :)
How has this been tested?
Unit tests!
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.