Closed
Bug 1430028
Opened 7 years ago
Closed 5 years ago
Remove the specific bindings for toolbar buttons with type="menu"
Categories
(Toolkit :: XUL Widgets, task, P2)
Toolkit
XUL Widgets
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.
Updated•7 years ago
|
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
Comment 1•7 years ago
|
||
"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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
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)
Comment 5•7 years ago
|
||
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•7 years ago
|
||
(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)
Comment 7•7 years ago
|
||
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•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
(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)
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
(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)
Comment 12•7 years ago
|
||
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•7 years ago
|
||
(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)
Comment 14•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
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)
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1b4b2402139e8b7f2c1cb2484720eede2e360c https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9ae64ebcfde3a2b4478d030739f4784132f06f
Comment 22•7 years ago
|
||
(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•7 years ago
|
||
(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)
Comment 24•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8960218 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8960219 -
Attachment is obsolete: true
Comment 25•7 years ago
|
||
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"
Comment 26•7 years ago
|
||
(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.
Updated•6 years ago
|
Flags: needinfo?(enndeakin)
Reporter | ||
Updated•5 years ago
|
Type: enhancement → task
Comment 27•5 years ago
|
||
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
Reporter | ||
Comment 28•5 years ago
|
||
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®exp=false&path= ), but it doesn't provide any significant benefit.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Updated•5 years ago
|
No longer blocks: war-on-xbl
You need to log in
before you can comment on or make changes to this bug.
Description
•