1498740 - Remove the toolkit toolbar-menubar-autohide binding

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P4)

Description

[Brian Grinstead [:bgrins]]

I think this can be done with customized built-ins like we did with the print preview toolbar (bug 1450813).

More details about the binding at https://bgrins.github.io/xbl-analysis/tree/#toolbar-menubar-autohide

AFAICT from https://searchfox.org/mozilla-central/search?q=autohide%3D%22tru&case=false&regexp=false&path=, the only place we do this is https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/browser/base/content/browser.xul#746.

Here's an outline of the basics here:

1) Make toolkit/content/widgets/toolbar.js and copy in https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/ToolbarMenubarAutohide.js
2) Add that file to jar.mn similar to https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/content/jar.mn#103 and then load it in chrome windows as per https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/content/customElements.js#250
3) Change `customElements.define("toolbar-menubar-autohide", MozToolbarMenubarAutohide);` to customElements.define("menubar-autohide", MozToolbarMenubarAutohide, {extends: "toolbar"}`. This'll wire up the CE only for `<toolbar is="menubar-autohide">`.
4) Add is="menubar-autohide" to the toolbar in addition to autohide="true" at: https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/browser/base/content/browser.xul#746

I could see possible cleanups where we drop [autohide] attr entirely and key any styling / C++ on the [is] attribute, but I wouldn't handle that here.

Comment 1

[Brian Grinstead [:bgrins]]

I know a bunch of related stuff is moving around in Bug 1356920. Mike, would anything in that bug change the plan outlined here in Comment 0 for the "toolbar-menubar" node?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> I know a bunch of related stuff is moving around in Bug 1356920. Mike, would
> anything in that bug change the plan outlined here in Comment 0 for the
> "toolbar-menubar" node?

AFAIK, no.

Comment 3

[Brian Grinstead [:bgrins]]

I forgot the last step: removing the binding code https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/content/widgets/toolbar.xml#12 and CSS rule https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/content/xul.css#287.

Comment 4

[Brian Grinstead [:bgrins]]

Gijs, I realized this binding has a customizableui counterpart: https://searchfox.org/mozilla-central/search?q=menubar-autohide&path=

xul.css (https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/content/xul.css#279-289):

  %ifdef XP_MACOSX
  toolbar[type="menubar"] {
    min-height: 0 !important;
    border: 0 !important;
  }
  %else
  toolbar[type="menubar"][autohide="true"] {
    -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide");
    overflow: hidden;
  }

browser.css (https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/browser/base/content/browser.css#73-81):

  %ifdef XP_MACOSX
  #toolbar-menubar {
    -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#toolbar-menubar-stub");
  }
  %endif

  #toolbar-menubar[autohide="true"] {
    -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#toolbar-menubar-autohide");
  }

I have a few questions:

1) Do we ever use the toolkit version of the binding in the browser UI? The only consumer I see is https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/browser/base/content/browser.xul#746 which is guarded on `#ifdef MENUBAR_CAN_AUTOHIDE` which I think is basically equivalent to `%ifndef XP_MACOSX` and so should always get the rule from browser.css, unless if I'm missing something.
2) If (1) is true, then does the work in Bug 1473311 and blockers cover the work needed for the customizableui toolbar-menubar-autohide binding?

If both of those are true then this bug might get a lot easier (i.e. delete the toolkit binding).

Comment 5

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Gijs, I realized this binding has a customizableui counterpart:
> https://searchfox.org/mozilla-central/search?q=menubar-autohide&path=

> 1) Do we ever use the toolkit version of the binding in the browser UI? The
> only consumer I see is
> https://searchfox.org/mozilla-central/rev/
> 28fe656de6a022d1fc01bbd3811023fca99cf223/browser/base/content/browser.
> xul#746 which is guarded on `#ifdef MENUBAR_CAN_AUTOHIDE` which I think is
> basically equivalent to `%ifndef XP_MACOSX` and so should always get the
> rule from browser.css, unless if I'm missing something.

Yeah, we only use an autohidden menubar in the main browser window, and there we use the browser binding rather than the toolkit one.

> 2) If (1) is true, then does the work in Bug 1473311 and blockers cover the
> work needed for the customizableui toolbar-menubar-autohide binding?

bug 1473311 is meant to cover it, though I don't think I filed specific deps for those bits yet. I wouldn't even move this to a CE, I'd just keep the logic in a separate file. This is ultimately a 1-binding-per-browser-window situation (it's not used elsewhere) and so I don't see a value in making it a web component / custom element, as we'll never have more than 1 anyway.

> If both of those are true then this bug might get a lot easier (i.e. delete
> the toolkit binding).

Yeah, I think we can definitely just delete the toolkit binding. If you agree with my assessment for (2), please file a dep, or needinfo me on that bug to file a dep? Thank you!

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #2)
> AFAIK, no.

I concur. I don't see this being a problem. Thanks for looking out!

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #5)
> > If both of those are true then this bug might get a lot easier (i.e. delete
> > the toolkit binding).
> 
> Yeah, I think we can definitely just delete the toolkit binding. If you
> agree with my assessment for (2), please file a dep, or needinfo me on that
> bug to file a dep? Thank you!

Sounds great. Let's go ahead and delete the toolkit binding here, and handle the customizable ui binding in Bug 1499236.

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: remove-toolbar-menubar-autohide.diff

Hey Brian, here's what I got so far.

I'm getting these errors after a `mach build` though, any hints?

```
JavaScript error: , line 0: uncaught exception: Error opening input stream (invalid filename?): chrome://global/content/elements/toolbar.js
JavaScript error: , line 0: uncaught exception: Error opening input stream (invalid filename?): chrome://global/content/elements/toolbar.js
```

Comment 9

[:Gijs (he/him)]

(In reply to Victor Porof [:vporof][:vp] from comment #8)
> Created attachment 9017425 [details] [diff] [review]
> remove-toolbar-menubar-autohide.diff
> 
> Hey Brian, here's what I got so far.
> 
> I'm getting these errors after a `mach build` though, any hints?
> 
> ```
> JavaScript error: , line 0: uncaught exception: Error opening input stream
> (invalid filename?): chrome://global/content/elements/toolbar.js
> JavaScript error: , line 0: uncaught exception: Error opening input stream
> (invalid filename?): chrome://global/content/elements/toolbar.js
> ```

You're packaging toolbar.js in content/widgets, but you are requesting it via URL pointing to content/elements. See the packaging of tabbox.js for a working example.


I'm a bit confused though, because comment #7 seems to suggest we could just remove this binding - I don't think you need the toolbar.js file at all in that case.

Comment 10

[Brian Grinstead [:bgrins]]

We can simply remove the binding and the reference to it in xul.css as per Comments 4-7. Sorry for any confusion, I originally thought this was in use but turns out it isn't.

Comment 11

[Victor Porof [:vporof][:vp]]

Attachment: remove-toolbar-menubar-autohide-2.diff

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 9018127 [details] [diff] [review]
remove-toolbar-menubar-autohide-2.diff

Review of attachment 9018127 [details] [diff] [review]:
-----------------------------------------------------------------

Code removal looks good. Please confirm with a try push before landing - I could imagine some toolkit tests relying on it

Comment 13

[Victor Porof [:vporof][:vp]]

Attachment: remove-toolbar-menubar-autohide-3.diff

Fixed https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b56e2abfa0526c77a033a1bfabd38cdfe830c53&selectedJob=206595434

Comment 14

[Brian Grinstead [:bgrins]]

Lots of intermittents, but I don't see anything related to this binding https://treeherder.mozilla.org/#/jobs?repo=try&revision=40416865c0481b54bdbd629d550b2a21a2d10e04

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e87ad3335ba
Remove toolbar-menubar-autohide binding. r=bgrins

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/6e87ad3335ba