+++ This bug was initially created as a clone of Bug #983940 +++ This is always wrong but only actually visible with a LWT applied.

Comment on attachment 8399839 [details] [diff] [review] v.1 Use margin-bottom: tabToolbarNavbarOverlap to fix the position >--- a/browser/themes/linux/browser.css >+++ b/browser/themes/linux/browser.css >@@ -1835,17 +1835,16 @@ richlistitem[type~="action"][actiontype= > use evil CSS to give the impression of smaller content */ > margin: -2px; > } > > /* Tabbrowser arrowscrollbox arrows */ > .tabbrowser-arrowscrollbox > .scrollbutton-up, > .tabbrowser-arrowscrollbox > .scrollbutton-down { > -moz-appearance: none; >- margin: 0; > } I don't think moving this single property to shared/ is a win if that means adding a separate rule for it. >+.tabbrowser-arrowscrollbox > .scrollbutton-up, >+.tabbrowser-arrowscrollbox > .scrollbutton-down { >+ margin: 0 0 @tabToolbarNavbarOverlap@ 0; >+} margin: 0 0 @tabToolbarNavbarOverlap@;

Addressed review comments

Comment on attachment 8399849 [details] [diff] [review] v.2 Don't move to shared and use 3-value margin syntax >--- a/browser/themes/shared/tabs.inc.css >+++ b/browser/themes/shared/tabs.inc.css >@@ -3,16 +3,20 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > %endif > > %define tabHeight 31px > %define tabCurveWidth 30px > %define tabCurveHalfWidth 15px > >+#TabsToolbar .toolbarbutton-1 { >+ margin-bottom: @tabToolbarNavbarOverlap@ !important; >+} Don't really think this belongs here. All other rules in tabs.inc.css are about #tabbrowser-tabs and its contents.

(In reply to Dão Gottwald [:dao] from comment #4) > Comment on attachment 8399849 [details] [diff] [review] > v.2 Don't move to shared and use 3-value margin syntax > > >--- a/browser/themes/shared/tabs.inc.css > >+++ b/browser/themes/shared/tabs.inc.css > >+#TabsToolbar .toolbarbutton-1 { > >+ margin-bottom: @tabToolbarNavbarOverlap@ !important; > >+} > > Don't really think this belongs here. All other rules in tabs.inc.css are > about #tabbrowser-tabs and its contents. I also thought about that but I didn't find a better place for it as I'd rather not duplicate it in all 3 browser.css as they end up diverging by mistake sometimes. Do you have a another idea?

(In reply to Matthew N. [:MattN] from comment #5) > I also thought about that but I didn't find a better place for it as I'd > rather not duplicate it in all 3 browser.css as they end up diverging by > mistake sometimes. Sure, but this rule doesn't seem particularly prone to that. Also, moving it to the right spot in browser.css would allow you to drop !important, wouldn't it? Otherwise I don't understand why !important is there.

(In reply to Dão Gottwald [:dao] from comment #6) > (In reply to Matthew N. [:MattN] from comment #5) > > I also thought about that but I didn't find a better place for it as I'd > > rather not duplicate it in all 3 browser.css as they end up diverging by > > mistake sometimes. > > Sure, but this rule doesn't seem particularly prone to that. Also, moving it > to the right spot in browser.css would allow you to drop !important, > wouldn't it? Otherwise I don't understand why !important is there. OK, I moved them near other #TabsToolbar .toolbarbutton-1 rules in the three themes. The !important was leftover from before I removed the -moz-any workaround !important I think. Pushed: https://hg.mozilla.org/integration/fx-team/rev/c4dbb3c39b33

Comment on attachment 8399849 [details] [diff] [review] v.2 Don't move to shared and use 3-value margin syntax [Approval Request Comment] Bug caused by (feature/regressing bug #): Not sure exactly but part of Australis (possibly initial tab overlap design) User impact if declined: Toolbar buttons will extend 1px too low (only noticeable with a lightweight theme) see attachment 8399798 [details]. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): margin change of 1px which should be low risk String or IDL/UUID changes made by this patch: None

Shouldn't the negative margin on the nav toolbar be set on the tabs box instead? This would avoid bugs like this and bug 989767. These wallpaper patches are generating a large amount of unnecessary CSS code.

(In reply to henryfhchan from comment #10) > Shouldn't the negative margin on the nav toolbar be set on the tabs box > instead? This would avoid bugs like this and bug 989767. These wallpaper > patches are generating a large amount of unnecessary CSS code. If I understand correctly, we would just have the opposite problem where items on the nav-bar would need a margin-top of @tabToolbarNavbarOverlap@ so it wouldn't be much different.

I don't quite get what you mean, but I doubt what you say isn't true. In the current approach, margin top is set on the nav-toolbar, thus anything on the tabbar will overlap with nav-toolbar's border, hence this bug. A better approach would be to stop setting a margin top on the nav-toolbar and directly set a margin bottom or transform on the box for tabs (excluding the customization targets). This saves the unnecessary code where we now have to further offset all existing toolbar items up a pixel to avoid the overlap. This new approach also avoids the other problem where the top border disappears in popups as the border is now outside the drawable client area due to its margin. I don't see why nav-toolbar items need a margin top, they wouldn't overlap anything for either approaches.

By box for tabs, I mean #tabbrowser-tabs. Setting a negative margin on #TabsToolbar would have the same drawbacks as the current approach. But items in #nav-bar still wouldn't be affected.

(In reply to henryfhchan from comment #13) > I don't quite get what you mean, but I doubt what you say isn't true. In the > current approach, margin top is set on the nav-toolbar, thus anything on the > tabbar will overlap with nav-toolbar's border, hence this bug. > > A better approach would be to stop setting a margin top on the nav-toolbar > and directly set a margin bottom or transform on the box for tabs (excluding > the customization targets). > > This saves the unnecessary code where we now have to further offset all > existing toolbar items up a pixel to avoid the overlap. This new approach > also avoids the other problem where the top border disappears in popups as > the border is now outside the drawable client area due to its margin. Yes, this came up before but I think we thought it was too late to fix this for 29. I agree that it is properly more maintainable as long as I'm not forgetting some other drawback. The reason I didn't do this in the initial implementation was because at that time users could still hide the nav-bar themselves and we didn't want the tabs to overlap the top one pixel of the page if it was hidden. (IIRC this also has to do with limitations of CSS sibling selectors needed to handle similar cases in that you can only style an element that comes after another, not before and we need to be able to check the hidden/collapsed state of adjacent toolbars. The CSS subject indicator proposal addresses the issue.) > I don't see why nav-toolbar items need a margin top, they wouldn't overlap > anything for either approaches. We would still want them properly vertically centered. We can handle this simplification in bug 989767. Thanks for reminding me about it. Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/81075b35ee13

The toolbar buttons are correctly shown when using a lightweight theme on: - latest Firefox Nightly (build ID: 20140407030203) - latest Firefox Aurora (build ID: 20140407004002) - Firefox 29 beta 6 (build ID: 20140407135746).