-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[SpeedDial] Fix navigation between SpeedDialActions #12725
Conversation
8c5dbd9
to
765fdae
Compare
@mbrookes Since this is quite the rewrite of the old behavior I pushed the fix without tests to get some early feedback and writing tests for this behavior is quite tedious. I tested this locally in the lab docs. I can focus the dial fab and then navigate with arrow keys through the actions. |
@eps1lon Thanks for working on it. While basic keyboard interaction is back, some important accessibility requirements have been lost. To a visually impaired screen-reader user this is a component is effectively a drop-down menu, so:
For sighted users, the opposite behavior to 1 & 2 applies for the default orientation. This was baked into the logic, (and explained in the comments), but unfortunately wasn't taken into account in the "direction" PR (possibly because keyboard interaction had already been broken by then). How to address that is going to take a little more thought. |
I did not agree with the rationale. However I did not know that this is considered common behavior concerning a11y. The new implementation was just so tempting because it did not have to branch for different orientations. @mbrookes Does down still select the first item even if the orientation is horizontal and should we set |
Perhaps then you could explain how best you think it should operate?
That sounds closer to the truth. 😉 |
@mbrookes I should not have changed the behavior just because it was easy. I'm going to look at the spec, other components within this repo and other implementations of the |
Could not find a section about navigation in speed dial actions in the spec. Material web components don't have this implemented either. Does anyone know a google product that has FABs with action items that can be navigated with the keyboard? Angular has a demo but either this is bugged or highly confusing:
If you consider the first key stroke you have 4 (directions) * 4 (first arrow keys) = 16 navigation behaviors that you have to cover. If I press down on an up-wards oriented speed dial should down now move up-wards because screen reader users consider this a drop-down menu? Concerning wrapping: Taking all this into considerations I would propose the following specification:
Problems with A:
Problems with B: Problems with C: A taken from https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_slider_role @mbrookes What's your opinion on this? |
@eps1lon I absolutely love how you defined the specification, and I agree 100% with your conclusion on problems. This is the reason the current implementation was chosen (in terms of the logic - the code can always be improved.) Given the new (since the original implementation) requirement WRT orientation, the use of I stand corrected WRT to W3 view on wrapping - I was of the impression that without an index, a NOP signalling the end of a range was preferable. This is how a native HTML select, and native OS menus operate. I agree that we should follow the W3 recommendation, however counter-intuitive it might appear. |
@mbrookes ubuntu gnome menus do wrap that's why this felt natural to me. Probably everybody who is used to a certain behavior considers the other behavior counter-intuitive. So what I'm taking out this conversation is that we:
Looks like a good tradeoff to me. |
The reason why the W3 ARIA recommendation seems counter-intuitive Is that a totally blind person has no visual reference as to when a menu wraps, so has to depend on memorising the first option. It is perhaps a trade-off with ease of returning to the first item, having reviewed the available options. |
That makes sense. But the recommendation lists |
@mbrookes I implemented the behavior according to the proposed spec with alternative B. This should now match the old behavior. I'm going to checkout a previous working version and see if this is indeed true and then add some tests to avoid future regression. I would delay the |
className and classes.root was not applied to anything or forwarded to anything in the past. They should be implemented in the future however. A missing tooltipTitle caused a warning further down the component tree.
onKeyDown was applied to the button and the SpeedDialAction. However the Action did not process onKeyDown and passing the same event listener to different components is ambiguous.
Following the proposed spec with alternative B
8c02338
to
daabffc
Compare
@mbrookes |
@eps1lon Sorry for the delay. With the custom close icon example, how does a user keyboard-navigate back to the FAB? (I have some accessibility concerns about that feature anyway - with the role of the FAB changing between its closed and open states, but that's a different concern). Also, you mentioned the Home and End keys. Should we add those here, or is that best left for another PR (creature feep...)? |
Except that Escape closes the SpeedDial... In the case of the custom close icon example, when the speed-dial is open, the FAB becomes one of the actions (hence me accessibility concerns), but with the current wrapping there's no way to navigate back to it without closing the speed-dial and opening it again. Should it wrap back to the FAB in that scenario? |
A custom icon does not imply that the FAB emits an action. If anything there should be a prop to indicate that and only then should this action be included in the arrow key navigation.
I understand now. The FAB should only emit an action if the custom icon is displayed. But this brings me back to my first point. It should be an extra prop and the example should include the intention of the custom icon |
The action could simply be triggered in the That, though, still leaves the question as to when to include the FAB in the wrap cycle (if that's the approach we're taking). So, if we have to add a prop one way or the other, it might as well host the FAB action logic too. The alternative is that we ignore w3C on this one, and make the behavior consistent with how native |
We still have to decide whether the FAB should be included in the navigation. I think we can take a little freedom from the w3c recommendation here. Just include the FAB in the navigation and call it a day. Adding another prop to control the navigation just adds complexity. |
My point was, if you don't wrap, then the FAB is already included in the navigation, since if you press a direction key n times, then press the opposite direction key the same number of times, you return to the FAB.
The idea here was that if you're have to add another prop (forced by wrap), then rather than it being to control the navigation, it is the onClick prop for the FAB action, that only fires when the FAB is open (moving that logic into the component), and that controlling the navigation based on whether this prop is used is secondary. I think you're now saying to include the FAB in the wrap regardless of whether the FAB triggers an action. Personally I would prefer to revert to the previous behavior (no wrap), which matches how the native HTML select and Material-UI Select & Menu behave. |
@mbrookes |
@eps1lon Sorry, fixed the quoting (needed to escape
Yes, for that scenario, but it wouldn't be my first choice. |
Considering everything I think we should remove the wrapping behavior and include the FAB when navigating with arrow keys. Makes it easier to reason about and since there are very few items in the menu to begin with wrapping is not that important in my opinion. |
- FAB is included in navigation - Fixed potential issues when mixing keyboard and mouse navigation or refocusing a SpeedDial
5db2370
to
a4b65b7
Compare
Merged master. CI is still passing. Anything left to do @mbrookes ? |
@eps1lon No, I think we're good. (Other than codecov..._) |
a55b588
to
0efc141
Compare
Fixes navigation in SpeedDial between actions to the behavior with alternative B described in this spec.
There were quite some props which had no effect on the component.
onKeyDown
was used both inSpeedDialAction
andButton
butSpeedDialAction
never handled those. Edit: The intented behavior (navigating with arrow keys in SpeedDial) was broken and should be fixed now.className
was forwarded to Tooltip which was accepted prior to #12085 but not applied anywhere I can see (Tooltip prior to #12085].Overall I think every test should have at least one complete mount with the basic setup.
Not sure if we should introduce a root wrapper for the
SpeedDialAction
to enableclassName
orclasses.root
Todo:
Partial fix of #12159
Closes #12361
Followup:
className
[Tooltip] Causes SpeeDialAction propTypes warnings #12159