Closed Bug 1399498 Opened 7 years ago Closed 7 years ago

[Windows 7] Make Background Tabs Dark for Default Theme

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: shorlander, Assigned: johannh)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(1 file)

Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.
59 bytes, text/x-review-board-request
dao
: review+
Details 
Background tabs should be dark and be in a solid box not floating on the glass fog background:

http://design.firefox.com/people/shorlander/photon/Mockups/windows-7.html
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Whiteboard: [photon-visual] → [reserve-photon-visual]
Blocks: photon-tabs
No longer blocks: photon-visual
Blocks: 1381997
Dão, this patch uses elements from the changes I made in bug 1391227 and bug 1398125, but it applies cleanly on central without those patches (it will have merge conflicts with those patches, in fact).

It would probably be more practical to review the other two patches before this one, so that I can rebase and we have a more clear changelog, but I'd leave that up to you.

I'm relatively certain that this should work well on Windows 8, but I'll try it myself later.
Blocks: 1398696
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

::: browser/themes/windows/browser.css:112
(Diff revision 1)
> + * Windows 7 draws the chrome background color as the tab background
> + * instead of in the tabs toolbar.
> + */
> +@media (-moz-os-version: windows-win7) {
> +  :root:not(:-moz-lwtheme) {
> +    --chrome-background-color: hsl(235,33%,19%);

This variable is currently used only in compacttheme.css (for good reasons; the name is confusing). Can we keep it there?

::: browser/themes/windows/browser.css:120
(Diff revision 1)
> +
> +  #TabsToolbar:not(:-moz-lwtheme) {
> +    color: hsl(240,9%,98%);
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab {

Does this need :not(:-moz-lwtheme)?

::: browser/themes/windows/browser.css:131
(Diff revision 1)
> +    border-image: none !important;
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > .tab-background {
> +    border-image: none !important;
> +    border-top: 1px solid var(--tabs-border);

Please document what you're doing here

::: browser/themes/windows/browser.css:135
(Diff revision 1)
> +    border-image: none !important;
> +    border-top: 1px solid var(--tabs-border);
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab[first-tab="true"] > .tab-stack > .tab-background {
> +    border-inline-start: 1px solid var(--tabs-border) !important;

This doesn't seem right for scrolling tabs. I need to think a bit more about this, also in light of bug 1390221 and bug 1390025. Can you remove it from this patch and file a new bug if it isn't already covered by an existing bug?

::: browser/themes/windows/compacttheme.css:38
(Diff revision 1)
>  @media (-moz-windows-glass) {
> -  .tabbrowser-tab {
> -    background-color: var(--chrome-background-color);
> +  /* Always show light toolbar buttons on aero glass surface. */
> +  #main-window #TabsToolbar .toolbarbutton-1,
> +  #main-window #TabsToolbar .scrollbutton-up,
> +  #main-window #TabsToolbar .scrollbutton-down {
> +    fill: hsl(240,9%,98%) !important;

If you remove :not(:-moz-lwtheme) from the #TabsToolbar:not(:-moz-lwtheme) rule you added in browser.css, this shouldn't be needed.

::: browser/themes/windows/compacttheme.css:42
(Diff revision 1)
> +  #main-window #TabsToolbar .scrollbutton-down {
> +    fill: hsl(240,9%,98%) !important;
>    }
> +
> +  #toolbar-menubar {
> +    text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);

Why is this in compacttheme.css?
Attachment #8909276 - Flags: review?(dao+bmo) → review-
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

> This variable is currently used only in compacttheme.css (for good reasons; the name is confusing). Can we keep it there?

I don't think so, it does exactly what we need here (have a way for default and light/dark themes to set the tab background). I can rename it to something more clear if you prefer. Alternatively we could also not use a variable at all and just add the rule in line 120 to compacttheme.css. Glancing at your second comment it seems like you'd prefer that, so I'll go this route.

> Does this need :not(:-moz-lwtheme)?

Since the only lwthemes that declare this variable are light/dark theme this works fine, but as mentioned above it might be clearer if we split this up.

> This doesn't seem right for scrolling tabs. I need to think a bit more about this, also in light of bug 1390221 and bug 1390025. Can you remove it from this patch and file a new bug if it isn't already covered by an existing bug?

This is what I've been using in bug 1398125 to draw a border around light/dark theme tabs, but now all three themes need a border around them. We can just delay that bug until this one lands and solve it there if you like (the bug is annoying but should be easy to uplift).

> If you remove :not(:-moz-lwtheme) from the #TabsToolbar:not(:-moz-lwtheme) rule you added in browser.css, this shouldn't be needed.

That doesn't work for light theme, which needs dark font inside the tabs but light toolbar buttons on aero.

> Why is this in compacttheme.css?

I should have added a comment. It's the counterpart of this rule: https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/themes/windows/browser-aero.css#334

That rule has :not(:-moz-lwtheme), but we want it to apply to light/dark theme as well, since they also use aero glass.
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

> This is what I've been using in bug 1398125 to draw a border around light/dark theme tabs, but now all three themes need a border around them. We can just delay that bug until this one lands and solve it there if you like (the bug is annoying but should be easy to uplift).

Thinking about it again, I might also remove this part from bug 1398125 and file an entirely new bug.
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

> Please document what you're doing here

Regarding the border-image override, I'll remove that from the patch and make a separate bug, it's not that important and maybe not correct anyway.
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186562

::: browser/themes/windows/browser.css:110
(Diff revision 2)
>  
> +/*
> + * Windows 7 draws the chrome background color as the tab background
> + * instead of in the tabs toolbar.
> + */
> +@media (-moz-os-version: windows-win7) {

I expect we don't want this for classic nor for high-contrast themes.

::: browser/themes/windows/browser.css:120
(Diff revision 2)
> +  #TabsToolbar:not(:-moz-lwtheme) {
> +    color: hsl(240,9%,98%);
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab:not(:-moz-lwtheme) {
> +    background-color: hsl(235,33%,19%);

Why is this not set on .tab-background?

::: browser/themes/windows/browser.css:124
(Diff revision 2)
> +  #tabbrowser-tabs > .tabbrowser-tab:not(:-moz-lwtheme) {
> +    background-color: hsl(235,33%,19%);
> +  }
> +
> +  /* Always show full-height tab separators on tabs with borders. */
> +  #tabbrowser-tabs > .tabbrowser-tab::before {

Why #tabbrowser-tabs > ?

::: browser/themes/windows/browser.css:129
(Diff revision 2)
> +  #tabbrowser-tabs > .tabbrowser-tab::before {
> +    border-image: none !important;
> +  }
> +
> +  /* Show border on tabs with background colors in Windows 7. */
> +  #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > .tab-background {

Why #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > ?

::: browser/themes/windows/compacttheme.css:38
(Diff revision 2)
>  @media (-moz-windows-glass) {
> -  .tabbrowser-tab {
> +  /* Always show light toolbar buttons on aero glass surface. */
> +  #main-window #TabsToolbar .toolbarbutton-1,
> +  #main-window #TabsToolbar .scrollbutton-up,
> +  #main-window #TabsToolbar .scrollbutton-down {
> +    fill: hsl(240,9%,98%) !important;

This shouldn't be needed as long as we set the right text color on #TabsToolbar.

::: browser/themes/windows/compacttheme.css:55
(Diff revision 2)
> +  }
> +}
> +
> +@media (-moz-os-version: windows-win7) {
> +  /* Show the tabs toolbar background color inside tabs on Win 7. */
> +  #tabbrowser-tabs > .tabbrowser-tab {

Why did you change this selector?
Attachment #8909276 - Flags: review?(dao+bmo) → review-
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186562

> I expect we don't want this for classic nor for high-contrast themes.

Why not?

> Why #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > ?

As with the background, to override using a more specific selector, I'll just use !important and let you decide what you prefer. :)

> This shouldn't be needed as long as we set the right text color on #TabsToolbar.

Again, light theme has black as #TabsToolbar color but needs white as fill for toolbarbutton icons because of aero. Maybe I'm missing something, how would it work without this override?

> Why did you change this selector?

While moving code around. I didn't notice the change in my diff view.
(In reply to Johann Hofmann [:johannh] from comment #10)
> Comment on attachment 8909276 [details]
> Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove
> glass fog on default theme.
> 
> https://reviewboard.mozilla.org/r/180866/#review186562
> 
> > I expect we don't want this for classic nor for high-contrast themes.
> 
> Why not?

Classic already has a darker title bar, and adding this custom background on top of that probably looks extra outlandish. For High Contrast this is just counterproductive in terms of accessibility, aesthetics aside.

> > This shouldn't be needed as long as we set the right text color on #TabsToolbar.
> 
> Again, light theme has black as #TabsToolbar color but needs white as fill
> for toolbarbutton icons because of aero. Maybe I'm missing something, how
> would it work without this override?

The tabs need black text color but not the toolbar itself.
(In reply to Dão Gottwald [::dao] from comment #12)
> > > This shouldn't be needed as long as we set the right text color on #TabsToolbar.
> > 
> > Again, light theme has black as #TabsToolbar color but needs white as fill
> > for toolbarbutton icons because of aero. Maybe I'm missing something, how
> > would it work without this override?
> 
> The tabs need black text color but not the toolbar itself.

So that would require us to override the TabsToolbar color to always be white in compacttheme.css AND override the tab color to be the original TabsToolbar color in compacttheme.css as well, I don't really understand how that is superior to only overriding the fill value of the TabsToolbar. Am I missing something?
(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Johann Hofmann [:johannh] from comment #10)
> > Comment on attachment 8909276 [details]
> > Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove
> > glass fog on default theme.
> > 
> > https://reviewboard.mozilla.org/r/180866/#review186562
> > 
> > > I expect we don't want this for classic nor for high-contrast themes.
> > 
> > Why not?
> 
> Classic already has a darker title bar, and adding this custom background on
> top of that probably looks extra outlandish. For High Contrast this is just
> counterproductive in terms of accessibility, aesthetics aside.

So I tried this and it just looks strange for dark and light theme. For two different reasons. For dark theme, it's a bit strange to retain the blue background while the rest of the UI goes dark. For light theme, we would have to override the TabsToolbar color to white since the black color doesn't work on the blue background. Do you really think we have to special-case classic theme? Would you agree to only not show the background color on the default theme for win classic?
(In reply to Johann Hofmann [:johannh] from comment #13)
> > The tabs need black text color but not the toolbar itself.
> 
> So that would require us to override the TabsToolbar color to always be
> white in compacttheme.css AND override the tab color to be the original
> TabsToolbar color in compacttheme.css as well, I don't really understand how
> that is superior to only overriding the fill value of the TabsToolbar. Am I
> missing something?

It gets us consistent colors for items with text (e.g. zoom controls) on the toolbar.

(In reply to Johann Hofmann [:johannh] from comment #14)
> So I tried this and it just looks strange for dark and light theme. For two
> different reasons. For dark theme, it's a bit strange to retain the blue
> background while the rest of the UI goes dark. For light theme, we would
> have to override the TabsToolbar color to white since the black color
> doesn't work on the blue background. Do you really think we have to
> special-case classic theme? Would you agree to only not show the background
> color on the default theme for win classic?

I'd have to see it to judge it. I don't think we should be concerned with Windows Classic in this bug.
(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Johann Hofmann [:johannh] from comment #13)
> > > The tabs need black text color but not the toolbar itself.
> > 
> > So that would require us to override the TabsToolbar color to always be
> > white in compacttheme.css AND override the tab color to be the original
> > TabsToolbar color in compacttheme.css as well, I don't really understand how
> > that is superior to only overriding the fill value of the TabsToolbar. Am I
> > missing something?
> 
> It gets us consistent colors for items with text (e.g. zoom controls) on the
> toolbar.

Ah, indeed. That's a good reason! I'll change it.

> (In reply to Johann Hofmann [:johannh] from comment #14)
> > So I tried this and it just looks strange for dark and light theme. For two
> > different reasons. For dark theme, it's a bit strange to retain the blue
> > background while the rest of the UI goes dark. For light theme, we would
> > have to override the TabsToolbar color to white since the black color
> > doesn't work on the blue background. Do you really think we have to
> > special-case classic theme? Would you agree to only not show the background
> > color on the default theme for win classic?
> 
> I'd have to see it to judge it. I don't think we should be concerned with
> Windows Classic in this bug.

Ok, let's open a follow-up bug for properly looking at Windows Classic & High Contrast and take anything that concerns it out of this patch.
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review187050

::: browser/themes/windows/browser.css:110
(Diff revision 4)
>  
> +/*
> + * Windows 7 draws the chrome background color as the tab background
> + * instead of in the tabs toolbar.
> + */
> +@media (-moz-os-version: windows-win7) {

Need to query -moz-windows-default-theme here

::: browser/themes/windows/compacttheme.css:36
(Diff revision 4)
>  }
>  
>  @media (-moz-windows-glass) {
> +  /* Always show light toolbar elements on aero glass surface. */
> +  #TabsToolbar,
> +  #TabsToolbar .toolbarbutton-1 {

.toolbarbutton-1 already inherits color
(In reply to Dão Gottwald [::dao] from comment #18)
> ::: browser/themes/windows/compacttheme.css:36
> (Diff revision 4)
> >  }
> >  
> >  @media (-moz-windows-glass) {
> > +  /* Always show light toolbar elements on aero glass surface. */
> > +  #TabsToolbar,
> > +  #TabsToolbar .toolbarbutton-1 {
> 
> .toolbarbutton-1 already inherits color

This is to override https://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/shared/compacttheme.inc.css#93.

Would you like me to add a comment?
(In reply to Johann Hofmann [:johannh] from comment #19)
> (In reply to Dão Gottwald [::dao] from comment #18)
> > ::: browser/themes/windows/compacttheme.css:36
> > (Diff revision 4)
> > >  }
> > >  
> > >  @media (-moz-windows-glass) {
> > > +  /* Always show light toolbar elements on aero glass surface. */
> > > +  #TabsToolbar,
> > > +  #TabsToolbar .toolbarbutton-1 {
> > 
> > .toolbarbutton-1 already inherits color
> 
> This is to override
> https://searchfox.org/mozilla-central/rev/
> d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/shared/compacttheme.
> inc.css#93.
> 
> Would you like me to add a comment?

Ideally that rule shouldn't target the buttons directly either.
(In reply to Dão Gottwald [::dao] from comment #20)
> (In reply to Johann Hofmann [:johannh] from comment #19)
> > (In reply to Dão Gottwald [::dao] from comment #18)
> > > ::: browser/themes/windows/compacttheme.css:36
> > > (Diff revision 4)
> > > >  }
> > > >  
> > > >  @media (-moz-windows-glass) {
> > > > +  /* Always show light toolbar elements on aero glass surface. */
> > > > +  #TabsToolbar,
> > > > +  #TabsToolbar .toolbarbutton-1 {
> > > 
> > > .toolbarbutton-1 already inherits color
> > 
> > This is to override
> > https://searchfox.org/mozilla-central/rev/
> > d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/shared/compacttheme.
> > inc.css#93.
> > 
> > Would you like me to add a comment?
> 
> Ideally that rule shouldn't target the buttons directly either.

So, I don't think that rule even applies anymore, at least it's not relevant to the bug it's trying to fix (on my machine). I'll try to remove it and fix whatever we break.
Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review187118

::: browser/themes/shared/compacttheme.inc.css:93
(Diff revision 5)
>  
> -#navigator-toolbox .toolbarbutton-1,
>  .browserContainer > findbar .findbar-button,
>  #PlacesToolbar toolbarbutton.bookmark-item {
>    color: var(--chrome-color);
>    text-shadow: none;

Is the rest of this rule still needed? Would be a good opportunity to remove this.
Attachment #8909276 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #23)
> Comment on attachment 8909276 [details]
> Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove
> glass fog on default theme.
> 
> https://reviewboard.mozilla.org/r/180866/#review187118
> 
> ::: browser/themes/shared/compacttheme.inc.css:93
> (Diff revision 5)
> >  
> > -#navigator-toolbox .toolbarbutton-1,
> >  .browserContainer > findbar .findbar-button,
> >  #PlacesToolbar toolbarbutton.bookmark-item {
> >    color: var(--chrome-color);
> >    text-shadow: none;
> 
> Is the rest of this rule still needed? Would be a good opportunity to remove
> this.

It seems like the text-shadow rule is still needed, I'll remove the color part.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab1e0e064452
Make chrome background show in tabs on Windows 7 and remove glass fog on default theme. r=dao
https://hg.mozilla.org/mozilla-central/rev/ab1e0e064452
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
See Also: → 1398696
Blocks: 1398125
Blocks: 1391227
Blocks: 1389223
No longer blocks: 1398125
Blocks: 1398125
Very bad move: dark favicons are invisible now (example: Atlassian Crucible favicon)
(In reply to Geobert Quach from comment #28)
> Very bad move: dark favicons are invisible now (example: Atlassian Crucible
> favicon)

You can follow bug 1094357.
Depends on: 1402552
Depends on: 1402614
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
See Also: → 1404136
Depends on: 1404286
Depends on: 1404136
See Also: 1404136 → 
I verified this issue using Windows 7 x32, ona Nightly 58.0a1 and Firefox Beta 57.0b4 with Build ID 20170928180207.
I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: --- → verified
Flags: qe-verify+
Depends on: 1425665
Depends on: 1423646
Depends on: 1439789
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: