624292 - Don't completely override titlebar backgrounds (missing gradient when Firefox is maximized)

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Stephen Walker]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre

The faux native with tabs-in-titlebar isn't displaying the "squares" bitmap between the titlebar and caption buttons.

Reproducible: Always

Steps to Reproduce:
1. Patch your XP install to allow non-signed themes, http://www.uxstyle.com/, if it works or use the older uxtheme.dll based patching
2. Extract WatercolorLite.msstyles from the zip and double click
3. Compare the normal & maximized titlebars

Comment 1

[Stephen Walker]

Attachment: Normal titlebar

Comment 2

[Stephen Walker]

Attachment: Maximized titlebar

Comment 3

[7842.9]

Temporary fix, add this to userChrome.css or the Stylish extension:

  /*show normal titlebar image intstead of solid color*/
  #main-window[tabsintitlebar] #titlebar-content:not(:-moz-lwtheme),
  #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar:not(:-moz-lwtheme) {
    background-color: transparent !important;
    color: CaptionText;
  }
  #main-window[tabsintitlebar] #titlebar-content:not(:-moz-lwtheme):-moz-window-inactive,
  #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar:not(:-moz-lwtheme):-moz-window-inactive {
    background-color: transparent !important;
    color: InactiveCaptionText;
  }

Comment 4

[Stephen Walker]

Not a widget issue as originally thought.

Comment 6

[Biju]

Attachment: fx_max_gradient_n1.png

(In reply to comment #3)
> Temporary fix
works almost great but has a minor issue see attachment fx_max_gradient_n1.png

Comment 7

[Dão Gottwald [::dao]]

Attachment: patch

This is a compromise, overlaying only the bottom of the title bar.

Comment 8

[Dão Gottwald [::dao]]

Attachment: screenshots with patch

Comment 9

[Stephen Walker]

(In reply to comment #7)
> Created attachment 506215 [details] [diff] [review]
> patch
> 
> This is a compromise, overlaying only the bottom of the title bar.

Is the intended look a native titlebar (comment 3 & transparent) or not (patch & gradient)?

Comment 10

[Dão Gottwald [::dao]]

We can't go with a fully native title bar, as comment 6 indicates.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: screenshot with reporter's theme

Screenshot with reporter's theme, looks pretty good

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 506215 [details] [diff] [review]
patch

Some notes to possibly improve these styles, now or in the future:

 - do we need to do this for the Luna/Olive/Silver XP themes? Seems like their titlebar are tall enough to work correctly, at least with default settings (if we remove these backgrounds and -moz-dialog from the tab bar). Although the end result isn't very different.

 - for the classic theme with a standard left-to-right gradient, we can increase the height of #titlebar and our theme code will draw it correctly.
The problem of course is calculating how many px to increase.. Maybe something easy could come out of the tabs-in-titlebar, together with a min-height.

with these two we would only need these compromises for custom themes.

Comment 13

[Dão Gottwald [::dao]]

(In reply to comment #12)
>  - do we need to do this for the Luna/Olive/Silver XP themes? Seems like their
> titlebar are tall enough to work correctly, at least with default settings (if
> we remove these backgrounds and -moz-dialog from the tab bar). Although the end
> result isn't very different.

I think a cleaner solution would be to let the TabsInTitlebar code set an attribute when the title bar is tall enough. I'll look into this in a followup bug.

>  - for the classic theme with a standard left-to-right gradient, we can
> increase the height of #titlebar and our theme code will draw it correctly.
> The problem of course is calculating how many px to increase.. Maybe something
> easy could come out of the tabs-in-titlebar, together with a min-height.

Sounds like slightly scary followup material :)

Comment 14

[Stephen Walker]

(In reply to comment #11)
> Created attachment 506574 [details]
> screenshot with reporter's theme
> 
> Screenshot with reporter's theme, looks pretty good

Well, if by "good" you mean non-native looking, not to mention the increased titlebar height which doesn't help, which I think Firefox kicked to the curb long ago but I haven't been following things as much as I used to.

Comment 15

[Dão Gottwald [::dao]]

(In reply to comment #14)

Feel free to toggle browser.tabs.drawInTitlebar to preserve the native title bar height. Tabs in the title bar obviously won't work when the title bar isn't tall enough.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 506215 [details] [diff] [review]
patch

The comment about "scary followup" made me nervous and so I'm minusing. Feel free to ask for approval again with a clear risk/reward statement. Feels like the reward is prettttty low.

Comment 18

[Dão Gottwald [::dao]]

Comment on attachment 506215 [details] [diff] [review]
patch

This patch alone is non-scary polish. The suggested followup isn't mandatory for it. It would just be a further visual improvement.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Actually I'd like to avoid the patch due to the extra visual glitch when the menu bar is open for the XP themes.

Comment 20

[Dão Gottwald [::dao]]

We've already discussed that, right? The selector needs #toolbar-menubar[incative] ~ to take care of that case.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

That didn't really help because with that the full top area switches from blue to gray when the menu is open

Comment 22

[Dão Gottwald [::dao]]

The menu bar is gray regardless of this patch, so the blue background behind the tab bar moving down is actually pretty nonsensical and ugly.

Comment 23

[Dão Gottwald [::dao]]

Attachment: patch

updated to tip, with the selector taking into account the menubar when it's temporarily shown

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 512612 [details] [diff] [review]
patch

a=beltzner

Comment 25

[Dão Gottwald [::dao]]

http://hg.mozilla.org/mozilla-central/rev/6387a9f398f4

Comment 26

[alex_mayorga]

Thanks on the fix Dão!
I filed a polish follow up as bug 634975.

Comment 27

[Dão Gottwald [::dao]]

(In reply to comment #12)
>  - do we need to do this for the Luna/Olive/Silver XP themes? Seems like their
> titlebar are tall enough to work correctly, at least with default settings (if
> we remove these backgrounds and -moz-dialog from the tab bar). Although the end
> result isn't very different.

filed bug 635063

Comment 28

[George Carstoiu]

Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110221 Firefox/4.0b12pre

Verified issue and it's no longer present.