Closed Bug 592676 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Terepin, Assigned: fryn)

References

(Blocks 1 open bug, )

Details

(Keywords: polish, Whiteboard: [softblocker])

Attachments

(1 file, 8 obsolete files)

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
Blocks: 568037
blocking2.0: --- → ?
Summary: (Tabs At Bottom) Toolbars background is missing gradient in upper part → (Tabs On Bottom) Toolbars background is missing gradient in upper part
Version: unspecified → Trunk
Shorlander - I assert this is nice-to-have, but non-blocking. Disagree?
blocking2.0: ? → -
(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.
blocking2.0: - → betaN+
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.
Attached image before and after (obsolete) —
Here is a screenshot how old(left) and new(right) look. Compared to the mockup, the border color should be a little lighter maybe.
Keywords: regression
Keywords: regressionpolish
Unowned blocker, over to Frank.
Assignee: nobody → fryn
Whiteboard: [softblocker]
Attachment #495242 - Attachment description: what code does → before and after
Attached patch patch [bitrotted] (obsolete) — Splinter Review
The patch uses parts of the code suggested by pino_sb@live.com (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.
Attachment #506065 - Flags: ui-review?(shorlander)
Attachment #506065 - Flags: review?(dao)
Attached patch patch (obsolete) — Splinter Review
Attachment #506123 - Flags: review?(dao)
Attachment #506065 - Attachment description: patch → patch [bitrotted]
Attachment #506065 - Attachment is obsolete: true
Attachment #506065 - Flags: ui-review?(shorlander)
Attachment #506065 - Flags: review?(dao)
Attachment #506123 - Attachment description: same patch except it applies on top of bug 627324's patch → patch
Attachment #506123 - Attachment filename: gradient-alt → gradient
Attachment #506123 - Flags: ui-review?(shorlander)
Status: NEW → ASSIGNED
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #495242 - Attachment is obsolete: true
Attachment #506123 - Attachment is obsolete: true
Attachment #506647 - Flags: ui-review?(shorlander)
Attachment #506647 - Flags: review?(dao)
Attachment #506123 - Flags: ui-review?(shorlander)
Attachment #506123 - Flags: review?(dao)
Attached image Toolbar detached (obsolete) —
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 #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.
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
(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.
Blocks: 629339
I have filed bug 629339 for the position of the toolbar
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).
Attachment #506647 - Flags: review?(dao) → review-
Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.
Whiteboard: [softblocker] → [softblocker][final?]
Attached patch patch v3 (obsolete) — Splinter Review
Everything in comment 14 addressed.
Attachment #506647 - Attachment is obsolete: true
Attachment #507065 - Attachment is obsolete: true
Attachment #511694 - Flags: review?(dao)
Attachment #506647 - Flags: ui-review?(shorlander)
Attached patch patch v3 (obsolete) — Splinter Review
forgot to configure .hgrc
now with 8 lines of context
Attachment #511694 - Attachment is obsolete: true
Attachment #511696 - Flags: review?(dao)
Attachment #511694 - Flags: review?(dao)
er, i'll remove that accidental extra line of whitespace when pushing this if r+'d.
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.
Attached patch patch v4 (obsolete) — Splinter Review
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] ?
Attachment #511696 - Attachment is obsolete: true
Attachment #511806 - Flags: review?(dao)
Attachment #511696 - Flags: review?(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 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?
Attached patch patch v5Splinter Review
Attachment #511806 - Attachment is obsolete: true
Attachment #511813 - Flags: review?(dao)
Attachment #511806 - Flags: review?(dao)
Attachment #511813 - Flags: review?(dao) → review+
Pushed.

http://hg.mozilla.org/mozilla-central/rev/2a4dc060de2b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker][final?] → [softblocker]
Target Milestone: --- → Firefox 4.0b12
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre

Verified issue and it's no longer present.
Status: RESOLVED → VERIFIED
#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
(In reply to comment #26)
Please file a new bug.
Depends on: 637212
Duplicate of this bug: 576061
Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
You need to log in before you can comment on or make changes to this bug.