[menu]: Fix toggle-menu command with light dismiss [chromium/src : main]
Dominic Farolino (Gerrit)
Dominic Farolino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Mason Freed
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 added 1 comment![]( "Open in Gerrit")
? DynamicTo<HTMLElement>(menu_item_element->commandForElement())I think it is safer to use `DynamicTo<HTMLMenuListElement>(menu_item_element->commandForElement())`.
Related details
- Dominic Farolino
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
1 comment
? DynamicTo<HTMLElement>(menu_item_element->commandForElement())I think it is safer to use `DynamicTo<HTMLMenuListElement>(menu_item_element->commandForElement())`.
- Di Zhang
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Di Zhang
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted Code-Review+1![]( "Open in Gerrit")
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 5 comments![]( "Open in Gerrit")
Looks good, other than the target-is-menulist restriction question.
const HTMLMenuItemElement* menu_item_element =nit: `auto*` is easier to read here
const HTMLMenuItemElement* menu_item_element =nit: maybe just `menu_item`?
menu_item_element ? DynamicTo<HTMLMenuListElement>(I think it should likely be "legal" to do this:
```
<menuitem command=show-popover commandfor=foo></menuitem>
<div popover id=foo></div>
```
I.e. not sure why the target has to be a menulist?
If you agree with my comment above, it'd be good to add a test that shows you can do `command=show-popover` from a `<menuitem>`.
I also think, and I've mentioned this to Di (so double-check she hasn't already done it), that it'd be good to make a test where the entire `<menulist>` resides in an open popover. Menus should work just fine within a popover, and e.g. the outer popover shouldn't be inadvertently closed at any point.
Related details
- 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 1 comment![]( "Open in Gerrit")
menu_item_element ? DynamicTo<HTMLMenuListElement>(I think it should likely be "legal" to do this:
```
<menuitem command=show-popover commandfor=foo></menuitem>
<div popover id=foo></div>
```I.e. not sure why the target has to be a menulist?
I suggested this restriction. I thought about it more and agree with Mason, menuitem should support all valid commands.
Related details
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
3 comments
nit: `auto*` is easier to read here
I suppose. I just hate `auto`! Done, though.
nit: maybe just `menu_item`?
Done
Di ZhangI think it should likely be "legal" to do this:
```
<menuitem command=show-popover commandfor=foo></menuitem>
<div popover id=foo></div>
```I.e. not sure why the target has to be a menulist?
I suggested this restriction. I thought about it more and agree with Mason, menuitem should support all valid commands.
Yeah this seems like a good idea. In my head I wasn't considering the other commands. I'll loosen this and add a test.
Related details
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
1 comment
If you agree with my comment above, it'd be good to add a test that shows you can do `command=show-popover` from a `<menuitem>`.
I also think, and I've mentioned this to Di (so double-check she hasn't already done it), that it'd be good to make a test where the entire `<menulist>` resides in an open popover. Menus should work just fine within a popover, and e.g. the outer popover shouldn't be inadvertently closed at any point.
I'll add a TODO on my list to add this kind of test after this CL, if that'a alright with you.
Related details
- 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 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
const HTMLMenuItemElement* menu_item_element =Dominic Farolinonit: `auto*` is easier to read here
I suppose. I just hate `auto`! Done, though.
Why do you hate `auto`, out of curiosity? It can cause readability problems in some cases, but I don't think that's true when the line is something like:
`auto* foo = DynamicTo<SomeReallyGiantTypeNameThatWouldAppearTwice>(bar);`
Dominic FarolinoIf you agree with my comment above, it'd be good to add a test that shows you can do `command=show-popover` from a `<menuitem>`.
I also think, and I've mentioned this to Di (so double-check she hasn't already done it), that it'd be good to make a test where the entire `<menulist>` resides in an open popover. Menus should work just fine within a popover, and e.g. the outer popover shouldn't be inadvertently closed at any point.
I'll add a TODO on my list to add this kind of test after this CL, if that'a alright with you.
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +2 |
1 comment
const HTMLMenuItemElement* menu_item_element =Dominic Farolinonit: `auto*` is easier to read here
Mason FreedI suppose. I just hate `auto`! Done, though.
Why do you hate `auto`, out of curiosity? It can cause readability problems in some cases, but I don't think that's true when the line is something like:
`auto* foo = DynamicTo<SomeReallyGiantTypeNameThatWouldAppearTwice>(bar);`
Yeah in that example it makes sense. I hate it in the cases where it causes readability problems, and I just remain consistent with that at all times, even at the expense of verbosity. But I think my policy just doesn't make sense heh.
Related details
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]: Fix toggle-menu command with light dismiss
`NearestTargetPopoverForInvoker()` finds the nearest target popover for
a given popover invoker that is a form control or button element.
Before this CL, this algorithm didn't consider HTMLMenuItemElements as
possible popover invokers, meaning it never returned relevant opened
HTMLMenuListElement popovers.
This CL updates this algorithm to consider HTMLMenuItemElement as a
popover invoker, and return its associated target menulist as a
popover. This corrects `FindTopmostRelatedPopover()`, which makes light
dismiss for clicks aimed at <menuitem>s work the same as <button>s.
Concretely, this ensures we do not double-activate a menuitem when we
click it and its popover is open. This fixes the behavior of the
`toggle-menu` command, ensuring it works the same as `toggle-popover`
on buttons/traditional popovers.
R=masonf
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menubar-invoke-menulist.html
- A third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menuitem-activate.html
Code-Review: +1 by Di Zhang, +1 by Mason Freed
| 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/53352
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |