1473311 - [meta] Remove bindings for <toolbar> elements from browser's toolbar.xml

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P3)

Description

[:Gijs (he/him)]

Essentially, toolbars have the following logic done via XBL:

- constructor
  This we could run early on window open from somewhere window-JS-y. I don't think the current things we're doing with this (dragging isn't really specific to toolbars, overflowable toolbar stuff should live in CustomizableUI) need to be in a XBL binding
- insertItem/currentset should just be migrated into CustomizableUI now that we no longer support legacy add-ons
- autohide menubar isn't really a repeatable thing, so could also just live in window JS instead of being its own component.
- toolbox property can be deduced from DOM, and CUI can probably just hardcode the toolbox as window.gNavToolbox given it only works with browser windows.

So the proposed migration would look roughly like:

- the overflowedDuringConstruction field is already unused and can be removed.
- change anything that relies on the toolbarName property to just read/write the attribute.
- update any consumers of .toolbox to deduce the toolbox from DOM and/or use the cached gNavToolbox reference (could probably be a smart getter inside wherever it's defined now, if it isn't already). The "toolboxid" attribute is already dead at this point, judging from DXR/searchfox.
- add a CUI.getCustomizationTarget() helper and move the customizationTarget logic into there instead, updating tests as necessary. As far as I can tell this notion is completely contained to CUI code so there's no need for it to be a "public" property on either the binding or even, post-migration, on the public CUI object, except maybe for tests (who could get the internal object or use a helper function, really).
- remove currentset/insertItem, and convert any consumers inside CUI to just contain that logic themselves (or in a helper method). Then the 'base' binding in https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/toolbar.xml can already be removed, as well as the menubar-stub binding. Ensure that those toolbars don't get the toolkit/ toolbars applied, possibly by creating some very generic CSS in browser/base/content/browser.css to set toolbar { -moz-binding: none } or whatever.
- put the `inactive=true` attribute in markup for the autohide menubar (that's all the constructor does), and put all the other logic in a separate js file (maybe create browser-menubar.js or something?). Remove CSS that attaches that binding and the binding itself.
- move the window dragging code to the extant windowdragging jsm, and invoke it from browser.js and https://dxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js (possibly later than we do now, which should be fine and a minor perf win).
- move the toolbarpaletteitem binding into the toolkit generic bindings file, and remove the browser/ copy of toolbar.xml .

Comment 2

[alexander :surkov (:asurkov)]

(In reply to :Gijs (he/him) from comment #0)
> Essentially, toolbars have the following logic done via XBL:
> 
> - constructor
>   This we could run early on window open from somewhere window-JS-y. I don't
> think the current things we're doing with this (dragging isn't really
> specific to toolbars, overflowable toolbar stuff should live in
> CustomizableUI) need to be in a XBL binding

> - remove currentset/insertItem, and convert any consumers inside CUI to just
> contain that logic themselves (or in a helper method). Then the 'base'
> binding in
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/content/toolbar.xml can already be removed, as well as the
> menubar-stub binding. Ensure that those toolbars don't get the toolkit/
> toolbars applied, possibly by creating some very generic CSS in
> browser/base/content/browser.css to set toolbar { -moz-binding: none } or
> whatever.

any suggestions where to move code from constructors of toolbar XBL binding? and what to search for? all toolbar elements having @customizable="true"?

> - put the `inactive=true` attribute in markup for the autohide menubar
> (that's all the constructor does), and put all the other logic in a separate
> js file (maybe create browser-menubar.js or something?). Remove CSS that
> attaches that binding and the binding itself.
> - move the window dragging code to the extant windowdragging jsm, and invoke
> it from browser.js and
> https://dxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/places.js (possibly later than we do now, which should be fine and a
> minor perf win).

is it about toolbar-drag binding, right?

> - move the toolbarpaletteitem binding into the toolkit generic bindings
> file, and remove the browser/ copy of toolbar.xml .

Comment 3

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #2)
> (In reply to :Gijs (he/him) from comment #0)
> > Essentially, toolbars have the following logic done via XBL:
> > 
> > - constructor
> >   This we could run early on window open from somewhere window-JS-y. I don't
> > think the current things we're doing with this (dragging isn't really
> > specific to toolbars, overflowable toolbar stuff should live in
> > CustomizableUI) need to be in a XBL binding
> 
> > - remove currentset/insertItem, and convert any consumers inside CUI to just
> > contain that logic themselves (or in a helper method). Then the 'base'
> > binding in
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/
> > customizableui/content/toolbar.xml can already be removed, as well as the
> > menubar-stub binding. Ensure that those toolbars don't get the toolkit/
> > toolbars applied, possibly by creating some very generic CSS in
> > browser/base/content/browser.css to set toolbar { -moz-binding: none } or
> > whatever.
> 
> any suggestions where to move code from constructors of toolbar XBL binding?

The ones in customizableui/content/toolbar.xml should be movable to one of the browser-* scripts that seems appropriate and gets loaded in the window when the window opens. For now, perhaps the best would be https://searchfox.org/mozilla-central/source/browser/base/content/browser-customization.js . You'll need to update https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#110-111 to ensure the file gets loaded.

> and what to search for? all toolbar elements having @customizable="true"?

I don't understand this question. The constructors all live in the toolbar.xml file I just cited.

If you're asking which toolbars need to have the constructor's code run for them, then the logical way is to check what the binding gets applied to:

https://searchfox.org/mozilla-central/search?q=customizableui%2Ftoolbar

which finds you:

https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/browser/themes/linux/browser.css#479-488

and

https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/browser/base/content/browser.css#74

For the latter, it turns out that we don't set this dynamically except in tests (which might just need to be removed):

https://searchfox.org/mozilla-central/search?q=%22customizable%22&case=false&regexp=false&path=

so all that's really left is the browser.xul hits here:

https://searchfox.org/mozilla-central/search?q=customizable%3D&case=false&regexp=false&path=

There's also accessible/tests/mochitest/tree/test_formctrl.xul -- but I have no idea what that's doing and if it really needs a customizable toolbar - you probably know better than me.


So the short answer is just the main toolbars in browser.xul (though the menubar should only be run on windows/linux, not on mac)

The longer answer is that with xpcom add-ons gone, and it being impossible to add custom toolbars since Firefox 29, we don't really need to support any other toolbars, but there might still be tests for some of that stuff, which we should be judicious about but not too afraid to just remove if they're only testing obsolete functionality.

> > - move the window dragging code to the extant windowdragging jsm, and invoke
> > it from browser.js and
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/places/
> > content/places.js (possibly later than we do now, which should be fine and a
> > minor perf win).
> 
> is it about toolbar-drag binding, right?

Yes.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to :Gijs (he/him) from comment #3)

> > and what to search for? all toolbar elements having @customizable="true"?
> 
> I don't understand this question. The constructors all live in the
> toolbar.xml file I just cited.

I meant how to find a toolbar element in the document. For example, it could be document.querySelectorAll('[customizable]') and then run code from toolbar's constructor for each element in the list. Or, possibly there's only one element ID in the document that this code should be run for.

> https://searchfox.org/mozilla-central/
> search?q=customizable%3D&case=false&regexp=false&path=
> 
> There's also accessible/tests/mochitest/tree/test_formctrl.xul -- but I have
> no idea what that's doing and if it really needs a customizable toolbar -
> you probably know better than me.

I think this particular test should be removed (not a test file of course). The test was introduced when cutomziable toolbar had own binding, but since it will removed soon, the test doesn't make much sense.

Comment 5

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #4)
> (In reply to :Gijs (he/him) from comment #3)
> 
> > > and what to search for? all toolbar elements having @customizable="true"?
> > 
> > I don't understand this question. The constructors all live in the
> > toolbar.xml file I just cited.
> 
> I meant how to find a toolbar element in the document. For example, it could
> be document.querySelectorAll('[customizable]') and then run code from
> toolbar's constructor for each element in the list. Or, possibly there's
> only one element ID in the document that this code should be run for.

I filed bug 1505734 with an idea about how to do this.