[menu] Activation behavior for open/close menu on arrow navigation [chromium/src : main]
Dominic Farolino (Gerrit)
Dominic Farolino added 8 comments![]( "Open in Gerrit")
Some comments for now that likely apply elsewhere, but I will do a more full pass tomorrow morning too.
This patch update the HTMLMenuItemElement::DefaultEventHandler method"updates"
ancestor menubar.And do nothing if there is no menubar ancestor?
- Close the current menulist.This feels ambiguous? What is the "current" menulist? There are two I can think of:
1. The menulist that this menuitem controls (aka the sub-menulist associated with *this* menuitem)
2. The menulist that this menuitem is a child of
- If element is a menuitem in a menubar,Else if?
invoked_menulist->InvokePopover(*this);This runs script, right? Can you add a test that exercises what happens if the popover is closed here while script runs? That should help inform us as to how we should proceed from here. Can `invoked_menulist` be closed at this point?
*invoked_menuitems.begin(), /*inclusive*/ true)) {style guide recommends `/*inclusive=*/true`
HTMLMenuItemElement* invoker_menuitem;Since this is a pointer, shouldn't we initialize this to non-garbage memory?
Related details
- Di Zhang
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 17 comments![]( "Open in Gerrit")
- Else, close all popovers. If it has an ancestor menubar and itsWhy do we close all popovers in this case? At least the Google docs menu opens the next menuitem in the menubar in this case.
kMenuItem,I think that (at least) the code right here needs to be modified to take this into account. The `is_popover` variable should apply to menus also, based on the usage here.
And it would be very good to add a test of that. You'd need to nest a `<menuitem>` inside a `<div popover>` and have it invoke a `<menulist>`, and make sure all of the popover light dismiss stuff still works correctly. Perhaps also nest a `<button popovertarget=foo>` inside the `<menulist>` and have *that* invoke a separate `<div popover id=foo>`? Just to test out that we covered all of the nesting cases, and menu items really **are** popovers?
kMenuItem,Should this be "MenuItem" or "MenuList"? The thing in the top layer is the `<menulist>` element, no?
/*include_event_handler_text=*/true, &document) &&include_event_handler_text should be `false`, since the `exception_state` is null.
invoked_menulist->InvokePopover(*this);This runs script, right? Can you add a test that exercises what happens if the popover is closed here while script runs? That should help inform us as to how we should proceed from here. Can `invoked_menulist` be closed at this point?
+1 - yes the popover could be closed in this step. E.g.
```
menulist.addEventListener('beforetoggle',() => menulist.hidePopover());
```
// Else, this does not invoke a menulist and we close all popovers.nit: `this menuitem does not...`
invoker_menuitem = DynamicTo<HTMLMenuItemElement>(This will be nullptr if there's an ancestor (non-menu) popover, which I think will lead to all of it being closed, right? That seems wrong.
ancestor_popover, document, HidePopoverFocusBehavior::kNone,Won't this always be `nullptr`?
if (auto* previous = ancestor_menuitems.NextFocusableMenuItem(`next` ?
DCHECK(menulist->popoverOpen());I think you could hit this DCHECK:
```
<menulist id=foo tabindex=0>
<style>
#foo {display:block}
</style>
```
...then focus the menulist and hit the left arrow
/*include_event_handler_text=*/true, &GetDocument());ditto
invoker, HidePopoverFocusBehavior::kFocusPreviousElement,Do you want it to focus the previous element in this case? Seems like no, since you're handling focus below.
event.SetDefaultHandled();Should we set default handled if neither branch above got hit? I.e. what if it was just a `<menuitem>` outside a menulist or menubar?
// TODO: implement scrolling to visible menuitem, forCan you add a bug link
/*include_event_handler_text=*/true, &GetDocument()) &&ditto
!invoked_menulist->popoverOpen();I don't think you need to check for !popoverOpen here - `IsPopoverReady` should do that for you already.
await test_driver.send_keys(document.activeElement, Tab);It might be a good idea to focus a starting element at the start of this test, so that you're starting from a known state.
Related details
- Di Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang added 8 comments![]( "Open in Gerrit")
This patch update the HTMLMenuItemElement::DefaultEventHandler methodDi Zhang"updates"
Done
- Else, close all popovers. If it has an ancestor menubar and itsWhy do we close all popovers in this case? At least the Google docs menu opens the next menuitem in the menubar in this case.
I think it is a similar pattern. It closes all currently opened popovers and focus on the next invoker, if available. The difference is that when it focuses on an invoker menuitem, it automatically opens it. In the current implementation, we need a DOM Activation to open a popover/menulist. Maybe that needs to be changed.
And do nothing if there is no menubar ancestor?
Right, I will add that.
This feels ambiguous? What is the "current" menulist? There are two I can think of:
1. The menulist that this menuitem controls (aka the sub-menulist associated with *this* menuitem)
2. The menulist that this menuitem is a child of
Second option.
- If element is a menuitem in a menubar,Di ZhangElse if?
Done
invoked_menulist->InvokePopover(*this);This runs script, right? Can you add a test that exercises what happens if the popover is closed here while script runs? That should help inform us as to how we should proceed from here. Can `invoked_menulist` be closed at this point?
If the popover is closed, `can_show` will return true and this line will run. But isn't that what the current test is testing (using ArrowRight to move activeElement to `h`)?
note: This is similar logic as what is used in `HTMLElement::HandleCommandInternal`, where it also calls `IsPopoverReady()` before calling either `HidePopoverInternal()` or `InvokePopover()`.
*invoked_menuitems.begin(), /*inclusive*/ true)) {Di Zhangstyle guide recommends `/*inclusive=*/true`
Done
HTMLMenuItemElement* invoker_menuitem;Since this is a pointer, shouldn't we initialize this to non-garbage memory?
I am not sure I understood, but I changed to initialize to nullptr.
I also changed to not enforce menuitem check yet, since it is possible the ancestor invoker is a button.
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 17 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
17 comments
- Else, close all popovers. If it has an ancestor menubar and itsDi ZhangWhy do we close all popovers in this case? At least the Google docs menu opens the next menuitem in the menubar in this case.
I think it is a similar pattern. It closes all currently opened popovers and focus on the next invoker, if available. The difference is that when it focuses on an invoker menuitem, it automatically opens it. In the current implementation, we need a DOM Activation to open a popover/menulist. Maybe that needs to be changed.
After offline chat, agree that there is a risk menus can be inside a popover and that popover shouldn't close. Updated the logic and added a test.
Please take another look at the close popovers logic, thank you.
kMenuItem,I think that (at least) the code right here needs to be modified to take this into account. The `is_popover` variable should apply to menus also, based on the usage here.
And it would be very good to add a test of that. You'd need to nest a `<menuitem>` inside a `<div popover>` and have it invoke a `<menulist>`, and make sure all of the popover light dismiss stuff still works correctly. Perhaps also nest a `<button popovertarget=foo>` inside the `<menulist>` and have *that* invoke a separate `<div popover id=foo>`? Just to test out that we covered all of the nesting cases, and menu items really **are** popovers?
Ok. Changed so `is_popover` includes the kMenuList case. Note I added it to bypass the `CHECK(top_layer_element_type != TopLayerElementType::kPopover);` in `TopLayerElementPopoverAncestor`, since previously, it is only called by non-popover elements.
Modified the test to include this case.
Perhaps also nest a <button popovertarget=foo> inside the <menulist> and have that invoke a separate <div popover id=foo>?
A button with popovertarget is not a valid invoker for a menulist and also won't follow the codepath added in this CL (since it only changes arrow key behavior for menuitem). I am not sure what to test here, except that popover works as usual.
kMenuItem,Should this be "MenuItem" or "MenuList"? The thing in the top layer is the `<menulist>` element, no?
Good point, changed.
/*include_event_handler_text=*/true, &document) &&include_event_handler_text should be `false`, since the `exception_state` is null.
Done
invoked_menulist->InvokePopover(*this);Di ZhangThis runs script, right? Can you add a test that exercises what happens if the popover is closed here while script runs? That should help inform us as to how we should proceed from here. Can `invoked_menulist` be closed at this point?
If the popover is closed, `can_show` will return true and this line will run. But isn't that what the current test is testing (using ArrowRight to move activeElement to `h`)?
note: This is similar logic as what is used in `HTMLElement::HandleCommandInternal`, where it also calls `IsPopoverReady()` before calling either `HidePopoverInternal()` or `InvokePopover()`.
Added a test where there is the event listener for before toggle. The code still runs, but will have popover warnings like "VM26:2 The `beforetoggle` event handler for a popover triggered another popover to be shown or hidden. Or a `loseinterest` event handler was cancelled. This is not recommended.".
// Else, this does not invoke a menulist and we close all popovers.nit: `this menuitem does not...`
Done
invoker_menuitem = DynamicTo<HTMLMenuItemElement>(This will be nullptr if there's an ancestor (non-menu) popover, which I think will lead to all of it being closed, right? That seems wrong.
I changed the logic to not have the DynamicTo here and instead just track the invoker. If an invoker is found, then check whether it is a menuitem in a menubar (to use the MenuItemList to find next/previous). If not, focus on invoker.
ancestor_popover, document, HidePopoverFocusBehavior::kNone,Won't this always be `nullptr`?
Right. I can change to that to be clearer.
if (auto* previous = ancestor_menuitems.NextFocusableMenuItem(Di Zhang`next` ?
Done
DCHECK(menulist->popoverOpen());I think you could hit this DCHECK:
```
<menulist id=foo tabindex=0>
<style>
#foo {display:block}
</style>
```...then focus the menulist and hit the left arrow
You are right. It gets hit. I removed that DCHECK and added the test.
/*include_event_handler_text=*/true, &GetDocument());Di Zhangditto
Done
invoker, HidePopoverFocusBehavior::kFocusPreviousElement,Do you want it to focus the previous element in this case? Seems like no, since you're handling focus below.
Done
event.SetDefaultHandled();Should we set default handled if neither branch above got hit? I.e. what if it was just a `<menuitem>` outside a menulist or menubar?
Changed to handle event by focusing on invoker in this case. This way, it will behave the same way as if the invoker is a button.
// TODO: implement scrolling to visible menuitem, forCan you add a bug link
Yes, also added one for each TODO for tracking purposes.
/*include_event_handler_text=*/true, &GetDocument()) &&Di Zhangditto
Done
!invoked_menulist->popoverOpen();I don't think you need to check for !popoverOpen here - `IsPopoverReady` should do that for you already.
Done
await test_driver.send_keys(document.activeElement, Tab);It might be a good idea to focus a starting element at the start of this test, so that you're starting from a known state.
Done
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
Note these changes are for arrow navigation starting from a menuitem. A follow-up patch for menulist invoked from button might be
necessary.nit: can you line wrap this at 72 chars?
kMenuList,Per our offline conversation, I thought the plan was to remove this entirely now?
Related details
- Di Zhang
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang added 2 comments![]( "Open in Gerrit")
Note these changes are for arrow navigation starting from a menuitem. A follow-up patch for menulist invoked from button might be
necessary.nit: can you line wrap this at 72 chars?
Done
kMenuList,Per our offline conversation, I thought the plan was to remove this entirely now?
Yes, but using only kPopover fails two CHECKs so it felt:
1. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=2167;drc=4388d13dc2a92e9123085050cf95db052cba8ab3
2. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=2224;drc=4388d13dc2a92e9123085050cf95db052cba8ab3
So it felt still necessary to have this enum distinction.
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
I still think it'd be better not to add a value to the enum for menus, since they're just popovers really.
- Else, close all popovers. If it has an ancestor menubar and itsDi ZhangWhy do we close all popovers in this case? At least the Google docs menu opens the next menuitem in the menubar in this case.
Di ZhangI think it is a similar pattern. It closes all currently opened popovers and focus on the next invoker, if available. The difference is that when it focuses on an invoker menuitem, it automatically opens it. In the current implementation, we need a DOM Activation to open a popover/menulist. Maybe that needs to be changed.
After offline chat, agree that there is a risk menus can be inside a popover and that popover shouldn't close. Updated the logic and added a test.
Please take another look at the close popovers logic, thank you.
The new test looks good.
kMenuList,Di ZhangPer our offline conversation, I thought the plan was to remove this entirely now?
Yes, but using only kPopover fails two CHECKs so it felt:
1. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=2167;drc=4388d13dc2a92e9123085050cf95db052cba8ab3
2. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=2224;drc=4388d13dc2a92e9123085050cf95db052cba8ab3So it felt still necessary to have this enum distinction.
I think maybe those two CHECKs should be adjusted to know about menus, perhaps? I.e. in both cases, it's "ok" if the provided popover is also a `<menulist>`.
Related details
- Di Zhang
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
2 comments
kMenuList,Di ZhangPer our offline conversation, I thought the plan was to remove this entirely now?
Mason FreedYes, but using only kPopover fails two CHECKs so it felt:
1. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=2167;drc=4388d13dc2a92e9123085050cf95db052cba8ab3
2. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=2224;drc=4388d13dc2a92e9123085050cf95db052cba8ab3So it felt still necessary to have this enum distinction.
I think maybe those two CHECKs should be adjusted to know about menus, perhaps? I.e. in both cases, it's "ok" if the provided popover is also a `<menulist>`.
Done
HTMLMenuItemElement* invoker_menuitem;Di ZhangSince this is a pointer, shouldn't we initialize this to non-garbage memory?
I am not sure I understood, but I changed to initialize to nullptr.
I also changed to not enforce menuitem check yet, since it is possible the ancestor invoker is a button.
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
Element* new_popovers_invoker =Can you change the comment to:
```
// If top_layer_element is an open menulist popover, find its invoker.
// Since "normal" popovers don't go through this code path (see the
// CHECK above), pass `nullptr` otherwise.
```
Related details
- Di Zhang
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
1 comment
Can you change the comment to:
```
// If top_layer_element is an open menulist popover, find its invoker.
// Since "normal" popovers don't go through this code path (see the
// CHECK above), pass `nullptr` otherwise.
```
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Code-Review+1![]( "Open in Gerrit")
- Di Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[menu] Activation behavior for open/close menu on arrow navigation
This patch updates the HTMLMenuItemElement::DefaultEventHandler method
to handle invoking menulist on arrow key navigations. We follow the
suggestions as outlined by APG for menubar [1].
- If element is a menuitem in a menulist,
- If arrow RIGHT,
- If menuitem invokes a sub-menulist,
- If sub-menulist is closed, show the sub-menulist.
- Focus on its first child.
- Else, close all popovers. If it has an ancestor menubar and its
ancestor invoker has a next menuitem, focus on that next menuitem.
- Else if arrow LEFT,
- Close the menulist that this menuitem is a child of.
- If its invoker is in a menulist, focus on this invoker.
- If its invoker is in a menubar, focus on the previous menuitem.
- Else if element is a menuitem in a menubar,
- If element invokes a menulist,
- If arrow UP, focus the last menuitem in the menulist.
- Else if arrow DOWN, focus the first menuitem in the menulist.
Note these changes are for arrow navigation starting from a menuitem. A
follow-up patch for menulist invoked from button might be necessary.
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/renderer/core/html/html_menu_item_element.cc
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/focus-menu-elements-arrowoperations.html
- A third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/focus-menu-elements-nested-arrowoperations.html
Code-Review: +1 by Mason Freed, +1 by Dominic Farolino
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53363
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |