Remove the toolkit toolbar-menubar-autohide binding

RESOLVED FIXED in Firefox 64

Status

()

P4
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: bgrins, Assigned: vporof)

Tracking

(Blocks: 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 months ago
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.
(Reporter)

Comment 1

5 months ago
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?
Flags: needinfo?(mconley)
See Also: → bug 1356920
(Reporter)

Updated

5 months ago
Status: NEW → ASSIGNED
Priority: -- → P4
(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.
(Reporter)

Comment 4

5 months ago
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).
Flags: needinfo?(gijskruitbosch+bugs)

Comment 5

5 months ago
(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!
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bgrinstead)
(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!
Flags: needinfo?(mconley)
(Reporter)

Updated

5 months ago
See Also: → bug 1499236
(Reporter)

Comment 7

5 months ago
(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.
Flags: needinfo?(bgrinstead)
(Reporter)

Updated

5 months ago
Summary: Convert toolbar-menubar-autohide to a Custom Element → Remove the toolkit toolbar-menubar-autohide binding
(Assignee)

Comment 8

5 months ago
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
```
Flags: needinfo?(bgrinstead)

Comment 9

5 months ago
(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.
(Reporter)

Comment 10

5 months ago
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.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 11

5 months ago
Attachment #9017425 - Attachment is obsolete: true
Attachment #9018127 - Flags: review?(bgrinstead)
(Reporter)

Comment 12

5 months ago
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
Attachment #9018127 - Flags: review?(bgrinstead) → review+
(Reporter)

Comment 14

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

Comment 15

5 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e87ad3335ba
Remove toolbar-menubar-autohide binding. r=bgrins
Keywords: checkin-needed

Comment 16

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e87ad3335ba
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1501235
You need to log in before you can comment on or make changes to this bug.