1430028 - Remove the specific bindings for toolbar buttons with type="menu"

Status: RESOLVED WORKSFORME

(Toolkit :: XUL Widgets, task, P2)

Description

[Tim Nguyen :ntim]

Both of these don't seem used and could possibly be removed.

Comment 1

[:Paolo Amadini]

"toolbarbutton[type=menu].badged-button" is actually used once in the search bar popup. I'm however looking into simplifying the handling of the menu buttons for both badged and non-badged after bug 1434860 lands.

Comment 2

[:Paolo Amadini]

Attachment: Bug 1430028 - Part 1 - Always use type="menu" instead of the type="panel" alias.

Review commit: https://reviewboard.mozilla.org/r/228974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228974/

Comment 3

[:Paolo Amadini]

Attachment: Bug 1430028 - Part 2 - Simplify toolbar buttons of the menu type.

Review commit: https://reviewboard.mozilla.org/r/228976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228976/

Comment 4

[:Paolo Amadini]

After part 2, the bindings only exist to set display="xul:menu", which I think is used to create the MenuBoxObject that handles displaying the menu. Do you have any thoughts on how to migrate away from this?

Comment 5

[:Paolo Amadini]

Also, we should do some Talos runs for part 2, but the extra elements hopefully won't show up in profiles, as we already have a number of unneeded elements that we create for each button.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #4)
> After part 2, the bindings only exist to set display="xul:menu", which I
> think is used to create the MenuBoxObject that handles displaying the menu.
> Do you have any thoughts on how to migrate away from this?

Forwarding the question to Neil, but I suspect we could do a check on tag name + attribute somewhere instead.

Comment 7

[:Paolo Amadini]

I stated some Talos builds for comparing the performance after applying part 2:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef4bd5f02590ee5a33cebfa23b2a80506cfb2061
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2a577ca8305a18a5beec49893b084666ee6d83

Comment 8

[:Paolo Amadini]

Most Talos results look alright, but there are two that stand out:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ef4bd5f02590ee5a33cebfa23b2a80506cfb2061&newProject=try&newRevision=3a2a577ca8305a18a5beec49893b084666ee6d83&framework=1&showOnlyConfident=1

Brian, you've probably looked at more Talos results than me recently. Are these regressions or expected noise? I've run six rebuilds, do we need more or would they be noisy anyways?

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #8)
> Most Talos results look alright, but there are two that stand out:
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=ef4bd5f02590ee5a33cebfa23b2a8050
> 6cfb2061&newProject=try&newRevision=3a2a577ca8305a18a5beec49893b084666ee6d83&
> framework=1&showOnlyConfident=1
> 
> Brian, you've probably looked at more Talos results than me recently. Are
> these regressions or expected noise? I've run six rebuilds, do we need more
> or would they be noisy anyways?

Those aren't the typical ones I see a lot of noise in (although it's interesting this is only on Win64). 6 rebuilds is usually significant, but since that's only a medium confidence I'm not sure. I notice looking at the last 14 days on those two tests that they may have dropped overall recently:

- https://treeherder.mozilla.org/perf.html#/graphs?series=try,1536965,1,1&highlightedRevisions=ef4bd5f02590&highlightedRevisions=3a2a577ca830
- https://treeherder.mozilla.org/perf.html#/graphs?series=try,1536954,1,1&highlightedRevisions=ef4bd5f02590&highlightedRevisions=3a2a577ca830

I think rather than triggering rebuilds I'd just pull latest m-c then do two new try pushes and see if the same tests regress.

Comment 10

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to :Paolo Amadini from comment #4)
> > After part 2, the bindings only exist to set display="xul:menu", which I
> > think is used to create the MenuBoxObject that handles displaying the menu.
> > Do you have any thoughts on how to migrate away from this?
> 
> Forwarding the question to Neil, but I suspect we could do a check on tag
> name + attribute somewhere instead.

It's still unclear to me how "extends" and "display" are supposed to work, and how they relate to each other. They both seem related to inheritance:

https://dxr.mozilla.org/mozilla-central/rev/a1fb8ffae378963b128deaaf3a76eff9dbb6be21/dom/xbl/nsXBLPrototypeBinding.cpp#1582-1659

I couldn't find any documentation either. Brian, can you clarify what the intent was? Do you think there could be another way to achieve the same behavior of opening the menu with different techniques?

Emilio, is the "display" attribute also related to the work in bug 1450652?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Paolo Amadini from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > (In reply to :Paolo Amadini from comment #4)
> > > After part 2, the bindings only exist to set display="xul:menu", which I
> > > think is used to create the MenuBoxObject that handles displaying the menu.
> > > Do you have any thoughts on how to migrate away from this?
> > 
> > Forwarding the question to Neil, but I suspect we could do a check on tag
> > name + attribute somewhere instead.
> 
> It's still unclear to me how "extends" and "display" are supposed to work,
> and how they relate to each other. They both seem related to inheritance:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a1fb8ffae378963b128deaaf3a76eff9dbb6be21/dom/xbl/nsXBLPrototypeBinding.
> cpp#1582-1659
> 
> I couldn't find any documentation either. Brian, can you clarify what the
> intent was? Do you think there could be another way to achieve the same
> behavior of opening the menu with different techniques?
> 
> Emilio, is the "display" attribute also related to the work in bug 1450652?

It is. So, extends="xul:foo" / display="xul:foo" is used as a way to override the way a XUL element node name for layout. If the remaining consumers of this binding always have the same nodename, you can add a line here:

  https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/layout/base/nsCSSFrameConstructor.cpp#4337

If there are multiple, the way to do it is adding a new `display` property value (`-moz-menu`?), and add the line here instead:

  https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/layout/base/nsCSSFrameConstructor.cpp#4495

But I'd try to stick with the first solution if possible.

Comment 12

[Brian Grinstead [:bgrins]]

I believe there's at least one another place we check the node name (in nsIDocument::GetBoxObjectFor): https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/base/nsDocument.cpp#6475-6476

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #10)
> It's still unclear to me how "extends" and "display" are supposed to work,
> and how they relate to each other. They both seem related to inheritance:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a1fb8ffae378963b128deaaf3a76eff9dbb6be21/dom/xbl/nsXBLPrototypeBinding.
> cpp#1582-1659
> 
> I couldn't find any documentation either. Brian, can you clarify what the
> intent was? Do you think there could be another way to achieve the same
> behavior of opening the menu with different techniques?
> 
> Emilio, is the "display" attribute also related to the work in bug 1450652?

We discussed this more at https://mozilla.logbot.info/content/20180404#c14560291. I do believe that burning down the list of display="xul:*" will be required to complete bug 1450652. Here are the remaining ones: https://searchfox.org/mozilla-central/search?q=display%3D%22xul%3A&path=xml.

Comment 14

[Brian Grinstead [:bgrins]]

As per https://bgrins.github.io/xbl-analysis/tree/#toolbarbutton-badged-menu and https://bgrins.github.io/xbl-analysis/tree/#menu unfortunately these don't map cleanly to a tag name:

Selectors for the `toolbarbutton-badged-menu` binding: toolbarbutton.badged-button[type="menu"], toolbarbutton.badged-button[type="panel"]

Selectors for the `menu` binding: menu (content/xul.css), toolbarbutton[type="menu"], toolbarbutton[type="panel"] (content/xul.css), button[type="menu"], button[type="panel"]

Comment 15

[Brian Grinstead [:bgrins]]

Emilio's second idea (adding a new `display` property value) seems like it'd work for the layout side for these complex selectors, but I'm not sure if that value would be easily accessible from the callers of GetBoxObjectFor. We should go through the entire list of display="xul:" and see how many times we have these more complex selectors vs only tag names.

Comment 16

[Brian Grinstead [:bgrins]]

Here are the bindings with display="xul:*" - generated from https://bgrins.github.io/xbl-analysis/tree/#display with this code in the webconsole: `[...document.querySelectorAll("details[display]")].map(d=> { return "* `" + d.querySelector("summary span").textContent + "` " + d.querySelector("[highlight=display]").textContent + " " + d.querySelectorAll(".metadata em")[1].textContent }).join("\n")`

* `toolbarbutton` display (xul:button) Selectors: toolbarbutton (content/xul.css)
* `menu` display (xul:menu) Selectors: menu (content/xul.css), toolbarbutton[type=\"menu\"], toolbarbutton[type=\"panel\"] (content/xul.css), button[type=\"menu\"], button[type=\"panel\"] (content/xul.css)
* `toolbarbutton-badged-menu` display (xul:menu) Selectors: toolbarbutton.badged-button[type=\"menu\"], toolbarbutton.badged-button[type=\"panel\"] (content/xul.css)
* `button` display (xul:button) Selectors: button (content/xul.css)
* `button-repeat` display (xul:autorepeatbutton) Selectors: button[type=\"repeat\"] (content/xul.css)
* `menu-button` display (xul:menu) Selectors: button[type=\"menu-button\"] (content/xul.css)
* `tab` display (xul:button) Selectors: tab (content/xul.css)
* `tabbrowser-tab` display (xul:hbox) Selectors: .tabbrowser-tab (content/browser.css)
* `menulist` display (xul:menu) Selectors: menulist (content/xul.css)
* `menulist-popuponly` display (xul:menu) Selectors: menulist[popuponly=\"true\"] (content/xul.css)
* `columnpicker` display (xul:button) Selectors: treecolpicker (content/xul.css)
* `colorpicker-button` display (xul:menu) Selectors: colorpicker[type=\"button\"] (content/xul.css)
* `listheader` display (xul:button) Selectors: listheader (content/xul.css)"

Emilio, do extended bindings inherit the node name set from `display` / `extends`? That is, if `toolbarbutton` binding has display="xul:button" then will `toolbarbutton-badged` also get it? https://bgrins.github.io/xbl-analysis/tree/#toolbarbutton

Comment 17

[Brian Grinstead [:bgrins]]

The only display="xul:*" values that need to worry about a box object are (taken from https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/base/nsDocument.cpp#6474-6490):

menu, popup, menupopup, panel, tooltip, tree, listbox, scrollbox

So the only bindings we need to worry about regarding box objects appear to only be ones with display="xul:menu":

* `menu` display (xul:menu)
* `toolbarbutton-badged-menu` display (xul:menu) 
* `menu-button` display (xul:menu)
* `menulist` display (xul:menu)
* `menulist-popuponly` display (xul:menu)
* `colorpicker-button` display (xul:menu)

Comment 18

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Selectors for the `toolbarbutton-badged-menu` binding:
> toolbarbutton.badged-button[type="menu"],
> toolbarbutton.badged-button[type="panel"]
> 
> Selectors for the `menu` binding: menu (content/xul.css),
> toolbarbutton[type="menu"], toolbarbutton[type="panel"] (content/xul.css),
> button[type="menu"], button[type="panel"]

In the meantime I'll move part 1 to another bug to remove the type="panel" alias sooner.

Comment 19

[Brian Grinstead [:bgrins]]

Do we ever actually use 'toolbarbutton-badged-menu'? Just scanning search results here I don't see any places where the selector `toolbarbutton.badged-button[type="menu"]` would apply: https://searchfox.org/mozilla-central/search?q=badged-button&path=. If we could remove that binding entirely it would simplify the `display` handling.

Comment 20

[:Paolo Amadini]

It's still used in the search bar when a site asks to register more than one search engine. Anyways, I don't think it would simplify things much, because we still have to check for type="menu" regardless of the "badged-button" class.

Comment 21

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1b4b2402139e8b7f2c1cb2484720eede2e360c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9ae64ebcfde3a2b4478d030739f4784132f06f

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> I believe there's at least one another place we check the node name (in
> nsIDocument::GetBoxObjectFor):
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/dom/base/nsDocument.cpp#6475-6476

I think Neil is looking into ways to get rid of this in Bug 1446961, at least for PopupBoxObject

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Emilio, do extended bindings inherit the node name set from `display` /
> `extends`? That is, if `toolbarbutton` binding has display="xul:button" then
> will `toolbarbutton-badged` also get it?
> https://bgrins.github.io/xbl-analysis/tree/#toolbarbutton

Huh, thought I had replied to this yesterday, sorry. Yes it does, it looks it up through the whole base binding chain.

Comment 24

[:Paolo Amadini]

The latest Talos run with 10 rebuilds shows that there is no regression, and Windows 64 is just very noisy.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5b1b4b2402139e8b7f2c1cb2484720eede2e360c&newProject=try&newRevision=bf9ae64ebcfde3a2b4478d030739f4784132f06f&framework=1&showOnlyNoise=1

I'll move part 2 to another bug and leave this bug open to deal with the "display" attribute, since there's more information here on the topic.

Comment 25

[:Paolo Amadini]

Changed the subject to reflect the work that remains to be done, and unassigned myself for now since this may be completed as part of the work Emilio is doing, or may depend on it.

Comment 26

[ExE Boss]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Emilio’s second idea (adding a new `display` property value) seems like it'd
> work for the layout side for these complex selectors, but I'm not sure if
> that value would be easily accessible from the callers of GetBoxObjectFor.

If we do that, then we’ll have to keep in mind bug 1288572.

Comment 27

[alexander :surkov (:asurkov)]

Is the bug still actual? At least it probably shouldn't block war-on-xbl bug any longer, since toolbarbuttons are not XBL widgets anymore

Comment 28

[Tim Nguyen :ntim]

The new CE is pretty simple compared to the bindings. We could potentially remove the dropmarker from the markup and reimplement it as styling in the consumers (https://searchfox.org/mozilla-central/search?q=toolbarbutton-menu-dropmarker&case=false&regexp=false&path= ), but it doesn't provide any significant benefit.