Convert menu element structure from command=toggle-menu to DOM nesting. [chromium/src : main]
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
Requesting reviewers for the following files:
- @dgr...@chromium.org for `content/test/data/accessibility/` and `third_party/blink/renderer/modules/accessibility/`
- @jar...@chromium.org for everything else
Related details
- David Grogan
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
HTMLMenuItemElement* GetMenuItem() const;nit: Blink Style Guide: Precede setters with the word “Set”; use bare words for getters. Consider renaming `GetMenuItem` to `MenuItem` and `GetMenuList` to `MenuList` as there is no naming conflict with the return types.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- David Grogan
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Grogan (Gerrit)
David Grogan voted and added 5 comments![]( "Open in Gerrit")
Votes added by David Grogan
| Code-Review | +1 |
5 comments
I'm pleasantly surprised by how well our existing accessibility code did with the new structure. I _think_ the only required addition was what you added here, and we don't have to do anything else for <submenu>, but I'll check that with ARIA after we get <submenu> approved in whatwg.
<button id="button" popovertarget="menu">Show Menu</button>Just for my own understanding: why did this menulist get put under a menubar? IIUC, this file could have gone unchanged in this CL? Or can authors no longer do <button popovertarget="menu"> in this new world?
I mean, it's no matter whatsoever because this file is going to get refactored with the upcoming type=radio change. Just for my own understanding.
<menuitem disabled>Orphan Disabled</menuitem>Just for diff cleanliness, can you put this back above `menubar`, assuming that's an equally valid place?
<button id="btn" commandfor="menu" command="toggle-menu">Implicit Button Name</button>Similar question about removing button in favor of menubar/submenu, except here we'd have to change `toggle-menu` obviously. (Don't worry about answering here also if the previous answer applies)
@BLINK-ALLOW:haspopup*Thanks for adding these. Did you prompt jetski to add more of these if they were missing or did you do it manually?
Related details
- David Baron
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 6 comments![]( "Open in Gerrit")
I think a sample before and after of the html in the commit message would be nice
++++++ROLE_SYSTEM_MENUITEM name='First item' EXPANDED FOCUSABLE HASPOPUP haspopup:menu
++++++ROLE_SYSTEM_MENUPOPUP ispopup:autoI can see that the menuitem and the menupopup here are associated, but based on advice from james teh I think that we should consider trying to automatically name the menupopup based on the name of the menuitem.
I don't actually understand the UX consequences of how this would impact any ATs, and I wish I did, but I'm pretty sure James said we should do this for "breadcrumbs".
Maybe we should file a bug for this?
<button id="button" popovertarget="menu">Show Menu</button>is it not possible to invoke a menu with a button anymore? that doesn't seem good, im pretty sure that scott ohara endorsed this pattern.
if this is just to make the a11y output look more similar or something and the pattern is still supported, then im fine with that
bool result = HTMLElement::HandleCommandInternal(invoker, command);
if (result && popoverOpen()) {
if (LocalFrame* frame = GetDocument().GetFrame()) {
if (frame->GetEventHandler().IsHandlingKeyEvent()) {
FocusFirstItem();
}
}
}Should this be looking for a ToggleMenu command or something? Which commands should result in the first menuitem in this list being focused?
menubar:where(:-internal-has-open-menuitem) menuitem {why does this use :where()? can you add a comment explaining?
interest-delay-start: normal;what is this for?
Related details
- David Baron
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
++++++ROLE_SYSTEM_MENUITEM name='First item' EXPANDED FOCUSABLE HASPOPUP haspopup:menu
++++++ROLE_SYSTEM_MENUPOPUP ispopup:autoI can see that the menuitem and the menupopup here are associated, but based on advice from james teh I think that we should consider trying to automatically name the menupopup based on the name of the menuitem.
I don't actually understand the UX consequences of how this would impact any ATs, and I wish I did, but I'm pretty sure James said we should do this for "breadcrumbs".
Maybe we should file a bug for this?
- David Baron
- Joey Arhar
David Baron (Gerrit)
David Baron added 11 comments![]( "Open in Gerrit")
David BaronI think a sample before and after of the html in the commit message would be nice
Done
++++++ROLE_SYSTEM_MENUITEM name='First item' EXPANDED FOCUSABLE HASPOPUP haspopup:menu
++++++ROLE_SYSTEM_MENUPOPUP ispopup:autoI can see that the menuitem and the menupopup here are associated, but based on advice from james teh I think that we should consider trying to automatically name the menupopup based on the name of the menuitem.
I don't actually understand the UX consequences of how this would impact any ATs, and I wish I did, but I'm pretty sure James said we should do this for "breadcrumbs".
Maybe we should file a bug for this?
Curious if @dgr...@chromium.org has thoughts but I'll file a bug shortly.... oh wait, he said he'll file a bug!
<button id="button" popovertarget="menu">Show Menu</button>Just for my own understanding: why did this menulist get put under a menubar? IIUC, this file could have gone unchanged in this CL? Or can authors no longer do <button popovertarget="menu"> in this new world?
I mean, it's no matter whatsoever because this file is going to get refactored with the upcoming type=radio change. Just for my own understanding.
As I said to Joey, I've gone back to more minimal changes for this set of tests.
<button id="button" popovertarget="menu">Show Menu</button>is it not possible to invoke a menu with a button anymore? that doesn't seem good, im pretty sure that scott ohara endorsed this pattern.
if this is just to make the a11y output look more similar or something and the pattern is still supported, then im fine with that
I was originally thinking it wouldn't be allowed, but it still is. I've rolled back these test changes in favor of the minimal changes that are needed (plus things like removing now-unneeded ids).
**NOTE**: probably some of the non-linux expectations need further adjustments that I won't be able to make until I get the CQ+1 results. Right now they're AI-generated test expectation updates.
Just for diff cleanliness, can you put this back above `menubar`, assuming that's an equally valid place?
Done
<button id="btn" commandfor="menu" command="toggle-menu">Implicit Button Name</button>Similar question about removing button in favor of menubar/submenu, except here we'd have to change `toggle-menu` obviously. (Don't worry about answering here also if the previous answer applies)
Done
Thanks for adding these. Did you prompt jetski to add more of these if they were missing or did you do it manually?
In most cases I think jetski added them on its own initiative and I thought it seemed reasonable, but there were a few that I added myself manually.
bool result = HTMLElement::HandleCommandInternal(invoker, command);
if (result && popoverOpen()) {
if (LocalFrame* frame = GetDocument().GetFrame()) {
if (frame->GetEventHandler().IsHandlingKeyEvent()) {
FocusFirstItem();
}
}
}Should this be looking for a ToggleMenu command or something? Which commands should result in the first menuitem in this list being focused?
oops, I lost the command check. Restored. It should be looking for toggle-popover or show-popover now, and I think it's only relevant to button activation now.
nit: Blink Style Guide: Precede setters with the word “Set”; use bare words for getters. Consider renaming `GetMenuItem` to `MenuItem` and `GetMenuList` to `MenuList` as there is no naming conflict with the return types.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
why does this use :where()? can you add a comment explaining?
I added a comment.
interest-delay-start: normal;David Baronwhat is this for?
This is from the fix for https://crrev.com/c/7633506 . It overrides the special-for-menubar declarations above and goes back to the normal behavior. (The comment I just added above hopefully helps?)
Related details
- David Grogan
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Code-Review+1![]( "Open in Gerrit")
- David Baron
- David Grogan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
<button id="button" popovertarget="menu">Show Menu</button>David Baronis it not possible to invoke a menu with a button anymore? that doesn't seem good, im pretty sure that scott ohara endorsed this pattern.
if this is just to make the a11y output look more similar or something and the pattern is still supported, then im fine with that
I was originally thinking it wouldn't be allowed, but it still is. I've rolled back these test changes in favor of the minimal changes that are needed (plus things like removing now-unneeded ids).
**NOTE**: probably some of the non-linux expectations need further adjustments that I won't be able to make until I get the CQ+1 results. Right now they're AI-generated test expectation updates.
- David Grogan
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
Also, I should note that both of your Code-Review+1 stamps got reset due to set-of-files changes.
David Grogan (Gerrit)
David Grogan voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Grogan
| Code-Review | +1 |
1 comment
++++++ROLE_SYSTEM_MENUITEM name='First item' EXPANDED FOCUSABLE HASPOPUP haspopup:menu
++++++ROLE_SYSTEM_MENUPOPUP ispopup:autoDavid BaronI can see that the menuitem and the menupopup here are associated, but based on advice from james teh I think that we should consider trying to automatically name the menupopup based on the name of the menuitem.
I don't actually understand the UX consequences of how this would impact any ATs, and I wish I did, but I'm pretty sure James said we should do this for "breadcrumbs".
Maybe we should file a bug for this?
Curious if @dgr...@chromium.org has thoughts but I'll file a bug shortly.... oh wait, he said he'll file a bug!
Yes, will. Haven't yet though. Resolving so this is submittable.
Related details
- David Baron
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- David Baron
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Commit-Queue | +2 |
Requesting reviewers for the following files:
- @dgr...@chromium.org for `content/test/data/accessibility/` and `third_party/blink/renderer/modules/accessibility/`
- @jar...@chromium.org for everything else
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
Convert menu element structure from command=toggle-menu to DOM nesting.
This makes the changes discussed in
https://github.com/openui/open-ui/issues/1456 and elsewhere to convert
the nesting structure for menu elements from using command=toggle-menu
on menus to represent submenu relationships to using DOM nesting to
represent submenus.
This introduces a <submenu> element that can be used where a <menuitem>
would be, containing two children, the <menuitem> and the associated
<menulist>. It removes the toggle-menu command, but still allows
<menulist> elements (which are implicitly popovers) that are not inside
a <submenu> to be invoked with a toggle-popover command or the
popovertarget attribute.
This the main change is converting markup that looks like this:
<menubar>
<menuitem command=toggle-menu commandfor=file-menu>File</menu>
</menubar>
<menulist id=file-menu>
<menuitem>New</menu>
<menuitem>Open</menu>
</menulist>
into this:
<menubar>
<submenu>
<menuitem>File</menuitem>
<menulist>
<menuitem>New</menu>
<menuitem>Open</menu>
</menulist>
</submenu>
</menubar>
The other common pattern that is still permitted (except that it was
previously using the toggle-menu command instead of toggle-popover) is:
<button command=toggle-popover commandfor=actions-menu>Actions</button>
<menulist id=actions-menu>
<menuitem>Go</menuitem>
</menulist>
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M content/test/data/accessibility/event/menulist-invoker-name-change.html
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-android-external.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-android.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-auralinux.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-blink.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-mac.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-uia-win.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus-expected-win.txt
- M content/test/data/accessibility/html/commandfor-api-popover-menus.html
- M content/test/data/accessibility/html/menulist-aria-label-overrides-implicit.html
- M content/test/data/accessibility/html/menulist-aria-labelledby-overrides-label.html
- M content/test/data/accessibility/html/menulist-invoked-by-button-expected-blink.txt
- M content/test/data/accessibility/html/menulist-invoked-by-button-expected-mac.txt
- M content/test/data/accessibility/html/menulist-invoked-by-button.html
- M content/test/data/accessibility/html/menulist-invoked-by-menuitem-expected-auralinux.txt
- M content/test/data/accessibility/html/menulist-invoked-by-menuitem-expected-blink.txt
- M content/test/data/accessibility/html/menulist-invoked-by-menuitem-expected-mac.txt
- M content/test/data/accessibility/html/menulist-invoked-by-menuitem.html
- M content/test/data/accessibility/html/menulist-invoker-haspopup-expected-blink.txt
- M content/test/data/accessibility/html/menulist-invoker-haspopup.html
- M content/test/data/accessibility/html/menulist-multiple-invokers.html
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/bindings/idl_in_core.gni
- M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
- M third_party/blink/renderer/core/dom/element.h
- M third_party/blink/renderer/core/html/build.gni
- 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/renderer/core/html/html_menu_item_element.h
- M third_party/blink/renderer/core/html/html_menu_list_element.cc
- M third_party/blink/renderer/core/html/html_menu_owner_element.cc
- M third_party/blink/renderer/core/html/html_menu_owner_element.h
- A third_party/blink/renderer/core/html/html_sub_menu_element.cc
- A third_party/blink/renderer/core/html/html_sub_menu_element.h
- A third_party/blink/renderer/core/html/html_sub_menu_element.idl
- M third_party/blink/renderer/core/html/html_tag_names.json5
- M third_party/blink/renderer/core/html/resources/html.css
- M third_party/blink/renderer/modules/accessibility/ax_node_object.cc
- M third_party/blink/renderer/modules/accessibility/ax_object.cc
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/button-invoke-menulist.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/focus-enters-menulist.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/focus-menu-elements-arrowoperations.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/focus-menu-elements-nested-arrowoperations.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/long-menulist-appearance.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menu-elements-pageup-pagedown.optional.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menu-elements-pseudo-ordering.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menu-elements-tab-focus.optional.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menu-safe-triangle.optional.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menubar-invoke-menulist.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menuitem-activate.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menuitem-anchor-positioning.html
- M third_party/blink/web_tests/external/wpt/html/semantics/menu/tentative/menuitem-implicit-interest-invoker.tentative.html
- M third_party/blink/web_tests/webexposed/element-instance-property-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Joey Arhar, +1 by David Grogan
| 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/60939
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |