flyout: a11y: Allow users to control flyout menus with keyboard [chromium/src : main]
Masa Fujita (Gerrit)
Masa Fujita has uploaded the change for review![]( "Open in Gerrit")
Commit message
flyout: a11y: Allow users to control flyout menus with keyboard
This CL adds the ability for the user to use the keyboard to control the
flyout menus.
When the user selects an item with arrow keys and decides to proceed,
the `View`'s click listeneris called. We capture this and run the flyout
logic when necessary.
This CL also makes it so that `ListMenuItemAdapter` sets up the view to
use both the delegate and the click listener upon click.
Change diff
Change information
- M ui/android/java/src/org/chromium/ui/listmenu/ListMenuItemAdapter.java
- M ui/android/java/src/org/chromium/ui/listmenu/ListMenuUtils.java
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita added 1 comment![]( "Open in Gerrit")
Related details
- Jenna Himawan
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 1 comment![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
1 comment
boolean handled = false;
if (hasClickListener(item)) {
// The item had a click listener in the model, but none was bound by the
// ViewBinder, and there is no click delegate to use. In this case, call
// the model click listener directly.
item.model.get(CLICK_LISTENER).onClick(view);
handled = true;
}
if (mDelegate != null) {
// Delegate, if possible.
mDelegate.onItemSelected(item.model);
handled = true;
}
if (handled) {
return;
}
// As a fallback, use ListView's performItemClick.
long id = getItemId(position);
((ListView) parent).performItemClick(v, position, id);@jhim...@google.com This is the part I'm unsure about - do you think it sometimes makes sense to run both the delegate and the click listener?
Related details
- Jenna Himawan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jenna Himawan (Gerrit)
Jenna Himawan added 6 comments![]( "Open in Gerrit")
* @param item The ListItem that is the target of the hover event.Replace hover -> "focused" in the Javadoc
updateHighlightPath(highlightPath);I'm a bit confused on why we're calling this here. Can you provide better documentation about what the method does?
I read the code a bit more. My understanding is that the `List<ListItem> highlightPath` is updated, but updateHighlightPath is not called right away. We update the highlight path, then pass the updated highlight path to another method, and expect the other method to updateHighlightPath using the parameter.
Could we not just updateHighlightPath right away?
* will be ran.nit: "both will run" (or "both will be run")
(if we keep this change)
boolean handled = false;
if (hasClickListener(item)) {
// The item had a click listener in the model, but none was bound by the
// ViewBinder, and there is no click delegate to use. In this case, call
// the model click listener directly.
item.model.get(CLICK_LISTENER).onClick(view);
handled = true;
}
if (mDelegate != null) {
// Delegate, if possible.
mDelegate.onItemSelected(item.model);
handled = true;
}
if (handled) {
return;
}
// As a fallback, use ListView's performItemClick.
long id = getItemId(position);
((ListView) parent).performItemClick(v, position, id);@jhim...@google.com This is the part I'm unsure about - do you think it sometimes makes sense to run both the delegate and the click listener?
We haven't had a scenario so far where this is true. Do you need it for your changes?
flyoutController.handleEnterEvent(It seems that the "handleEnterEvent" is activated on click. This makes sense, but you should probably rename the method so that its name is compatible with any user input that should trigger the flyout right away (including click, right arrow, space, and enter)
flyoutController.handleEnterEvent(You can handle this in a follow-up, but pressing the keyboard right arrow should also enter the flyout.
Related details
- Masa Fujita
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 6 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
6 comments
* @param item The ListItem that is the target of the hover event.Replace hover -> "focused" in the Javadoc
Done
updateHighlightPath(highlightPath);I'm a bit confused on why we're calling this here. Can you provide better documentation about what the method does?
I read the code a bit more. My understanding is that the `List<ListItem> highlightPath` is updated, but updateHighlightPath is not called right away. We update the highlight path, then pass the updated highlight path to another method, and expect the other method to updateHighlightPath using the parameter.
Could we not just updateHighlightPath right away?
Actually `updateHighlightPath()` is a method that immediately changes the UI state (the variable `List<ListItem> highlightPath` is created during recursion and is just passed around without updates). I renamed it and added doc to clarify a bit more. Does it make sense?
nit: "both will run" (or "both will be run")
(if we keep this change)
oops, thanks!
boolean handled = false;
if (hasClickListener(item)) {
// The item had a click listener in the model, but none was bound by the
// ViewBinder, and there is no click delegate to use. In this case, call
// the model click listener directly.
item.model.get(CLICK_LISTENER).onClick(view);
handled = true;
}
if (mDelegate != null) {
// Delegate, if possible.
mDelegate.onItemSelected(item.model);
handled = true;
}
if (handled) {
return;
}
// As a fallback, use ListView's performItemClick.
long id = getItemId(position);
((ListView) parent).performItemClick(v, position, id);Jenna Himawan@jhim...@google.com This is the part I'm unsure about - do you think it sometimes makes sense to run both the delegate and the click listener?
We haven't had a scenario so far where this is true. Do you need it for your changes?
The reason why we need this is because:
- BasicListMenu always sets a delegate but
- ListMenuUtils sets a CLICK_LISTENER for submenus
and we need both of them to be called, unless we change either's implementation.
I considered having the delegate call the listener but the delegate doesn't have access to the `View`.
I wasn't sure if you were comfortable with calling both though.
You can handle this in a follow-up, but pressing the keyboard right arrow should also enter the flyout.
I'm planning on doing this in another CL.
It seems that the "handleEnterEvent" is activated on click. This makes sense, but you should probably rename the method so that its name is compatible with any user input that should trigger the flyout right away (including click, right arrow, space, and enter)
- Jenna Himawan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Jenna Himawan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 2 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
2 comments
org.chromium.ui.listmenu.ListMenuItemProperties.KEY_LISTENER,qq: can `ListMenuSubmenuHeaderItemProperties.KEY_LISTENER` be replaced with `ListMenuItemProperties.KEY_LISTENER`?
flyoutController.handleEnterEvent(Masa FujitaYou can handle this in a follow-up, but pressing the keyboard right arrow should also enter the flyout.
I'm planning on doing this in another CL.
On a second thought, I decided to do that here too.
Related details
- Jenna Himawan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jenna Himawan (Gerrit)
Jenna Himawan added 2 comments![]( "Open in Gerrit")
updateHighlightPath(highlightPath);Masa FujitaI'm a bit confused on why we're calling this here. Can you provide better documentation about what the method does?
I read the code a bit more. My understanding is that the `List<ListItem> highlightPath` is updated, but updateHighlightPath is not called right away. We update the highlight path, then pass the updated highlight path to another method, and expect the other method to updateHighlightPath using the parameter.
Could we not just updateHighlightPath right away?
Actually `updateHighlightPath()` is a method that immediately changes the UI state (the variable `List<ListItem> highlightPath` is created during recursion and is just passed around without updates). I renamed it and added doc to clarify a bit more. Does it make sense?
I think this helps make it clearer. It seems a bit asymmetric, though...
When we enter the flyout, we update List<ListItem> highlightPath outside the enterFlyout method, then the first thing the enterFlyout method does is update the highlights using the highlightPath that it is passed.
When we exit the flyout, we call updateHighlights with highlightPath.subList(...).
Would it make sense to have the enterFlyout method add `item` to the end of the list and then call updateHighlights?
You can do this in a separate CL; sorry for bothering you!
Please don't make this change; just use this approach: https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabOverflowMenuCoordinator.java;l=153;drc=eca209c559383ae76c131b640d242a5c648b5a2b
LMK if you have any more questions!
Related details
- Masa Fujita
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |