Closed Bug 1430028 Opened 6 years ago Closed 5 years ago

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

Categories

(Toolkit :: UI Widgets, task, P2)

task

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: ntim, Unassigned)

References

Details

Attachments

(2 obsolete files)

Both of these don't seem used and could possibly be removed.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
"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.
Depends on: 1434860
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?
Flags: needinfo?(bgrinstead)
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.
(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.
Flags: needinfo?(bgrinstead) → needinfo?(enndeakin)
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?
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
(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?
Flags: needinfo?(emilio)
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(emilio)
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
(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.
Flags: needinfo?(bgrinstead)
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"]
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.
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
Flags: needinfo?(emilio)
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)
(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.
Depends on: 1451406
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.
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
(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
See Also: → 1446961
(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.
Flags: needinfo?(emilio)
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.
Depends on: 1451691
Attachment #8960218 - Attachment is obsolete: true
Attachment #8960219 - Attachment is obsolete: true
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.
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Summary: Remove support for toolbarbutton[type=panel] and toolbarbutton[type=menu].badged-button → Remove the specific bindings for toolbar buttons with type="menu"
(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.
Flags: needinfo?(enndeakin)
Type: enhancement → task

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

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.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
No longer blocks: war-on-xbl
You need to log in before you can comment on or make changes to this bug.