-
Notifications
You must be signed in to change notification settings - Fork 205
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(dropdown): improve accessibility #905
Conversation
83fc227
to
ffb1311
Compare
b09b4f0
to
d03d62c
Compare
d03d62c
to
82fd8ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions that might lead to a few more changes. Overall looks great!
@@ -63,13 +63,11 @@ export class Menu extends SpectrumElement { | |||
} | |||
|
|||
public focus(): void { | |||
if (this.menuItems.length === 0) { | |||
if (!this.menuItems.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need this change -- is this just a code style choice, or is there something subtle I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just style, I had removed this check at one point and then re-added it not knowing the style changed. There may be some slight amount of type security outside of the TS space in the small fraction of time this.menuItems === undefined
, but I don't know enough V8/timing to confirm.
${this.buttonContent} | ||
</div> | ||
`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hadn't realized this extended from ActionButton -- neat. Whenever something like a core render method is overridden I try to think about maintainability. Since the only difference is the tag name, is that something you could parameterize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make the source render method more composable so that we only have to override part of it:
protected render(): TemplateResult {
return this.isAnchor ? this.renderAnchor : this.renderButton;
}
protected get renderAnchor() {}
protected get renderButton() {}
That's probably nicer system wide, I'll take swing at it.
e87ba76
to
2f25390
Compare
@adixon-adobe this has been updated. It's got a couple of settings and changes that it would be good to have in #912 to speed/stabilize the CI process while we lock down the docs/visual regression cache. Let me know if you see any other good suggestions herein! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good -- thanks!
Description
Update the accessibility tree of the
Dropdown
pattern and its derivative components (ActionMenu
andSplitButton
) as well as theMenu
pattern that they leverage internally.Preview available: https://5f6e5921adb59e0095ccb7c2--spectrum-web-components.netlify.app/
This does:
:focus
to[focused]
to ensure transient visual staterole="group"
as per its updated rulesMenuItems
withid
s when missingoption
s andmenuitem
sdiv
elements overbutton
s in<sp-menu-item>
This does NOT:
role=combobox
patterntextfield
as a child of this roleRelated Issue
fixes #764
refs #292
fixes #908
Motivation and Context
Components should be accessible x-browser, x-context, etc.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: