864355 - Extend customization target across (almost) the entire nav-bar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We want the customization target to extend across almost the entire nav-bar. The only element that shouldn't be in there are (as far as I can tell) the menu panel button and the window-controls toolbar item.

Comment 1

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

Attachment: Patch v1

Here's my first crack at it.

Had to modify some CSS rules to account for the nav-bar-customizationtarget node between the back/forward button and stop/reload buttons and the toolbar. Hopefully I didn't miss any in there.

Anyhow, this seems to do the job. Thoughts?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 740355 [details] [diff] [review]
Patch v1

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

::: browser/base/content/browser.css
@@ -139,5 @@
>  #wrapper-urlbar-container #urlbar-container > #urlbar > toolbarbutton,
>  #urlbar-container:not([combined]) > #urlbar > toolbarbutton,
>  #urlbar-container[combined] + #reload-button + #stop-button,
>  #urlbar-container[combined] + #reload-button,
> -toolbar:not([mode="icons"]) > #urlbar-container > #urlbar > toolbarbutton,

Is this rule just removed because non-icon mode toolbars won't exist?

::: browser/base/content/browser.js
@@ +4051,5 @@
>      var reload = document.getElementById("reload-button");
>      var stop = document.getElementById("stop-button");
>  
>      if (urlbar) {
> +      if (urlbar.parentNode.parentNode.getAttribute("mode") != "icons" ||

If you apply the patch from bug 573329 then we shouldn't need to update this line anymore. I guess we should just land the patch in bug 573329 on jamun now.

::: browser/base/content/browser.xul
@@ +530,5 @@
>               defaultset="unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,webrtc-status-button,downloads-button,home-button,bookmarks-menu-button-container,window-controls"
>               customizationtarget="nav-bar-customizationtarget"
>               context="toolbar-context-menu">
>  
> +      <hbox id="nav-bar-customizationtarget" class="customization-target" flex="0">

I'm just going to assume that you didn't change anything when you indented all of the children of this element :)

::: browser/themes/osx/browser.css
@@ +8,5 @@
>  %filter substitution
>  %define fgTabTexture linear-gradient(hsla(0,0%,100%,0.6), hsla(0,0%,100%,0.6) 0px, hsl(0,0%,99%) 1px, hsl(0,0%,92%))
>  %define fgTabBackgroundMiddle linear-gradient(transparent, transparent)
>  %define forwardTransitionLength 150ms
> +%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) #navigator-toolbox[iconsize=large][mode=icons] > :-moz-any(#nav-bar[currentset*="unified-back-forward-button,urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget > #unified-back-forward-button

The second rule here (and in the windows/browser.css) should also check for [iconsize=large][mode=icons], or [mode=icons] at the least.

Comment 3

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

Attachment: Patch v1.1

(In reply to Jared Wein [:jaws] from comment #2)
> Comment on attachment 740355 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 740355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.css
> @@ -139,5 @@
> >  #wrapper-urlbar-container #urlbar-container > #urlbar > toolbarbutton,
> >  #urlbar-container:not([combined]) > #urlbar > toolbarbutton,
> >  #urlbar-container[combined] + #reload-button + #stop-button,
> >  #urlbar-container[combined] + #reload-button,
> > -toolbar:not([mode="icons"]) > #urlbar-container > #urlbar > toolbarbutton,
> 
> Is this rule just removed because non-icon mode toolbars won't exist?
> 

Yeah - I've removed that change since it's taken care of in bug 573329 now.


> ::: browser/base/content/browser.js
> @@ +4051,5 @@
> >      var reload = document.getElementById("reload-button");
> >      var stop = document.getElementById("stop-button");
> >  
> >      if (urlbar) {
> > +      if (urlbar.parentNode.parentNode.getAttribute("mode") != "icons" ||
> 
> If you apply the patch from bug 573329 then we shouldn't need to update this
> line anymore. I guess we should just land the patch in bug 573329 on jamun
> now.
> 

I don't need to add the new parentNode, but I *do* need to remove the line - otherwise, the stop/reload buttons don't appear to merge into the urlbar (this is because urlbar.praentNode.getAttribute("mode") != "icons" always returns true).

> ::: browser/base/content/browser.xul
> @@ +530,5 @@
> >               defaultset="unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,webrtc-status-button,downloads-button,home-button,bookmarks-menu-button-container,window-controls"
> >               customizationtarget="nav-bar-customizationtarget"
> >               context="toolbar-context-menu">
> >  
> > +      <hbox id="nav-bar-customizationtarget" class="customization-target" flex="0">
> 
> I'm just going to assume that you didn't change anything when you indented
> all of the children of this element :)
> 

Correct! :)

> ::: browser/themes/osx/browser.css
> @@ +8,5 @@
> >  %filter substitution
> >  %define fgTabTexture linear-gradient(hsla(0,0%,100%,0.6), hsla(0,0%,100%,0.6) 0px, hsl(0,0%,99%) 1px, hsl(0,0%,92%))
> >  %define fgTabBackgroundMiddle linear-gradient(transparent, transparent)
> >  %define forwardTransitionLength 150ms
> > +%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) #navigator-toolbox[iconsize=large][mode=icons] > :-moz-any(#nav-bar[currentset*="unified-back-forward-button,urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget > #unified-back-forward-button
> 
> The second rule here (and in the windows/browser.css) should also check for
> [iconsize=large][mode=icons], or [mode=icons] at the least.

As per your lead in bug 573329, I've removed the [mode=icons] conditions.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 740816 [details] [diff] [review]
Patch v1.1

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

::: browser/themes/osx/browser.css
@@ +8,5 @@
>  %filter substitution
>  %define fgTabTexture linear-gradient(hsla(0,0%,100%,0.6), hsla(0,0%,100%,0.6) 0px, hsl(0,0%,99%) 1px, hsl(0,0%,92%))
>  %define fgTabBackgroundMiddle linear-gradient(transparent, transparent)
>  %define forwardTransitionLength 150ms
> +%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) #navigator-toolbox[iconsize=large] > :-moz-any(#nav-bar[currentset*="unified-back-forward-button,urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget > #unified-back-forward-button

The second rule here (and in windows/browser.css) still needs to check for iconsize=large.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Nevermind about the previous comment. I didn't notice that it was still inside of the :-moz-any().

Comment 6

[John Doe]

I know that I'm late to the party since there is already reviewed and working patch to this bug, but i'll ask anyway.

If i understand correctly you are trying to fix the "move important controls in a hidden toolbar" problem here.

Would it be possible to just make the containing toolbar always visible instead of limiting the customization options?

I think that it's a good compromise.

Comment 7

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

Attachment: Patch v1.2 (r+'d by jaws)

De-bitrotted, landing in a minute.

Comment 8

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

Landed on jamun as https://hg.mozilla.org/projects/jamun/rev/4b42aaccf679

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And backed out on jamun, for breaking everything (bug 866590):
https://hg.mozilla.org/projects/jamun/rev/332f1a4361e1

Comment 10

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

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #9)
> And backed out on jamun, for breaking everything (bug 866590):
> https://hg.mozilla.org/projects/jamun/rev/332f1a4361e1

Good catch.

This problem is caused when the currentset of a toolbar doesn't include "webrtc-status-button".

Originally, we guaranteed that the webrtc-status-button was in the nav-bar by making it an explicit child of nav-bar, and not setting removable="true" on it. toolbar.xml knew to leave it alone[1].

We're currently ignoring the removable attribute, and the entire contents of the nav-bar are being swept out before injecting the items from currentset.

It seems that we never added a UI migration to migrateUI to add the webrtc-status-button to the nav-bar's currentset. This means that it's getting swept out.

So, solutions:

1) Add a UI migration to add the webrtc-status-button to the nav-bar's currentset
2) Stop ignoring the removable attribute when setting up the nav-bar, and only remove items if they can be removed.
3) Put the webrtc-status-button outside of the customization target

Gonna use Occam's on this one and go with #3, unless there are any objections.

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#288

Comment 11

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

Attachment: Patch v1.3

The only thing I've changed is that I've put the webrtc-status-button outside of the customization target.

Comment 12

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

Comment on attachment 743060 [details] [diff] [review]
Patch v1.3

Nope, I think Occam's Razor failed me. It turns out we probably want to go with just respecting the removable attribute, otherwise, things like the titlebar-placeholder's get slurped out of the TabsToolbar and toolbar-menubar in bug 864425.

Comment 13

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

Attachment: Patch v1.4

So now we respect the removable attribute so that child nodes of customizable toolbars *without* that attribute are ignored when extracting items not in the currentset.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #12)
> It turns out we probably want to go
> with just respecting the removable attribute

Agreed - was thinking yesterday we'll need this for the URL bar too.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 743097 [details] [diff] [review]
Patch v1.4

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

And filed bug 866978 to enforce this in customization mode.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/projects/jamun/rev/7105d3282462

Comment 17

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

https://hg.mozilla.org/projects/ux/rev/4b42aaccf679

Comment 18

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

Sorry - correction. Ignore the changeset in comment 17 - that was backed out. The one you're interested in is:

https://hg.mozilla.org/projects/ux/rev/7105d3282462

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

The patch for this bug caused the navbar to get out of order on OS X. The removable widgets are located outside of the customization target and the non-removable ones are in the customization target (as inspected with DOM Inspector).

I did a local backout of this patch (with a minor hunk failure) and the breakage went away. See http://screencast.com/t/1H2ri7PORK for a screenshot of the breakage.

By the way... I think I just won screencast.com, since the autogenerated URL concludes with 'PORK'. /me drops mic.

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/7105d3282462