606735 - aero basic tabstrip picking up the wrong color.

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Asa Dotzler [:asa]]

Attachment: current state screenshot on top. adjusted on bottom

Ideally the tabstrip background color would blend with the titlebar color. Right now it looks kinda bad.

Comment 1

[Asa Dotzler [:asa]]

I believe that the current look works fine for the tabs on bottom case but not for the tabs on top (which I believe is the default). Hopefully we can get it right for both cases.

Comment 2

[Asa Dotzler [:asa]]

The closer I look at this, the more I think that for tabs on top, the tab strip simply needs to be transparent like it is for the glass theme. The window frame color/gradient is correct behind it.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> The closer I look at this, the more I think that for tabs on top, the tab strip
> simply needs to be transparent like it is for the glass theme. The window frame
> color/gradient is correct behind it.

There's just gray behind the tab strip, and I don't think the color we'd want her is currently accessible or covered by native rendering.

Comment 4

[Valerio]

Attachment: correct aero basic color

I made this and using stylish works well:

@media all and (-moz-windows-default-theme) {
#navigator-toolbox[tabsontop="false"] > toolbar:not(:-moz-lwtheme), #TabsToolbar[tabsontop="true"]:not(:-moz-lwtheme) {
  background: rgb(185,209,234) !important;
}

#navigator-toolbox[tabsontop="false"] > toolbar:-moz-window-inactive:not(:-moz-lwtheme),#TabsToolbar[tabsontop="true"]:-moz-window-inactive:not(:-moz-lwtheme) {
  background: rgb(215,228,242) !important;
}
}

@media all and (-moz-windows-compositor) {
#navigator-toolbox[tabsontop="false"] > toolbar:not(:-moz-lwtheme), #TabsToolbar[tabsontop="true"]:not(:-moz-lwtheme)  {
  background: transparent !important;
}

#navigator-toolbox[tabsontop="false"] > toolbar:-moz-window-inactive:not(:-moz-lwtheme),#TabsToolbar[tabsontop="true"]:-moz-window-inactive:not(:-moz-lwtheme) {
  background: transparent !important;
}
}

Comment 5

[Marco B.]

This should block final in my view. The Aero Basic theme is negligible on normal desktop systems, but don't forget the netbooks. Nearly every Windows 7 netbook runs with Aero Basic, instead of Aero Glass.

Comment 6

[Dão Gottwald [:dao]]

Attachment: work-in-progress patch

Comment 7

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

Hard to notice on Aero Basic machines, would approve a patch, but not gonna block on it.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

Comment 9

[:Felipe Gomes (needinfo for replies!)]

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

This looks beautiful!

To simplify these rules a bit I'd really vote for adding an !important rule for the menubar-autohide binding in xul.css
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#279
This would get rid of all the autohide selectors here and it doesn't seem wrong to me (helps avoid a menu with the autohide attribute and a possible incorrect binding)

I understand though if you prefer not to make xul.css changes for this. In this case, we can still get rid of the autohides by dropping the first rule:

+  #toolbar-menubar:not([autohide=true]):not(:-moz-lwtheme):-moz-system-metric(windows-default-theme),

as we don't support custom drag areas for windows without custom titlebar drawing yet.

I didn't find a situation where the !important markers for the background-color here are required.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> To simplify these rules a bit I'd really vote for adding an !important rule for
> the menubar-autohide binding in xul.css
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#279
> This would get rid of all the autohide selectors here and it doesn't seem wrong
> to me (helps avoid a menu with the autohide attribute and a possible incorrect
> binding)
> 
> I understand though if you prefer not to make xul.css changes for this.

I'd rather not. We usually don't override or extend the autohide binding, but somebody else might want to.

> In this
> case, we can still get rid of the autohides by dropping the first rule:
> 
> + 
> #toolbar-menubar:not([autohide=true]):not(:-moz-lwtheme):-moz-system-metric(windows-default-theme),
> 
> as we don't support custom drag areas for windows without custom titlebar
> drawing yet.

That's a pretty annoying bug (it affects the stock downloads manager), and I don't think we should remove correct selectors for it. It's also sort of a regression, as this used to work at least without aero snap support.

> I didn't find a situation where the !important markers for the background-color
> here are required.

I'll look into this.

Comment 11

[Dão Gottwald [:dao]]

Ok, !important isn't needed for the background-color. I think we should keep the rest as is, though.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

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

Alright then let's just remove the !important from the background-color

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch

removed !important

Comment 14

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

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

a=beltzner

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a4784c581b3f

Comment 16

[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.