592676 - (Tabs On Bottom) Toolbars background is missing gradient in upper part

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Peter Henkel [:Terepin]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre

See mockup.

Reproducible: Always

Comment 1

[Johnathan Nightingale [:johnath]]

Shorlander - I assert this is nice-to-have, but non-blocking. Disagree?

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to comment #1)
> Shorlander - I assert this is nice-to-have, but non-blocking. Disagree?

I was going to say "agreed" but then I looked at it and it is looking pretty bad at the moment. Bug 589259 and 588764 might mitigate that some but I don't think we should ship it the way it looks right now.

Comment 3

[pino_sb]

I'd say the correct code would be:

#main-window[sizemode="normal"] #navigator-toolbox[tabsontop="false"] > toolbar:not(#toolbar-menubar):not(#nav-bar):not([inFullscreen="true"]):not(:-moz-lwtheme) {
  border-left: 1px solid rgba(10%,10%,10%,.4);
  border-right: 1px solid rgba(10%,10%,10%,.4);
}

#nav-bar + #customToolbars + #PersonalToolbar[collapsed="true"] + #TabsToolbar[tabsontop="false"]:last-child:not(:-moz-lwtheme) {
  border-left: none !important;
  border-right: none !important;
}
  
#navigator-toolbox[tabsontop="false"] > #PersonalToolbar:not(:-moz-lwtheme) {
  border-top: 1px solid rgba(10%,10%,10%,.4) !important;
  background-image: -moz-linear-gradient(rgba(255,255,255,.5), rgba(255,255,255,0));
}

This adds a border and a gradient. The colors are hardcoded in my code, which is wrong I believe. I don't know how to create patches though, so this is merely the code, it might help...

An additional note: Adding custom toolbars has severe problems with tabs on bottom; there is a gap between custom bars and the bookmarks bar, the border-radius is applied to the bookmarks toolbar, etc. I tried to add the gradient and border to the correct toolbar (the one below nav-bar), but I couldn't get it done. So now it only worsens the problems, but it seems hard if not impossible to fix in css.

Comment 4

[pino_sb]

Attachment: before and after

Here is a screenshot how old(left) and new(right) look. Compared to the mockup, the border color should be a little lighter maybe.

Comment 5

[Justin Dolske [:Dolske]]

Unowned blocker, over to Frank.

Comment 6

[Frank Yan (:fryn)]

Attachment: patch [bitrotted]

The patch uses parts of the code suggested by [e-mail] (thanks, pino_sb!) and implements the look shown in the attachment "before and after". It also makes sure that the UI doesn't move around when previewing/applying a persona.

Comment 7

[Frank Yan (:fryn)]

Attachment: patch

Comment 8

[Frank Yan (:fryn)]

Attachment: patch v2

Stephen suggested that the bottom border of the tabs toolbar (aka the top border of the content area) (aka #navigator-toolbox::after) be opaque also in tabs-on-bottom mode and that the top left/right corner pixels remain glassed. Patch v2 addresses this.

Comment 9

[Valerio]

Attachment: Toolbar detached

with this patch applied, if I add a new toolbar from the window "Customize Toolbar", this remains detached from the the other toolbars

Comment 10

[pino_sb]

(In reply to comment #9)
> Created attachment 507065 [details]
> Toolbar detached
> 
> with this patch applied, if I add a new toolbar from the window "Customize
> Toolbar", this remains detached from the the other toolbars

Yea, that's an issue I mentioned in comment 3 as well. It is not caused by this patch however. The same thing happens without it. 

It seems noone has thought about the possibility of custom toolbars, since a lot of css is flawed when you use them with tabs on bottom (having a custom toolbar hidden can still affect looks for example). I did try to make the code for this bug work with custom toolbars, but I didn't manage. The background of the tab bar depends on the existence of toolbars above it, but with custom toolbars there are way too many possibilities to adjust for.

Comment 11

[Valerio]

have you tried to change the value of -moz-box-ordinal-group for the toolbars? the correct order should be:
- menubar
- nav-bar
- bookmarks-toolbar
- custom-toolbar
- tabs toolbar

Comment 12

[Frank Yan (:fryn)]

(In reply to comment #9)
> Created attachment 507065 [details]
> Toolbar detached
> 
> with this patch applied, if I add a new toolbar from the window "Customize
> Toolbar", this remains detached from the the other toolbars

(In reply to comment #11)
> have you tried to change the value of -moz-box-ordinal-group for the toolbars?

I think both of these issues should be combined into a separate bug entitled something like "(tabs on bottom) custom toolbars should be grouped between bookmarks toolbar and tabs toolbar". I don't know of an existing bug that addresses them, so please file one, and make it depend on this bug.

Comment 13

[Valerio]

I have filed bug 629339 for the position of the toolbar

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 506647 [details] [diff] [review]
patch v2

>   #toolbar-menubar:not(:-moz-lwtheme),
>   #TabsToolbar[tabsontop="true"]:not(:-moz-lwtheme),
>   #navigator-toolbox[tabsontop="false"] > #nav-bar:not(:-moz-lwtheme),
>   #nav-bar + #customToolbars + #PersonalToolbar[collapsed="true"] + #TabsToolbar[tabsontop="false"]:last-child:not(:-moz-lwtheme) {
>     background: transparent !important;
>     color: black;
>     text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
>   }
>+  #nav-bar + #customToolbars + #PersonalToolbar[collapsed="true"] + #TabsToolbar[tabsontop="false"]:last-child:not(:-moz-lwtheme) {
>+    border-left: none !important;
>+    border-right: none !important;
>+  }

It looks like it might be possible to merge this with the rule above it.

nit: use border-*-style

>+  #main-window[sizemode="normal"] #navigator-toolbox[tabsontop="true"] > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not([inFullscreen="true"]):-moz-lwtheme,
>+  #main-window[sizemode="normal"] #navigator-toolbox[tabsontop="false"] > toolbar:not(#toolbar-menubar):not(#nav-bar):not([inFullscreen="true"]):-moz-lwtheme {
>+    border-color: transparent !important;
>+  }

Why not just #main-window[sizemode=normal] #navigator-toolbox > toolbar:-moz-lwtheme?

>-  #navigator-toolbox[tabsontop="false"]::after,
>   #main-window[disablechrome] #navigator-toolbox[tabsontop="true"]::after {
>     background-color: @glassToolbarBorderColor@;
>   }

Given the current direction of bug 631698, can you avoid this change for now?

>   #navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar {
>     padding-top: 0 !important;
>-    margin-top: 3px;
>+    margin-top: 2px;
>+  }
>+  #navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar:not(:-moz-lwtheme) {
>+    border-top: 1px solid @glassToolbarBorderColor@ !important;
>+    background-image: -moz-linear-gradient(rgba(255,255,255,.5), rgba(255,255,255,0));
>+  }
>+  #navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar:-moz-lwtheme {
>+    border-top: 1px solid transparent !important;
>   }

I think this would be simpler this way:

#navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar {
  padding-top: 0 !important;
  margin-top: 3px;
}
#navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar:not(:-moz-lwtheme) {
  margin-top: 2px;
  border-top: 1px solid @glassToolbarBorderColor@ !important;
  background-image: -moz-linear-gradient(rgba(255,255,255,.5), rgba(255,255,255,0));
}

And you should use @toolbarHighlight@ instead of rgba(255,255,255,.5).

Comment 15

[(no longer active)]

Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.

Comment 16

[Frank Yan (:fryn)]

Attachment: patch v3

Everything in comment 14 addressed.

Comment 17

[Frank Yan (:fryn)]

Attachment: patch v3

forgot to configure .hgrc
now with 8 lines of context

Comment 18

[Frank Yan (:fryn)]

er, i'll remove that accidental extra line of whitespace when pushing this if r+'d.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 511696 [details] [diff] [review]
patch v3

Given this:

>+  #main-window[sizemode=normal] #navigator-toolbox > toolbar:-moz-lwtheme {
>+    border-color: transparent !important;
>+  }

Why not apply the border regardless of :-moz-lwtheme (and without !important):

>   #navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar {
>     padding-top: 0 !important;
>     margin-top: 3px;
>   }
>+  #navigator-toolbox:not([tabsontop="true"]) > #PersonalToolbar:not(:-moz-lwtheme) {
>+    margin-top: 2px;
>+    border-top: 1px solid @toolbarShadowColor@ !important;
>+    background-image: -moz-linear-gradient(@toolbarHighlight@, rgba(255,255,255,0));
>+  }

Also, you can use [tabsontop=false] here and [inFullscreen] instead of [inFullscreen=true] elsewhere.

Comment 20

[Frank Yan (:fryn)]

Attachment: patch v4

Comments addressed.

In fullscreen mode, #main-window[sizemode=fullscreen] always seems to be true. Why do we need :not([inFullscreen]) with [sizemode=normal] or [sizemode=maximized] ?

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> In fullscreen mode, #main-window[sizemode=fullscreen] always seems to be true.
> Why do we need :not([inFullscreen]) with [sizemode=normal] or
> [sizemode=maximized] ?

It makes going fullscreen less jarring.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 511806 [details] [diff] [review]
patch v4

>+  #navigator-toolbox[tabsontop=false] > #PersonalToolbar {
>     padding-top: 0 !important;
>-    margin-top: 3px;
>+    margin-top: 2px;
>+    border-top: 1px solid @toolbarShadowColor@;
>+    background-image: -moz-linear-gradient(@toolbarHighlight@, rgba(255,255,255,0));
>   }

Doesn't this apply the gradient when using a persona?

Comment 23

[Frank Yan (:fryn)]

Attachment: patch v5

Comment 24

[Frank Yan (:fryn)]

Pushed.

http://hg.mozilla.org/mozilla-central/rev/2a4dc060de2b

Comment 25

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre

Verified issue and it's no longer present.

Comment 26

[Christian Ascheberg]

#navigator-toolbox[tabsontop=false] > #PersonalToolbar {
  padding-top: 0 !important;
  margin-top: 2px;
  border-top: 1px solid @toolbarShadowColor@;
}

In the following config, the above border-top part of the patch adds a line that should not be there:
bookmarks toolbar + tabsontop false + personas used + maximized window

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #26)
Please file a new bug.

Comment 29

[Vlad [QA]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre