-
Notifications
You must be signed in to change notification settings - Fork 202
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
[CRISP-683] Add <sp-menu />
#96
Conversation
d51f9c3
to
decbca0
Compare
public selected = false; | ||
|
||
@property({ type: Number, reflect: true }) | ||
public tabIndex = -1; |
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.
I didn't know about this about this accessibility best practice (tabindex should be < 0). Learned something new!
I was wondering what the drawbacks were for setting the default to 0
instead of -1
.
In the -1
case, the consumer would have to explicitly set the tabIndex attribute to 0 for it to be tabbable.
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.
This is something that applies specifically to an element that will be delivered with the role="menuitem"
attribute. In that only one of the child items the role="menu"
parent CAN have tabindex="0"
at a time, making them all tabindex="-1"
by default and then allowing the parent element to manage which gets tabindex="0"
seems like the best approach here. I'm open to thoughts on a cleaner way to manage that.
Separately, this might play into the conversation we got into just a little bit last week or so around what should be a "package" and what should be part of one. Likely the sp-menu-item/devider/group
should all be "child" elements of the "menu" package to make that a little more clear. I'd prefer to address that in #116, but could be convinced to address this here if it helps clarify the above.
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.
Extended info on -1/0 as a value:
When an element have tabindex="-1"
what that means is that the element can be programmatically focused, but can't focus the element via keyboard input. This means that we can control for UIs (like menus) that might have a lot of entries can be given a single entry point, and then expose the rest of the list via opt-in (arrow keyss as opposed to tab) as is outlined in https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.html.
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.
Ahh okay, that makes sense. I wasn't aware that tabindex="-1"
had that behavior. Your implementation is crystal clear, I was just unfamiliar with the role="menu-item"
attribute.
Focusing through the menu items with the arrow-keys as opposed to tabs feels great!
Looks like we could add that arrow-key interface to our tab-list sometime in future as well:
https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html
Looks and feels great! The only thing left is documentation and it's good to go for me 👍 |
Thanks @andrewhatran, adding the docs is on the list! I also want to make sure I'm properly covered with tests, so if you have a minute to review: #117 it would be a big help. Thanks! |
…rols. Aria coverage.
7f76ff1
to
5e03fae
Compare
</sp-popover> | ||
``` | ||
|
||
## Accessibility |
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.
Let me know if I can be more clear here.
@@ -53,7 +53,7 @@ if [ -f /proc/version ]; then | |||
fi | |||
|
|||
# launch firefox and grab the PID | |||
"$FIREFOX_BIN" -profile $PROFILE_PATH -no-remote $1 & | |||
"$FIREFOX_BIN" -profile $PROFILE_PATH -no-remote -headless $1 & |
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.
Headless Firefox avoids issues where it doesn't allow you to capture focus.
@@ -12,8 +12,7 @@ import '../lib/banner'; | |||
import { Banner } from '../lib/banner'; | |||
import { fixture } from '@open-wc/testing'; | |||
import { html } from 'lit-html'; | |||
import { chai } from '@bundled-es-modules/chai'; | |||
const expect = chai.expect; | |||
import { expect } from '@bundled-es-modules/chai'; |
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.
Messed up the import syntax previously, this is simpler.
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 great! Thanks for writing such neat and thorough unit tests. Documentation and storybooks looked good, just have some nit-picky questions/comments
src/menu-group/menu-group.ts
Outdated
|
||
import menuGroupStyles from './menu-group.css'; | ||
|
||
let instances = 0; |
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.
What do you think about making this a private static property of MenuGroup? just for the sake of keeping the variable contained by the class its exclusively being used by.
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.
That sounds like an awesome alternative, but I'm not sure what that would look like. Could you share an example?
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.
sure! something like this:
private static instanceCount = 0;
constructor() {
super();
MenuGroup.instanceCount++;
}
public render(): TemplateResult {
const labelledby = `menu-heading-category-${MenuGroup.instanceCount}`;
...
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.
heres a helpful note about how it works: https://aliolicode.com/2016/05/07/typescript-static-members/
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.
Is this what you were thinking? I had a little trouble with the compiler when trying your version directly, but I can dig deeper if this wasn't what you were hoping for here.
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.
yep this is it! thanks 👍
} | ||
#button:hover, | ||
#button:focus, | ||
#button.is-open { |
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.
spectrum web component follows a pattern we rename boolean attributes from is-<attribute>
to just<attribute>
via spectrum-config.js
. could you do that for is-open
?
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.
Good to know. This specific instance is for functionality that isn't covered yet: https://opensource.adobe.com/spectrum-css/2.13.0/docs/#menu-nested Is it OK if I make an issue instead of addressing this right now?
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.
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.
wfm! thanks 👍
* @element sp-menu | ||
* | ||
*/ | ||
export class Menu extends LitElement { |
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.
out of curiosity, would there be any benefit for Menu to extend Focusable? I'm not sure if that would save lines for any of the focus logic but it might be worth considering if you haven't already!
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.
I'll look at this again, for sure. To support that, do you see functionality beyond public focus():
that is duplicated here?
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.
Right now its focus()
and potentially the keydown
and focusin
event handling, however if you think the behaviors are different enough I wouldnt worry about this. i was also wondering if it might be helpful to have the blur()
method as well though for de-selecting.
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.
I think the functionality in sp-menu
is pretty different than than in Focusable
. However, now that you've got me looking at it more, I'm unsure whether sp-menu
should have more of the functionality in Focusable
or not... I've created #147 to track and think it would be great to have the team look at it together at some point in the future. Accessibility is hard, and sometime it takes a village.
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.
lgtm, thanks 😃
Blocks
#95
Description
Add support for the Menu pattern from Spectrum CSS: http://opensource.adobe.com/spectrum-css/2.13.0/docs/#menu
Related Issue
Motivation and Context
Let's support more of the Spectrum CSS standards!
How Has This Been Tested?
I've added tests for all of the individual menu parts.
Screenshots (if appropriate):
Types of changes
Checklist: