Closed Bug 1498740 Opened 2 years ago Closed 2 years ago

Remove the toolkit toolbar-menubar-autohide binding

Categories

(Toolkit :: XUL Widgets, task, P4)

task

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bgrins, Assigned: vporof)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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: → 1356920
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.
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)
(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)
See Also: → 1499236
(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)
Summary: Convert toolbar-menubar-autohide to a Custom Element → Remove the toolkit toolbar-menubar-autohide binding
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)
(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.
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)
Attachment #9017425 - Attachment is obsolete: true
Attachment #9018127 - Flags: review?(bgrinstead)
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+
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
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e87ad3335ba
Remove toolbar-menubar-autohide binding. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e87ad3335ba
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1501235
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.