We're planning to merge back/forward, URL bar, reload/stop/go into one toolbar item to reduce maintenance costs and make customization more intuitive (eschewing magicall combining UI) when we land the customization panel flow for Australis.

Super Dumb idea

(In reply to Phillip M. Jones, C.E.T. from comment #1) > Super Dumb idea Phillip, comments like these are unhelpful. Stating why you disagree would be more helpful.

If you're going to do this, at least provide an option to disable conditional forward button.

One of the great advantages of Firefox over Chrome is the interface customizability. The advantages of this move are spurious, (seriously, what kind of 'maintenance costs' are involved in keeping buttons on the toolbar seperate?!) and will just push people to Chrome. Firefox devs have to realise that, at this point, Chrome is better in almost every way to Firefox. I love Firefox, but I'm not deluding myself about this. The only real advantage we have is customizability. I may be in the heavy minority, but I use Firefox and suffer through its rough points because I love using a browser that works how I want it to - not how google wants it. Please don't do this. This is user-hostile.

We're going to allow users to keep them separate for now, but critical items like these will not be allowed to be removed from the navigation toolbar.

That is nice. But like you stated also for the stay-plan of the add-on bar: "For Now" My opinion is, if you really are wanting to go with Australis in that more "Beginner User Friendly Customization mode" then you should go on and lock/remove Elements as planned. I do not agree with this plan, that is my right. But do you really believe if you hear always an user outcry and you walk back one or 2 steps that you really reach your goals? A Dev Team's decision should be put in stone and not flexible like a gum band, that does nothing good for the Situation. So please decide if you either want to go that Restrictive Route or stop doing all that Stuff at all. Because decisions like that do please in the End no Side. To speak in Binary Numbers.. It's one or Zero! In the end, after some versions the same question is then again the same for you. So please do it all or do nothing at all. You can not please both Sides, stay ore Move. But please god, stop messing around and make a Decision!

This is why maintaining this is not zero effort.

I think the problem here is rather, the addition of a superfluous animation/"feature" that glitches the UI, is distracting, requires maintenance and the only benefit is the gain of 10 to 15 px screen space in a world where most people use widescreens. Also, why does this gimmick outweigh the ability to customize 5 different toolbar items? Are there any other reasons?

(In reply to Frank Yan (:fryn) from comment #7) > Created attachment 750758 [details] > screencast of glitch in current UI > > This is why maintaining this is not zero effort. Two things: - I believe this is a platform bug, something that should be fixed either way. - In bug 856665 I'm arguing that we shouldn't render the keyhole when it's detached from the location bar, as it looks less intentional when adjacent items in the toolbar don't have any button shape. This would drive down the maintenance cost.

As we discussed in the Australis meeting we still want.

(In reply to Stephen Horlander from comment #10) > As we discussed in the Australis meeting we still want. Will need a more complete reasoning than comment 0, then.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11) > (In reply to Stephen Horlander from comment #10) > > As we discussed in the Australis meeting we still want. > > Will need a more complete reasoning than comment 0, then. I think "make customization more intuitive" is sufficient reasoning but I can expand on the rationale. "Magic buttons" have a few problems: - Lie to the user about the actual state of their toolbar - Individually dragging the buttons make it unclear exactly what is going to happen when they exit customization mode - Rearranging the buttons without an obvious way to put them back together outside of resetting the entire toolbar - Add-ons can insert themselves between these items breaking the them apart with no input from the user We could fix all of these problem by putting in extra work and using better visual indicators of how the buttons flow together. However this would increase the complexity of the interface *and* the code. It doesn't make sense to maintain this behavior when we believe that this widget is a key part of the interface that shouldn't be removed or easily altered.

This bug's summary isn't "get rid of the magic buttons", though. It's suggesting a more specific (simple) solution to that problem. We could avoid "magic buttons" by offering different combinations of hard-coded buttons/button groups, as suggested on the firefox-dev thread. It introduces some implementation and migration complexity; I wonder how much. Are there other downsides?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13) > This bug's summary isn't "get rid of the magic buttons", though. It's > suggesting a more specific (simple) solution to that problem. We could avoid > "magic buttons" by offering different combinations of hard-coded > buttons/button groups, as suggested on the firefox-dev thread. It introduces > some implementation and migration complexity; I wonder how much. Are there > other downsides? I think these downsides are exactly what we're trying to get away from when moving towards a more agile and competitive browser front-end. Continuing to carry forward separate combinations, or introducing new implementations to support minor use-cases distracts from working on more pertinent issues.

Wow.. Why you do not make it easy for you and remove ALL options to customize. You speak of being competitive... People do move away from Chrome partly BECAUSE Firefox is offering all that Customization options - There is even a petition running that Chrome adds for example a fully customizable bar - How do you guys justify these changes then? That bigtime Restrictions are not only Firefox User Hostile, no, if anyone wants to change from Chrome to Firefox they will see no reason anymore because "Firefox becomes anyway a Google Rip-Off" You understand what i mean? Is it really necessary to lock everything down like it is for example at Chrome or IE? Firefox gained it's Users BECAUSE they like it a bit complicated. And now suddenly that should not be the case anymore? Sorry, you guys ask seriously the wrong people!

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13) > This bug's summary isn't "get rid of the magic buttons", though. Perhaps we should fix the summary then. The UX and Product folks have made their requirements clear.

You know that reaching a decision is never that simple and includes other factors, so that's really not a useful or productive statement. We discussed this on IRC. The arguments in favor are: - "magic" buttons have implementation as well as user customization-interaction downsides (previously detailed here, in comment 0 and comment 12) - there is no great "middle ground" here. Allowing flexibility necessarily allows accounting for the various combinations, even if we try to restrict those further. Offering multiple variants of these buttons is still confusing, and doesn't really avoid any implementation complexity. - customization of this type is a "2%" use case that is best left addressed by add-ons, and the combined benefits of the simplified model (for users) and implementation/maintenance savings (for us) outweigh the concerns and loss of good will from the set of users who will be upset at losing this built-in flexibility On the whole I find this argument reasonable, though I really wish we could validate the last assertion with some data. I think we should just move forward with this.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17) > You know that reaching a decision is never that simple and includes other > factors.... It should be that simple and that it's not is a serious and growing problem. How to proceed here absolutely should be the decision of UX and Product. It's mightily frustrating to see that Product and UX, who are charged with specifying requirements and interactions, are being told that it's not that simple. This has to change. I'm way off topic now and so I'll take this to Governance and we can sort things out there.

When I merge the Back/ Forward buttons together with the urlbar toolbaritem, the translucent property of the buttons cause the urlbar textbox to remain visible. In this case the separate toolbaritem for the back/ forward buttons allowed for its current appearance. AFAICT merging these sets together will account for a significant amount of work. I'm also asking whether this is possible at all.

This is WITH the forward button visible and enabled.

The styling that is used for the conditional forward button and the back button's clip path will need to be updated. When this bug is complete, there will be a significant amount of code that can be removed. It is also possible that the margin tricks that were needed to change position of the forward button without shifting toolbaritems may be obsolete since this combined toolbaritem can keep the same size regardless of the forward button being visible or not. You can look at early implementations for the conditional forward button to get an idea if you are curious, bug 682534.

Actually, most of that conversation can be read in bug 674744. It should give some background as to the approach that was taken to implement the conditional forward button the way that it is today. Making this change though should be doable, but it may take a couple days.

Jared, I don't know if you have time to try this patch out, but there are two issues that I currently see: 1. Hovering the urlbar shows a darker textbox border than the urlbar itself near the back/forward buttons 2. Follow this STR: * Open UX and navigate to another webpage in the same tab * Go back one page, so that the forward buttons becomes enabled * Click on the forward button with your mouse and you will see a visual glitch when the forward button disables. Especially 2) is interesting to me, because atm I don't have an idea to fix it. How about you? Thanks!

(In reply to Mike de Boer [:mikedeboer] from comment #24) > 1. Hovering the urlbar shows a darker textbox border than the urlbar itself > near the back/forward buttons This is a known issue which is not new. It is tracked in bug 700363.

(In reply to Mike de Boer [:mikedeboer] from comment #24) > 2. Follow this STR: > * Open UX and navigate to another webpage in the same tab > * Go back one page, so that the forward buttons becomes enabled > * Click on the forward button with your mouse and you will see a visual > glitch when the forward button disables. > > Especially 2) is interesting to me, because atm I don't have an idea to fix > it. How about you? It looks like the clip path for the back button is being applied too soon.

Comment on attachment 758654 [details] [diff] [review] WIP: working patch, only tested on OSX Review of attachment 758654 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +311,5 @@ > aElm.addEventListener("mousedown", mousedownHandler, true); > aElm.addEventListener("click", clickHandler, true); > } > > + // Bug 414797: Clone back-button's context menu into both the Do we still need to do this? Now that there is only one back and forward button, we shouldn't need to make copies of their context menus. @@ +2174,5 @@ > > var ibefore = null; > if (urlbar && searchbar) { > if (urlbar.nextSibling == searchbar || > + urlbar.getAttribute("combined")) Is this attribute necessary anymore? ::: browser/base/content/browser.xul @@ +571,5 @@ > <toolbar id="nav-bar" class="toolbar-primary chromeclass-toolbar" > aria-label="&navbarCmd.label;" > fullscreentoolbar="true" mode="icons" customizable="true" > iconsize="large" > + defaultset="urlbar-container,search-container,webrtc-status-button,bookmarks-menu-button,downloads-button,home-button" We should probably rename urlbar-container to navigation-container. ::: browser/components/nsBrowserGlue.js @@ +1212,5 @@ > let currentset = this._getPersist(toolbarResource, currentsetResource); > // Need to migrate only if toolbar is customized and all 3 elements are found. > if (currentset && > + currentset.indexOf("urlbar-container") != -1) { > + currentset = currentset.replace(/(^|,)urlbar-container($|,)/, We shouldn't be changing old UI migrations. They are only run once. We'll need to introduce a new UI migration step at the bottom so that it will run for all users. ::: browser/themes/osx/browser.css @@ +6,5 @@ > > %include shared.inc > %filter substitution > %define forwardTransitionLength 150ms > +%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) #navigator-toolbox[iconsize=large] > :-moz-any(#nav-bar[currentset*="urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget > #urlbar-container Having this shouldn't be necessary anymore, since we can now always expect the conditional forward button to be present. @@ +1180,5 @@ > border-top-right-radius: 0; > border-bottom-right-radius: 0; > } > > +@conditionalForwardWithUrlbar@[forwarddisabled] > #urlbar { The graphical glitch is because you are now clipping the urlbar instead of the urlbar-container. The grey you are seeing is the toolbar color coming through here now. What happens if you put an hbox around #urlbar and apply the clipping to the hbox?

Jared: this is a patch of the merged buttons working on all platforms. A couple of remarks: 1. I was able to remove the defines in each individual browser.css file, except for Windows. That buggar relies too much on the complexity of the selector, so keeping the define there helps readability in any case. I tried many permutations to reduce the selector length, but each try resulted in a whacky UI. 2. Was able to fix the clip-path for OSX, which also fixes bug 873662 and bug 703063. The clip-path for Windows also needed a slight offset adjustment. 3. Wrapping the urlbar in an hbox and applying the clip-path on that one worked perfectly! Thanks for the suggestion ;) 4. Cloning the popup for the forward button still appears to be necessary, also style-wise. I would like to suggest to move deprecation of that code to another (new) bug. 5. Migration: what migration step would you suggest to add to nsBrowserGlue.js? That is all :)

(In reply to Jared Wein [:jaws] from comment #27) > > + // Bug 414797: Clone back-button's context menu into both the > > Do we still need to do this? Now that there is only one back and forward > button, we shouldn't need to make copies of their context menus. There have never been multiple back and forward buttons. > We should probably rename urlbar-container to navigation-container. Why? This seems completely unnecessary. > We shouldn't be changing old UI migrations. They are only run once. We'll > need to introduce a new UI migration step at the bottom so that it will run > for all users. If you keep the urlbar-container ID, you shouldn't have to migrate anything.

Comment on attachment 759136 [details] [diff] [review] WIP: patch for all platforms >--- a/browser/themes/linux/browser.css >+++ b/browser/themes/linux/browser.css > #forward-button, >-@conditionalForwardWithUrlbar@ > #forward-button { >+#navigation-container > #forward-button { > list-style-image: url("moz-icon://stock/gtk-go-forward-ltr?size=toolbar"); > } "#navigation-container > #forward-button" is redundant, it's already covered by #forward-button. You shouldn't touch browser/metro/ at all for this bug. Desktop and metro Firefox have entirely separate UIs. For some reason you removed @conditionalForwardWithUrlbarWidth@ and hardcoded its value all over the place. Please undo this change. On Windows you made all selectors involving @conditionalForwardWithUrlbar@ use the descendent selector. You should use the child selector instead. This and comment 29 are just drive-by comments, I didn't do a complete review of this patch.

Comment on attachment 759136 [details] [diff] [review] WIP: patch for all platforms What Dao said. Dao will be a better reviewer for this. Please send future reviews his way. I can give drive-by feedback if necessary.

Dão: I humbly reverted all the changes you remarked upon, so I think there's nothing left in the way of a review!

(In reply to Dão Gottwald [:dao] from comment #30) > > On Windows you made all selectors involving @conditionalForwardWithUrlbar@ > use the descendent selector. You should use the child selector instead. I used the descendent selector in selectors like `@conditionalForwardWithUrlbar@ #urlbar` instead of `@conditionalForwardWithUrlbar@ > #urlbar`, because I wrapped #urlbar in an hbox to apply the back-button clip-path on (as tipped by Jared). These are the only occurrences I ought to have made that change, so if you see any others: needs to be corrected. Or would you like the selector to become `@conditionalForwardWithUrlbar@ > hbox > #urlbar`, or `@conditionalForwardWithUrlbar@ > .urlbar-wrapper > #urlbar`? If so, I have something new to learn as to selector efficience, performance or readability :) Thanks in advance!

(In reply to Mike de Boer [:mikedeboer] from comment #33) > Or would you like the selector to become `@conditionalForwardWithUrlbar@ > > hbox > #urlbar`, or `@conditionalForwardWithUrlbar@ > .urlbar-wrapper > > #urlbar`? Yep (the latter)

(In reply to Dão Gottwald [:dao] from comment #34) > > Yep (the latter) Alright, will do. Could you please quickly explain to me why this is unilaterally better?

(In reply to Mike de Boer [:mikedeboer] from comment #35) > (In reply to Dão Gottwald [:dao] from comment #34) > > > > Yep (the latter) > > Alright, will do. Could you please quickly explain to me why this is > unilaterally better? Please see https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS and https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS#Avoid_the_descendant_selector.21 in particular.

That was quick, thanks ;)

Unbitrotted patch and updated selectors

Properly unbitrotted/ updated patch.

Comment on attachment 761362 [details] [diff] [review] Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >--- a/browser/base/content/browser.css >+++ b/browser/base/content/browser.css >+#wrapper-urlbar-container #urlbar-container > .urlbar-wrapper > #urlbar > toolbarbutton, >+#urlbar-container > .urlbar-wrapper > #urlbar > toolbarbutton, > #urlbar-reload-button:not([displaystop]) + #urlbar-stop-button, > #urlbar-reload-button[displaystop], >-#reload-button:not([displaystop]) + #stop-button, > #reload-button[displaystop] { > visibility: collapse; > } Please use the child selector in the first line. I don't understand the second line. What are you trying to do there? As for "#reload-button[displaystop]", the reload-button element is gone, right? >-#urlbar-container[combined] > #urlbar > #urlbar-icons > #go-button, >+#urlbar-container > .urlbar-wrapper > #urlbar > #urlbar-icons > #go-button, > #urlbar[pageproxystate="invalid"] > #urlbar-icons > .urlbar-icon:not(#go-button), > #urlbar[pageproxystate="valid"] > #urlbar-icons > #go-button, > #urlbar[pageproxystate="invalid"][focused="true"] > #urlbar-go-button ~ toolbarbutton, > #urlbar[pageproxystate="valid"] > #urlbar-go-button, > #urlbar:not([focused="true"]) > #urlbar-go-button { > visibility: collapse; > } The first line unconditionally hides #go-button. Seems like that element should be removed. >+ <hbox class="urlbar-wrapper" flex="1"> You can use an id instead of a class here since it's unique. >--- a/browser/themes/osx/browser.css >+++ b/browser/themes/osx/browser.css >-@conditionalForwardWithUrlbar@ > #forward-button:not(:-moz-lwtheme) { >+#urlbar-container > #forward-button:not(:-moz-lwtheme) { Since #forward-button is always a child node of #urlbar-container, "#urlbar-container >" seems redundant here. >-@conditionalForwardWithUrlbar@ > #forward-button { >+#urlbar-container > #forward-button { ditto (stopping here, please look for similar cases and fix them all) >+#urlbar-container:not([switchingtabs]) #urlbar { use the child selector >+#urlbar-container[forwarddisabled] .urlbar-wrapper { ditto (stopping here, please look for similar cases and fix them all) >--- a/browser/themes/windows/browser.css >+++ b/browser/themes/windows/browser.css >-@conditionalForwardWithUrlbar@ + #urlbar-container { >+@conditionalForwardWithUrlbar@ { > padding-left: @conditionalForwardWithUrlbarWidth@px; > -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; > position: relative; Doesn't this need to target the urlbar-wrapper node? >- pointer-events: none; >-} >- >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar { >+} >+ >+@conditionalForwardWithUrlbar@ > .urlbar-wrapper > #urlbar { > -moz-border-start: none; > margin-left: 0; >- pointer-events: all; >-} Why did you remove the pointer-events stuff? >+@conditionalForwardWithUrlbar@:not([switchingtabs]) #urlbar { Use the child selector. Please look for similar cases and fix them all. >-#urlbar-search-splitter + #urlbar-container > #urlbar , >+#urlbar-search-splitter #urlbar , > #urlbar-search-splitter + #search-container > #searchbar > .searchbar-textbox { > -moz-margin-start: 0; > } #urlbar can't be nested in #urlbar-search-splitter, so this updated selector is never going to match something.

Dão: I ask you for feedback, because I'm stuck on the styling in RTL mode on Windows. For some reason the #urlbar-container is stuck to the rightmost border. Do you have an idea as to why this might happen and if there's a work around?

patch with comments addressed.

Mike, as you are working on this can you make sure not to regress bug 571454? Thanks!

Removing the items from M7 that do not block landing on m-c.

Comment on attachment 764186 [details] [diff] [review] Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >+ <toolbaritem id="urlbar-container" align="center" flex="400" persist="width" >+ forwarddisabled="true" title="&locationItem.title;" removable="false" >+ class="chromeclass-toolbar-additional chromeclass-location" nooverflow="true"> Adding chromeclass-toolbar-additional here would hide the location bar in popups, which we don't want. This raises the question whether we also want to prevent back and forward buttons from being hidden in popups. This would be a behavior change. > <svg:clipPath id="windows-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse"> >- <svg:path d="M 0,0 0,7.8 C 2.5,11 4,14 4,18 4,22 2.5,25 0,28 l 0,22 10000,0 0,-50 L 0,0 z"/> >+ <svg:path d="m0,-5l0,7.8c2.5,3.2 4,6.2 4,10.2c0,4 -1.5,7 -4,10l0,22l10000,0l0,-50l-10000,0z"/> > </svg:clipPath> > #endif > #ifdef XP_MACOSX > <svg:clipPath id="osx-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox"> > <svg:path d="M 0,0 C 0.15,0.12 0.25,0.3 0.25,0.5 0.25,0.7 0.15,0.88 0,1 L 1,1 1,0 0,0 z"/> > </svg:clipPath> > <svg:clipPath id="osx-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse"> >- <svg:path d="m 0,-5 0,9.03 C 3.6,6.8 6,11.1 6,16 6,21 3.6,25 0,28 l 0,27 10000,0 0,-55 L 0,-5 z"/> >+ <svg:path d="m-30,-10l-0.14952,7.73c6.59882,1.77 8.74842,7.57 8.74842,12.47c0,5 -1.8996,11.5 -8.24851,13.25l0.04961,25.75l10000.4506,0l0,-55l-10000.8506,-4.2z"/> These numbers look very strange at a first glance... I'm actually not sure why they need to change at all. >--- a/browser/themes/linux/browser.css >+++ b/browser/themes/linux/browser.css >+#back-button, >+#forward-button, >+#urlbar-wrapper > #urlbar { > pointer-events: all; > } Use this as the selector here: #urlbar-wrapper > * >--- a/browser/themes/osx/browser.css >+++ b/browser/themes/osx/browser.css > #forward-button:-moz-lwtheme { > border-top-left-radius: 0; > border-bottom-left-radius: 0; > } >-@conditionalForwardWithUrlbar@ > #forward-button { >+#forward-button { > border-radius: 0; > -moz-margin-end: 0; > } The second rule seems to make the first one redundant. There's also an existing #forward-button rule that you should merge this with. >-@conditionalForwardWithUrlbar@ + #urlbar-container { >- padding-left: @conditionalForwardWithUrlbarWidth@px; >- -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; >- position: relative; >- pointer-events: none; >-} >- >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar { >+#urlbar-wrapper > #urlbar { > -moz-border-start: none; >+ padding-left: 0; > margin-left: 0; >- pointer-events: all; >-} Why is the stuff you removed not needed anymore? >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar > #identity-box { >+#urlbar-wrapper > #urlbar > #identity-box { > border-radius: 0; > } Remove "#urlbar-wrapper > #urlbar >" and merge with an existing #identity-box rule if possible. >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { >+#urlbar-container[forwarddisabled] #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { > transition: 0s padding-left; > padding-left: 10px; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { >+#urlbar-container[forwarddisabled] #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { > transition: 0s padding-right; > padding-right: 10px; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled]:hover:not([switchingtabs]) + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box { >+#urlbar-container[forwarddisabled]:hover:not([switchingtabs]) #urlbar > #notification-popup-box[hidden] + #identity-box { > /* delay the hiding of the forward button when hovered to avoid accidental clicks on the url bar */ > transition-delay: 100s; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled]:not(:hover) + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { >+#urlbar-container[forwarddisabled]:not(:hover) #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { > padding-left: 10.01px; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled]:not(:hover) + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { >+#urlbar-container[forwarddisabled]:not(:hover) #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { > padding-right: 10.01px; > } >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container > #urlbar > #notification-popup-box { >+#urlbar-container[forwarddisabled] #urlbar > #notification-popup-box { > padding-left: 7px; > } Use the child selector. >--- a/browser/themes/windows/browser.css >+++ b/browser/themes/windows/browser.css >-%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) :-moz-any(#nav-bar[currentset*="unified-back-forward-button,urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget > #unified-back-forward-button >+%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) :-moz-any(#nav-bar[currentset*="urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget > #urlbar-container ":-moz-any(#nav-bar[currentset*="urlbar-container"],#nav-bar:not([currentset])) > #nav-bar-customizationtarget >" seems redundant. > @conditionalForwardWithUrlbar@ > #forward-button > .toolbarbutton-icon { > /*mask: url(keyhole-forward-mask.svg#mask); XXX: this regresses twinopen */ > clip-path: url(chrome://browser/content/browser.xul#windows-keyhole-forward-clip-path); >- -moz-margin-start: -6px !important; >+ margin-left: -6px !important; > border-left-style: none; > border-radius: 0; > padding-left: 7px; > padding-right: 3px; > } Why this change? >+@conditionalForwardWithUrlbar@ > #back-button, >+@conditionalForwardWithUrlbar@ > #forward-button, >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar { >+ pointer-events: all; >+} Should also use "@conditionalForwardWithUrlbar@ > *" here.

(In reply to Dão Gottwald [:dao] from comment #46) > Comment on attachment 764186 [details] [diff] [review] > Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem > > >+ <toolbaritem id="urlbar-container" align="center" flex="400" persist="width" > >+ forwarddisabled="true" title="&locationItem.title;" removable="false" > >+ class="chromeclass-toolbar-additional chromeclass-location" nooverflow="true"> > > Adding chromeclass-toolbar-additional here would hide the location bar in > popups, which we don't want. > > This raises the question whether we also want to prevent back and forward > buttons from being hidden in popups. This would be a behavior change. I'd say the solution here would be to just remove the 'chromeclass-toolbar-additional' here, right? > > > <svg:clipPath id="windows-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse"> > >- <svg:path d="M 0,0 0,7.8 C 2.5,11 4,14 4,18 4,22 2.5,25 0,28 l 0,22 10000,0 0,-50 L 0,0 z"/> > >+ <svg:path d="m0,-5l0,7.8c2.5,3.2 4,6.2 4,10.2c0,4 -1.5,7 -4,10l0,22l10000,0l0,-50l-10000,0z"/> > > </svg:clipPath> > > #endif > > #ifdef XP_MACOSX > > <svg:clipPath id="osx-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox"> > > <svg:path d="M 0,0 C 0.15,0.12 0.25,0.3 0.25,0.5 0.25,0.7 0.15,0.88 0,1 L 1,1 1,0 0,0 z"/> > > </svg:clipPath> > > <svg:clipPath id="osx-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse"> > >- <svg:path d="m 0,-5 0,9.03 C 3.6,6.8 6,11.1 6,16 6,21 3.6,25 0,28 l 0,27 10000,0 0,-55 L 0,-5 z"/> > >+ <svg:path d="m-30,-10l-0.14952,7.73c6.59882,1.77 8.74842,7.57 8.74842,12.47c0,5 -1.8996,11.5 -8.24851,13.25l0.04961,25.75l10000.4506,0l0,-55l-10000.8506,-4.2z"/> > > These numbers look very strange at a first glance... I'm actually not sure > why they need to change at all. The numbers indeed still required some normalization, but if you look closely you'll notice the updated path fixes bug 703063 - hence the negative shift on the x-axis of 30px and the negated round curve is deeper. My follow-up patch will contain a path with normalized values.

Wait, now I understand what you meant by 'popups'... I will figure out how we can make it work for popups as well. Thanks for bringing that up.

(In reply to Dão Gottwald [:dao] from comment #46) > > >+#back-button, > >+#forward-button, > >+#urlbar-wrapper > #urlbar { > > pointer-events: all; > > } > > Use this as the selector here: > > #urlbar-wrapper > * > > >+@conditionalForwardWithUrlbar@ > #back-button, > >+@conditionalForwardWithUrlbar@ > #forward-button, > >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar { > >+ pointer-events: all; > >+} > > Should also use "@conditionalForwardWithUrlbar@ > *" here. I must say that I am quite surprised the usage of the universal selector as the right-most subselector is advocated here. https://developer.mozilla.org/en/Writing_Efficient_CSS says never to do this. Can a class be used here instead?

A class would be overkill. We can also just list all elements we want this for. The reason I commented on the first block at all was that #urlbar-wrapper is redundant in "#urlbar-wrapper > #urlbar" -- #urlbar is always a child node of #urlbar-wrapper.

Unbitrotted patch with Dão's comments addressed.

Unbitrotted patch.

removing stop & reload buttons again (proper unbitrotting this time)

Comment on attachment 771299 [details] [diff] [review] Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >--- a/browser/themes/linux/browser.css >+++ b/browser/themes/linux/browser.css >-%define conditionalForwardWithUrlbar window:not([chromehidden~=toolbar]) :-moz-any(#nav-bar[currentset*="unified-back-forward-button,urlbar-container"], #nav-bar:not([currentset])) > #nav-bar-customizationtarget > #unified-back-forward-button Please restore conditionalForwardWithUrlbar as 'window:not([chromehidden~="toolbar"]) #urlbar-container' and use it where appropriate to reduce code duplication. Applies to Windows and OS X as well. > #urlbar-container { > -moz-box-orient: horizontal; > -moz-box-align: stretch; >-} >- >-@conditionalForwardWithUrlbar@ + #urlbar-container { > -moz-padding-start: @conditionalForwardWithUrlbarWidth@px; > -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; > position: relative; > pointer-events: none; > } This should still depend on whether back/forward buttons are displayed (i.e. @conditionalForwardWithUrlbar@). >--- a/browser/themes/osx/browser.css >+++ b/browser/themes/osx/browser.css > #nav-bar #back-button[open="true"]:not(:-moz-lwtheme) { > background-position: -30px 0; > } > #nav-bar #forward-button > .toolbarbutton-icon { > /* shift the icon away from the back button */ > margin-left: 3px; > margin-right: -1px; > } Update these and similar selectors -- #nav-bar is now redundant. > /* Forward button styles */ > > #forward-button { > -moz-margin-start: 0; >+ border-radius: 0; >+ -moz-margin-end: 0; > } Can this entire rule be dropped? Not sure what it's good for. >--- a/browser/themes/windows/browser.css >+++ b/browser/themes/windows/browser.css > #nav-bar .toolbarbutton-1:not([disabled]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1:not([disabled]):hover > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon, > #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container, >-@conditionalForwardWithUrlbar@ > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon { >+#urlbar-container > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon { > background-image: linear-gradient(hsla(0,0%,100%,.6), hsla(0,0%,100%,.1)); > border-color: hsla(210,54%,20%,.15) hsla(210,54%,20%,.2) hsla(210,54%,20%,.25); > box-shadow: 0 1px hsla(0,0%,100%,.3) inset, > 0 1px hsla(210,54%,20%,.03), > 0 0 2px hsla(210,54%,20%,.1); > } > > #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon, >-@conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon { >+#urlbar-container > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon { > border-color: hsla(210,54%,20%,.3) hsla(210,54%,20%,.35) hsla(210,54%,20%,.4); > background-color: hsla(210,48%,96%,.75); > box-shadow: 0 0 1px hsla(210,54%,20%,.03), > 0 0 2px hsla(210,54%,20%,.1); > } Aren't '#urlbar-container > .toolbarbutton-1' and '#urlbar-container > #forward-button' already covered by '#nav-bar .toolbarbutton-1'? I.e. can you drop the lines that you added here? >+#urlbar-container > #forward-button { >+#urlbar-container > #forward-button > menupopup { >+#urlbar-container > #forward-button > .toolbarbutton-icon { >+#urlbar-container > #back-button { >+#urlbar-container > #back-button:-moz-locale-dir(rtl) { >+#urlbar-container > #back-button > menupopup { >+#urlbar-container > #back-button > .toolbarbutton-icon { >+#urlbar-container > #back-button:not([disabled="true"]):not([open="true"]):not(:active):hover > .toolbarbutton-icon { >+#urlbar-container > #back-button:not([disabled="true"]):hover:active > .toolbarbutton-icon, >+#urlbar-container > #back-button[open="true"] > .toolbarbutton-icon { >+#urlbar-container > #back-button[disabled] > .toolbarbutton-icon { remove '#urlbar-container > ', it's redundant >-@conditionalForwardWithUrlbar@ + #urlbar-container { >- padding-left: @conditionalForwardWithUrlbarWidth@px; >- -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; >- position: relative; >+#urlbar-container { >+ -moz-box-align: center; > pointer-events: none; > } 'pointer-events: none' belongs in the rule that also uses conditionalForwardWithUrlbarWidth to set the margin/padding when the back/forward buttons are present.

Re-added the `conditionalForwardWithUrlbar` var. Please see my reply to your review for the reason why I flagged you for feedback this time...

I flagged you for feedback instead of review, because: (In reply to Dão Gottwald [:dao] from comment #54) > > > /* Forward button styles */ > > > > #forward-button { > > -moz-margin-start: 0; > >+ border-radius: 0; > >+ -moz-margin-end: 0; > > } > > Can this entire rule be dropped? Not sure what it's good for. I tried removing it, but didn't get further than border-radius - the other we need. > > >--- a/browser/themes/windows/browser.css > >+++ b/browser/themes/windows/browser.css > > > #nav-bar .toolbarbutton-1:not([disabled]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > > #nav-bar .toolbarbutton-1:not([disabled]):hover > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon, > > #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-icon, > > #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container, > >-@conditionalForwardWithUrlbar@ > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon { > >+#urlbar-container > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon { > > background-image: linear-gradient(hsla(0,0%,100%,.6), hsla(0,0%,100%,.1)); > > border-color: hsla(210,54%,20%,.15) hsla(210,54%,20%,.2) hsla(210,54%,20%,.25); > > box-shadow: 0 1px hsla(0,0%,100%,.3) inset, > > 0 1px hsla(210,54%,20%,.03), > > 0 0 2px hsla(210,54%,20%,.1); > > } > > > > #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon, > > #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon, > >-@conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon { > >+#urlbar-container > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon { > > border-color: hsla(210,54%,20%,.3) hsla(210,54%,20%,.35) hsla(210,54%,20%,.4); > > background-color: hsla(210,48%,96%,.75); > > box-shadow: 0 0 1px hsla(210,54%,20%,.03), > > 0 0 2px hsla(210,54%,20%,.1); > > } > > Aren't '#urlbar-container > .toolbarbutton-1' and '#urlbar-container > > #forward-button' already covered by '#nav-bar .toolbarbutton-1'? I.e. can > you drop the lines that you added here? > > >+#urlbar-container > #forward-button { > > >+#urlbar-container > #forward-button > menupopup { > > >+#urlbar-container > #forward-button > .toolbarbutton-icon { > > >+#urlbar-container > #back-button { > > >+#urlbar-container > #back-button:-moz-locale-dir(rtl) { > > >+#urlbar-container > #back-button > menupopup { > > >+#urlbar-container > #back-button > .toolbarbutton-icon { > > >+#urlbar-container > #back-button:not([disabled="true"]):not([open="true"]):not(:active):hover > .toolbarbutton-icon { > > >+#urlbar-container > #back-button:not([disabled="true"]):hover:active > .toolbarbutton-icon, > >+#urlbar-container > #back-button[open="true"] > .toolbarbutton-icon { > > >+#urlbar-container > #back-button[disabled] > .toolbarbutton-icon { > > remove '#urlbar-container > ', it's redundant I replaced them with '@conditionalForwardWithUrlbar > ' instead, which is necessary due to `#navbar .toolbarbutton-1 ...` rules taking precedence over `#back-button`. Descendant selectors must be evil. Then there is the issue of the identity-box; it doesn't look good on the UX branch in RTL mode at the moment. I fixed the background positioning to make it look somewhat better but I think I need to file a separate bug to fix this. Of course, at first I thought my patch was causing this, but it turned out to be existing issue. Proved to be a wonderful time-sink though! If you're ok with my aforementioned comments, I'll flag the patch for review. Thanks!!

(In reply to Mike de Boer [:mikedeboer] from comment #56) > > >+#urlbar-container > #back-button[disabled] > .toolbarbutton-icon { > > > > remove '#urlbar-container > ', it's redundant > > I replaced them with '@conditionalForwardWithUrlbar > ' instead, which is > necessary due to `#navbar .toolbarbutton-1 ...` rules taking precedence over > `#back-button`. Descendant selectors must be evil. Can you add :not(#back-button) or/and :not(#forward-button) to the rules bothering you? > Then there is the issue of the identity-box; it doesn't look good on the UX > branch in RTL mode at the moment. I fixed the background positioning to make > it look somewhat better but I think I need to file a separate bug to fix > this. Of course, at first I thought my patch was causing this, but it turned > out to be existing issue. Proved to be a wonderful time-sink though! Did you find out what's causing it? I assume it's a UX-branch-only regression?

(In reply to Dão Gottwald [:dao] from comment #57) > > Can you add :not(#back-button) or/and :not(#forward-button) to the rules > bothering you? > Thanks for the tip, trying that angle now. > > Did you find out what's causing it? I assume it's a UX-branch-only > regression? I found where the regression was introduced on the UX branch, but not not yet on m-c. In other words, it's also a regression on m-c. :(

Dão, with adding `:not(#back-button)` and `:not(#forward-button)` I could not achieve the desired effect. The whole interwoven style mashup relies too much on inheritance. As a side note, I think it'd be worthwhile to spend some time on a bug that is focused on replacing/ removing/ refactoring all the descendant selectors in the windows theme. It seems to me that that could potentially be a nice perf win, no?

Comment on attachment 773009 [details] [diff] [review] Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem Dão, I can change the selectors for Windows back to `#urlbar-container > `, which is cheaper than the full-blown `@conditionalForwardWithUrlbar@`. What do you think?

We are expecting to get a nice perf win by not having to run initialization code after this.

Comment on attachment 773009 [details] [diff] [review] Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem Review of attachment 773009 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +3941,5 @@ > if (this._initialized) > return; > > + let reload = document.getElementById("urlbar-reload-button"); > + let stop = document.getElementById("urlbar-stop-button"); Why do we need to keep CombinedStopReload? When will it not be in the location bar anymore?

Hi Dão, as promised (a little later, because I tried to fiddle and try some more options), a patch that removes the `#urlbar-container > ` prefix from the `#forward-button` selectors. I wanted to do that one first, because it's the simplest one. I suffixed each rule in `#forward-button` with `!important` and tried removing each one by checking with the DOM Inspector which selector(s) override a specific style. As with previous attempts, I failed miserably. I hope you are able to shed some light on this issue!

Comment on attachment 774104 [details] [diff] [review] WIP: test selector-simplification on Windows So does this patch work or does it not work? If you're just concerned about the !important flags you had to add, I think it's ok to keep these now and investigate removing them in a follow-up.

(In reply to Dão Gottwald [:dao] from comment #64) > So does this patch work or does it not work? If you're just concerned about > the !important flags you had to add, I think it's ok to keep these now and > investigate removing them in a follow-up. Since the patch does work, I was indeed concerned about the !important flags I had to add. I will take your suggestion and continue the investigation in a follow-up. It does give me an itchy feeling in my stomach, though :S

(In reply to Jared Wein [:jaws] from comment #62) > Why do we need to keep CombinedStopReload? When will it not be in the > location bar anymore? If I disable the init call right now, the urlbar-reload button does not get swapped with the urlbar-stop button anymore, which is - I think - the whole point of CombinedStopReload singleton? How do you see this working otherwise?

re-introduced conditionForwardWithUrlbar variable and added !important statements in windows theme where needed.

WAK! Bittorrent Sync duplicated my patch file twice!

Comment on attachment 777105 [details] [diff] [review] Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >--- a/browser/themes/linux/browser.css Wed Jul 17 12:36:32 2013 +0200 >+++ b/browser/themes/linux/browser.css Wed Jul 17 15:38:54 2013 +0200 >-@conditionalForwardWithUrlbar@ + #urlbar-container { >+@conditionalForwardWithUrlbar@ { > -moz-padding-start: @conditionalForwardWithUrlbarWidth@px; > -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; > position: relative; > pointer-events: none; > } I believe this needs to be @conditionalForwardWithUrlbar@ > #urlbar-wrapper (already mentioned in comment 40). Same applies to Windows. >--- a/browser/themes/osx/browser.css Wed Jul 17 12:36:32 2013 +0200 >+++ b/browser/themes/osx/browser.css Wed Jul 17 15:38:54 2013 +0200 > #forward-button { > -moz-margin-start: 0; >-} >- >-#nav-bar #forward-button > .toolbarbutton-icon { >+ border-radius: 0; >+ -moz-margin-end: 0; >+} nit: move border-radius down such that -moz-margin-end comes right after -moz-margin start. >-#nav-bar #forward-button { >+#forward-button { > clip-path: url(chrome://browser/content/browser.xul#osx-keyhole-forward-clip-path); > } merge this into the above rule > #forward-button:-moz-lwtheme { > border-top-left-radius: 0; > border-bottom-left-radius: 0; > } >+#forward-button:-moz-lwtheme { > -moz-padding-start: 2px; > -moz-padding-end: 0; > } merge these two rules >-@conditionalForwardWithUrlbar@ + #urlbar-container { >- padding-left: @conditionalForwardWithUrlbarWidth@px; >- -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; >- position: relative; >- pointer-events: none; >-} >- >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar { >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar { > -moz-border-start: none; >+ padding-left: 0; > margin-left: 0; >- pointer-events: all; >-} Why is it ok to remove the pointer-events stuff? Already asked this in comment 46. >-@conditionalForwardWithUrlbar@ + #urlbar-container:-moz-locale-dir(rtl), >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar:-moz-locale-dir(rtl) { >+@conditionalForwardWithUrlbar@ > #back-button:-moz-locale-dir(rtl), >+@conditionalForwardWithUrlbar@ > #forward-button:-moz-locale-dir(rtl), >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper:-moz-locale-dir(rtl), >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar:-moz-locale-dir(rtl) { > /* let osx-urlbar-back-button-clip-path clip the urlbar's right side for RTL */ > transform: scaleX(-1); > } Why did you add #back-button and #forward-button here? >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar > #identity-box { >+@conditionalForwardWithUrlbar@[forwarddisabled] > #urlbar-wrapper > #urlbar > #identity-box { > border-radius: 0; > } Why did you add [forwarddisabled] here? >--- a/browser/themes/windows/browser.css Wed Jul 17 12:36:32 2013 +0200 >+++ b/browser/themes/windows/browser.css Wed Jul 17 15:38:54 2013 +0200 >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar > #identity-box { >+#urlbar-container > #urlbar-wrapper > #urlbar > #identity-box { > border-radius: 0; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { >+#urlbar-container[forwarddisabled] > #urlbar-wrapper > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { > padding-left: 5px; > transition: padding-left; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { >+#urlbar-container[forwarddisabled] > #urlbar-wrapper > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { > padding-right: 5px; > transition: padding-right; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled]:hover:not([switchingtabs]) + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box { >+#urlbar-container[forwarddisabled]:hover:not([switchingtabs]) > #urlbar-wrapper > #urlbar > #notification-popup-box[hidden] + #identity-box { > /* forward button hiding is delayed when hovered */ > transition-delay: 100s; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled]:not(:hover) + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { >+#urlbar-container[forwarddisabled]:not(:hover) > #urlbar-wrapper > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(ltr) { > /* when not hovered anymore, trigger a new non-delayed transition to react to the forward button hiding */ > padding-left: 5.01px; > } > >-@conditionalForwardWithUrlbar@[forwarddisabled]:not(:hover) + #urlbar-container > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { >+#urlbar-container[forwarddisabled]:not(:hover) > #urlbar-wrapper > #urlbar > #notification-popup-box[hidden] + #identity-box:-moz-locale-dir(rtl) { > /* when not hovered anymore, trigger a new non-delayed transition to react to the forward button hiding */ > padding-right: 5.01px; > } >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar > #identity-box > #page-proxy-favicon { >+#urlbar-container > #urlbar-wrapper > #urlbar > #identity-box > #page-proxy-favicon { > -moz-margin-end: 1px; > } >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container > #urlbar > #notification-popup-box { >+#urlbar-container[forwarddisabled] > #urlbar-wrapper > #urlbar > #notification-popup-box { > padding-left: 5px; > } As I understand it, you need to keep using @conditionalForwardWithUrlbar@ in the above rules.

OK, that's it. I'm going to split the patches up into 1) general/ shared adjustments (XUL, JS and shared theme stuff) 2) one patch per platform. All this back and forth is causing us both more work, which is starting to frustrate me - and probably you as well!

Comment on attachment 777688 [details] [diff] [review] Patch 1.1: Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem Spotted a problem that I didn't see before. You moved type="menu" from SetClickAndHoldHandlers directly into browser.xul. SetClickAndHoldHandlers isn't always called, though, and type="menu" shouldn't be there when it's not called. Also, you should remove SetClickAndHoldHandlers() from BrowserToolboxCustomizeDone because the back/forward buttons can't be customized away anymore. I suppose this also means you can get rid of _clickHandlersAttached.

(In reply to Dão Gottwald [:dao] from comment #73) > Comment on attachment 777688 [details] [diff] [review] > Patch 1.1: Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 > toolbaritem > > Spotted a problem that I didn't see before. You moved type="menu" from > SetClickAndHoldHandlers directly into browser.xul. SetClickAndHoldHandlers > isn't always called, though, and type="menu" shouldn't be there when it's > not called. Also, you should remove SetClickAndHoldHandlers() from > BrowserToolboxCustomizeDone because the back/forward buttons can't be > customized away anymore. I suppose this also means you can get rid of > _clickHandlersAttached. Splitting the patches already pays off! :)

Comment on attachment 777690 [details] [diff] [review] Patch 2.1: Linux theme changes > #forward-button[disabled] { > list-style-image: url("moz-icon://stock/gtk-go-forward-ltr?size=toolbar&state=disabled"); > } >+#forward-button[disabled] { > transform: scale(0); > opacity: 0; > pointer-events: none; > } These two rules can be merged.

Note for the OSX patch: I added transform: scaleX(-1) to the back- and forward-button as well, because otherwise the clip-path would not be correctly applied and the arrow icons also need to be flipped in RTL mode. If I scale the entire urlbar-container, the button positions are also flipped, which means that the buttons remain on the left side of the urlbar. This way I could achieve all desired effects (including the urlbar focusring running all the way through) with the least amount of code changes. I think.

Comment on attachment 777707 [details] [diff] [review] Patch 1.2: Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >- // Bug 414797: Clone unified-back-forward-button's context menu into both the >+ // Bug 414797: Clone back-button's context menu into both the > // back and the forward buttons. "Clone the back/forward buttons' context menu into both buttons." >+ if (backButton && !backButton._clickHandlersAttached) { >+ backButton._clickHandlersAttached = true; Is any of this still needed? >+ let popup = document.getElementById("backForwardMenu"); > backButton.setAttribute("type", "menu"); > backButton.appendChild(popup); > _addClickAndHoldListenersOnElement(backButton); > > let forwardButton = document.getElementById("forward-button"); > popup = popup.cloneNode(true); >+ popup.removeAttribute("id"); >+ // Prevent the context attribute on back-button from being >+ // inherited. >+ popup.setAttribute("context", ""); You need to move the id removal and context attribute setting back to where it was, because they're needed for both buttons. >- <toolbaritem id="unified-back-forward-button" class="chromeclass-toolbar-additional" >- context="backForwardMenu" removable="false" >- forwarddisabled="true" >- title="&backForwardItem.title;" >- nooverflow="true"> >- <toolbarbutton id="back-button" class="toolbarbutton-1" >+ <toolbaritem id="urlbar-container" align="center" flex="400" persist="width" >+ forwarddisabled="true" title="&locationItem.title;" removable="false" >+ class="chromeclass-location" nooverflow="true"> >+ <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > label="&backCmd.label;" > command="Browser:BackOrBackDuplicate" > onclick="checkForMiddleClick(this, event);" >- tooltip="back-button-tooltip"/> >- <toolbarbutton id="forward-button" class="toolbarbutton-1" >+ tooltip="back-button-tooltip" >+ context="backForwardMenu"/> >+ <toolbarbutton id="forward-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > label="&forwardCmd.label;" > command="Browser:ForwardOrForwardDuplicate" > onclick="checkForMiddleClick(this, event);" > tooltip="forward-button-tooltip"/> The forward button needs context="backForwardMenu" too.

Carrying over r=dao.

Reverted to previous clip-path for OSX.

Grokked clip-path nao.

Comment on attachment 777729 [details] [diff] [review] Patch 4.1: Windows theme changes >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper { >+ -moz-box-align: stretch; > pointer-events: none; > } What's -moz-box-align: stretch doing here? >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar { >- -moz-border-start: none; >- margin-left: 0; >+#back-button, >+#forward-button, >+#urlbar { > pointer-events: all; > } > >-@conditionalForwardWithUrlbar@:not([switchingtabs]) + #urlbar-container > #urlbar { >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar { >+ -moz-border-start: none; >+ margin-left: 0; >+} Re-merge these two rules under the second selector. #back-button and #forward-button don't need pointer-events: all, since you moved pointer-events: none to #urlbar-wrapper. >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper { >+ padding-left: @conditionalForwardWithUrlbarWidth@px; >+ -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; >+ position: relative; >+} Re-merge this into the previous @conditionalForwardWithUrlbar@ > #urlbar-wrapper rule. >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar > #identity-box > #page-proxy-favicon { >+#urlbar-container > #urlbar-wrapper > #urlbar > #identity-box > #page-proxy-favicon { > -moz-margin-end: 1px; > } You missed this one, @conditionalForwardWithUrlbar@ needs to be kept here.

Comment on attachment 777747 [details] [diff] [review] Patch 1.4: Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >- let popup = document.getElementById("backForwardMenu").cloneNode(true); >+ let popup = document.getElementById("backForwardMenu"); You need to keep cloning the popup before you mess around with it. >+ // Prevent the context attribute on back-button from being >+ // inherited. "Prevent the back/forward buttons' context attributes from being inherited." > #ifndef XP_UNIX > <svg:clipPath id="windows-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox"> > <svg:path d="M 0,0 C 0.16,0.11 0.28,0.29 0.28,0.5 0.28,0.71 0.16,0.89 0,1 L 1,1 1,0 0,0 z"/> > </svg:clipPath> > <svg:clipPath id="windows-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse"> >- <svg:path d="M 0,0 0,7.8 C 2.5,11 4,14 4,18 4,22 2.5,25 0,28 l 0,22 10000,0 0,-50 L 0,0 z"/> >+ <svg:path d="m0,-5l0,7.8c2.5,3.2 4,6.2 4,10.2c0,4 -1.5,7 -4,10l0,22l10000,0l0,-50l-10000,0z"/> > </svg:clipPath> > #endif > #ifdef XP_MACOSX > <svg:clipPath id="osx-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox"> > <svg:path d="M 0,0 C 0.15,0.12 0.25,0.3 0.25,0.5 0.25,0.7 0.15,0.88 0,1 L 1,1 1,0 0,0 z"/> > </svg:clipPath> > <svg:clipPath id="osx-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse"> >- <svg:path d="m 0,-5 0,9.03 C 3.6,6.8 6,11.1 6,16 6,21 3.6,25 0,28 l 0,27 10000,0 0,-55 L 0,-5 z"/> >+ <svg:path d="m0,-10l0,9.03c3.6,2.77 6,7.07 6,11.97c0,5 -2.4,9 -6,12l0,27l10000,0l0,-55l-10000,-5z"/> > </svg:clipPath> > #endif So these changes only affect the vertical position of the paths since horizontally this bug shouldn't make a difference, right?

Comment on attachment 777748 [details] [diff] [review] Patch 3.2: OSX theme changes >-@conditionalForwardWithUrlbar@ + #urlbar-container { >+@conditionalForwardWithUrlbar@ { >+ pointer-events: none; >+} Like on Windows and Linux, make this @conditionalForwardWithUrlbar@ > #urlbar-wrapper. >+#back-button, >+#forward-button, >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar { >+ pointer-events: all; >+} and drop #back-button/#forward-button here >-@conditionalForwardWithUrlbar@ + #urlbar-container { >+@conditionalForwardWithUrlbar@[forwarddisabled] > #urlbar-wrapper { > padding-left: @conditionalForwardWithUrlbarWidth@px; > -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; >- position: relative; >- pointer-events: none; Why did you add [forwarddisabled] here? Why did you drop position: relative? Can you move these properties into the aforementioned @conditionalForwardWithUrlbar@ > #urlbar-wrapper rule where you set pointer-events: none? >-@conditionalForwardWithUrlbar@ + #urlbar-container:-moz-locale-dir(rtl), >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar:-moz-locale-dir(rtl) { >+@conditionalForwardWithUrlbar@ > #back-button:-moz-locale-dir(rtl), >+@conditionalForwardWithUrlbar@ > #forward-button:-moz-locale-dir(rtl), >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper:-moz-locale-dir(rtl), >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar:-moz-locale-dir(rtl) { > /* let osx-urlbar-back-button-clip-path clip the urlbar's right side for RTL */ > transform: scaleX(-1); > } It's still unclear to me why you added #back-button and #forward-button to this rule.

> So these changes only affect the vertical position of the paths since > horizontally this bug shouldn't make a difference, right? Indeed.

Comment on attachment 777833 [details] [diff] [review] Patch 1.5: Merge Back/ Forward/ URLBar/ Stop/ Reload/ Go buttons into 1 toolbaritem >- <svg:path d="M 0,0 0,7.8 C 2.5,11 4,14 4,18 4,22 2.5,25 0,28 l 0,22 10000,0 0,-50 L 0,0 z"/> >+ <svg:path d="m0,-5l0,7.8c2.5,3.2 4,6.2 4,10.2c0,4 -1.5,7 -4,10l0,22l10000,0l0,-50l-10000,0z"/> >- <svg:path d="m 0,-5 0,9.03 C 3.6,6.8 6,11.1 6,16 6,21 3.6,25 0,28 l 0,27 10000,0 0,-55 L 0,-5 z"/> >+ <svg:path d="m0,-10l0,9.03c3.6,2.77 6,7.07 6,11.97c0,5 -2.4,9 -6,12l0,27l10000,0l0,-55l-10000,-5z"/> I keep reading "l" as "1" here. Can you add back the spaces around the letters to make this more readable?

Made SVG paths more readable. Carrying over r=dao.

Comment on attachment 777856 [details] [diff] [review] Patch 3.3: OSX theme changes > #back-button:-moz-lwtheme, > #forward-button:-moz-lwtheme { > background-origin: border-box; > border: 1px solid rgba(0,0,0,0.4); >- border-radius: @toolbarbuttonCornerRadius@; > box-shadow: inset 0 1px rgba(255,255,255,0.3), 0 1px rgba(255,255,255,0.2); > } Since you removed the border-radius here... >+#back-button:not(:-moz-lwtheme) { > height: 31px; > padding: 4px 5px 5px 3px; > margin-bottom: -1px; > background: url(chrome://browser/skin/keyhole-circle.png) 0 0 no-repeat; > border-radius: 0; > } ... it's not needed any longer here, is it? > #forward-button { > -moz-margin-start: 0; >-} >- >-#nav-bar #forward-button > .toolbarbutton-icon { >+ -moz-margin-end: 0; >+ border-radius: 0; >+ clip-path: url(chrome://browser/content/browser.xul#osx-keyhole-forward-clip-path); >+} and here? > #forward-button:-moz-lwtheme { > border-top-left-radius: 0; > border-bottom-left-radius: 0; >+ -moz-padding-start: 2px; >+ -moz-padding-end: 0; > } and here? >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper { >+ position: relative; >+ pointer-events: none; >+} >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper { > padding-left: @conditionalForwardWithUrlbarWidth@px; > -moz-margin-start: -@conditionalForwardWithUrlbarWidth@px; As requested in my previous comment, merge this into the first @conditionalForwardWithUrlbar@ > #urlbar-wrapper rule. (This should help with making the patch more sane here. It seems to be moving around lots of lines due to that rule having been split.) >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper { [...] >-@conditionalForwardWithUrlbar@[forwarddisabled] + #urlbar-container { > clip-path: url("chrome://browser/content/browser.xul#osx-urlbar-back-button-clip-path"); > } As I understand it, we want [forwarddisabled] for the clip-path.

Comment on attachment 777885 [details] [diff] [review] Patch 3.4: OSX theme changes > /* Conditional forward button styles */ nit: remove this comment, as the forward button is always conditional now

Carrying over r=dao.

Comment on attachment 777887 [details] [diff] [review] Patch 4.2: Windows theme changes >-@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar { >- -moz-border-start: none; >- margin-left: 0; >+#urlbar { > pointer-events: all; > } > >-@conditionalForwardWithUrlbar@:not([switchingtabs]) + #urlbar-container > #urlbar { >+@conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar { >+ -moz-border-start: none; >+ margin-left: 0; >+} Put -moz-border-start, margin-left and pointer-events in one rule like it was befor, with @conditionalForwardWithUrlbar@ > #urlbar-wrapper > #urlbar as the selector. >+#urlbar-container { >+ -moz-box-orient: horizontal; >+ -moz-box-align: stretch; >+} >-#urlbar-container { >- -moz-box-orient: horizontal; >- -moz-box-align: stretch; >-} Why move this around?

Comment on attachment 777893 [details] [diff] [review] Patch 4.3: Windows theme changes nice

O...M...G... \o/ Alright, everyone: hug Dão!

Folded patch, containing 1.6, 2.2, 3.5, 4.3. r=dao

I understand simplifying firefox, but why remove customization this far, this makes firefox as unusable as chrome. I agree with comment 5 (In reply to Jared Wein [:jaws] from comment #5) > We're going to allow users to keep them separate for now, but critical items > like these will not be allowed to be removed from the navigation toolbar. And think the above should be in firefox, i find if this bug is put into firefox it will kill maybe Firefox's major point, customization. I would have to say this merge would be horrible.

I honestly think this is a stupid idea. As is almost all the other Australis stuff, like letting FF draw its own window frame. Is "Rearranging the buttons without an obvious way to put them back together outside of resetting the entire toolbar" really a reason to take away the ability to hide certain controls and move them elsewhere in the interface? I think it's more of a reason to tell that user to stop using Firefox and go to Chrome. Or even better yet, tell him to stop using a computer altogether. Even babies can put things into a following order. I really don't see how the lack of brains in some users justifies treating all your users like idiots and rejecting all your users who DO know how to work with Firefox. This suggestion and contradictory statements like "we're going to let FF draw its own window frame to increase UI consistency" (have you considered the fact that that would make Firefox the only program of the 200 on my computer that uses its own, non-native, eye-soringly system-inconsistent window frame?) are economical/political/whatever suicide. FF23 is bad enough already, but it's livable. Anything beyond it is just insulting to anyone who has ever loved Firefox for what it used to be. And for what? Because some idiots decided to start using Firefox and they're too inept to know what they're doing.

Just use Palemoon or Cyberfox, Firefox turns into a Chrome Clone anyway. No matter how much you post here, it will not change anything. But if Mozilla gets hit enough in Marketshare, THAT they will recognize for sure! But venting in Bugzilla changes nothing. Switch Browsers and show Mozilla your opinion... THIS helps :) I have already done it.

(In reply to saphirjd from comment #105) > Just use Palemoon or Cyberfox, Firefox turns into a Chrome Clone anyway. No > matter how much you post here, it will not change anything. > > But if Mozilla gets hit enough in Marketshare, THAT they will recognize for > sure! But venting in Bugzilla changes nothing. Switch Browsers and show > Mozilla your opinion... THIS helps :) I have already done it. Yeah, I switched back to Palemoon about half an hour before your post. I used it before but switched back to official FF because I distrust the quality, lifespan, etc of forked projects. But with things as they are lately, my dislike of Mozilla's plans outweighs. Goodbye Firefox and good riddance

Well, you can trust a Browser like Palemoon much more as these Days Mozilla. In my Opinion a Browser Company which just sees the solution for all their problems in recreating another Browser... That is just... bad :) Anyway, was my last post here. I just wanted to say that i will try to make as many people switch away from Firefox as possible. Mozilla can not be trusted anymore. Good Bye and best luck with your own Version of Chrome!

This fix causes a usability problem because the reload button now is *too small*. It becomes one of the smallest button of the UI when instead is one of the most used. Please make it customizable, decisions like this one in this bug are really unintelligible, don't have any logic sense. Forcing the things is always a bad idea, users don't appreciate that.

Reload button too small! An option to move reload button afer navigation button with normal size needed! Do you want force users to use F5? Okay then, i'll use F5, but my girlfriend doesnt. She still using Chrome because of bad reload button positioning in australis.

So, any chance of movement on this? I'd really appreciate being able to move the reload/stop button. The button is far too difficult to hit, and it is far away from the other navi buttons.

to comment 111: https://addons.mozilla.org/en-US/firefox/addon/classicthemerestorer This should get you sorted. HOWEVER, this is another proof how silly another decision from so-called QA (congrats Phillip, you were right! :-P (cf comment 1)) actually was. Because it should not be necessary that we need a third-party add-on to get the reload button back where it belongs (i. e. left of the Location Bar). This is so pathetic.