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.

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 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.

(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 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.

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

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.

De-bitrotted, landing in a minute.

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

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

(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

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

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.

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.

(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 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.

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

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.