1404286 - Border between tabs and navigation toolbar is too dark on Windows 7

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1404286 - Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7.

Review commit: https://reviewboard.mozilla.org/r/185006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185006/

Comment 2

[Johann Hofmann [:johannh]]

Comment on attachment 8913615 [details]
Bug 1404286 - Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7.

https://reviewboard.mozilla.org/r/185006/#review190084

::: browser/themes/windows/browser.css:117
(Diff revision 1)
>   * instead of in the tabs toolbar.
>   */
>  @media (-moz-os-version: windows-win7) {
>    @media (-moz-windows-default-theme) {
>      :root:not(:-moz-lwtheme) {
> -      --tabs-border: #4A4A4F;
> +      --tabs-border: @glassShadowColor@;

This makes the aero background shine through borders between tabs, which looks weird, depending on your background (we already have this problem for light/dark themes).

I think the best solution for this is to always set the chrome-background on the tab instead of the tab-background for Windows 7 (so that the border has the right background color to mix with) and putting the top-border that is currently on tabs onto .scrollbox-innerbox.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Comment on attachment 8913615 [details]
> Bug 1404286 - Let the border between tabs and navigation toolbar use the
> standard chrome-on-glass border color on Windows 7.
> 
> https://reviewboard.mozilla.org/r/185006/#review190084
> 
> ::: browser/themes/windows/browser.css:117
> (Diff revision 1)
> >   * instead of in the tabs toolbar.
> >   */
> >  @media (-moz-os-version: windows-win7) {
> >    @media (-moz-windows-default-theme) {
> >      :root:not(:-moz-lwtheme) {
> > -      --tabs-border: #4A4A4F;
> > +      --tabs-border: @glassShadowColor@;
> 
> This makes the aero background shine through borders between tabs, which
> looks weird, depending on your background (we already have this problem for
> light/dark themes).

When I tested this patch I suspected something like that but didn't really see it. It must be minor, certainly less jarring than the toolbar border mismatch.

> I think the best solution for this is to always set the chrome-background on
> the tab instead of the tab-background for Windows 7 (so that the border has
> the right background color to mix with) and putting the top-border that is
> currently on tabs onto .scrollbox-innerbox.

I think this should be a separate bug. You mentioned you wanted to change this anyway because the hover effect is currently broken.

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8913615 [details]
Bug 1404286 - Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7.

https://reviewboard.mozilla.org/r/185006/#review190112

Ok, fine, though I have to say I find that issue much more jarring than the problem this patch is fixing :D

> > I think the best solution for this is to always set the chrome-background on
> > the tab instead of the tab-background for Windows 7 (so that the border has
> > the right background color to mix with) and putting the top-border that is
> > currently on tabs onto .scrollbox-innerbox.
> 
> I think this should be a separate bug. You mentioned you wanted to change
> this anyway because the hover effect is currently broken.

Yes, that's in bug 1391539, it would be great if you could review that one so that I can base a fix for the problem on top of it.

::: browser/themes/windows/browser.css:12
(Diff revision 1)
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  @namespace html url("http://www.w3.org/1999/xhtml");
>  
>  %include ../shared/browser.inc.css
> +%filter substitution
> +%define glassShadowColor hsla(209,67%,12%,0.35)

Shouldn't this be hsla(240, 5%, 5%, 0.3) (--color-chrome-border-30 from http://design.firefox.com/people/shorlander/photon/Mockups/windows-7.html)?

::: browser/themes/windows/browser.css:117
(Diff revision 1)
>   * instead of in the tabs toolbar.
>   */
>  @media (-moz-os-version: windows-win7) {
>    @media (-moz-windows-default-theme) {
>      :root:not(:-moz-lwtheme) {
> -      --tabs-border: #4A4A4F;
> +      --tabs-border: @glassShadowColor@;

I think you need to override --tabs-border in lwthemes as well, it would be nice to have it consistent with the borders defined in browser-aero.css (though it's probably hardly perceivable).

Comment 5

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Comment on attachment 8913615 [details]
> Bug 1404286 - Let the border between tabs and navigation toolbar use the
> standard chrome-on-glass border color on Windows 7.
> 
> https://reviewboard.mozilla.org/r/185006/#review190112
> 
> Ok, fine, though I have to say I find that issue much more jarring than the
> problem this patch is fixing :D

In non-maximized windows the tabs toolbar / nav bar border is way darker than the other glass borders around the UI. It's very visible.

> > > I think the best solution for this is to always set the chrome-background on
> > > the tab instead of the tab-background for Windows 7 (so that the border has
> > > the right background color to mix with) and putting the top-border that is
> > > currently on tabs onto .scrollbox-innerbox.
> > 
> > I think this should be a separate bug. You mentioned you wanted to change
> > this anyway because the hover effect is currently broken.
> 
> Yes, that's in bug 1391539, it would be great if you could review that one
> so that I can base a fix for the problem on top of it.

Not sure I understand. Is the fix already in bug 1391539 or do you want to fix it after that?

> ::: browser/themes/windows/browser.css:12
> (Diff revision 1)
> >  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> >  @namespace html url("http://www.w3.org/1999/xhtml");
> >  
> >  %include ../shared/browser.inc.css
> > +%filter substitution
> > +%define glassShadowColor hsla(209,67%,12%,0.35)
> 
> Shouldn't this be hsla(240, 5%, 5%, 0.3) (--color-chrome-border-30 from
> http://design.firefox.com/people/shorlander/photon/Mockups/windows-7.html)?

Not sure. We've been using this value since forever and it seems to work well. I suspect Stephen just came up randomly with something different for his mockup, not necessarily because it's better than this color. I'll file a new bug on potentially changing it.

> ::: browser/themes/windows/browser.css:117
> (Diff revision 1)
> >   * instead of in the tabs toolbar.
> >   */
> >  @media (-moz-os-version: windows-win7) {
> >    @media (-moz-windows-default-theme) {
> >      :root:not(:-moz-lwtheme) {
> > -      --tabs-border: #4A4A4F;
> > +      --tabs-border: @glassShadowColor@;
> 
> I think you need to override --tabs-border in lwthemes as well, it would be
> nice to have it consistent with the borders defined in browser-aero.css
> (though it's probably hardly perceivable).

Not sure offhand what the problem is with lwthemes, but it seems orthogonal to this patch. Can you file a new bug?

Comment 6

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #5)
> > > > I think the best solution for this is to always set the chrome-background on
> > > > the tab instead of the tab-background for Windows 7 (so that the border has
> > > > the right background color to mix with) and putting the top-border that is
> > > > currently on tabs onto .scrollbox-innerbox.
> > > 
> > > I think this should be a separate bug. You mentioned you wanted to change
> > > this anyway because the hover effect is currently broken.
> > 
> > Yes, that's in bug 1391539, it would be great if you could review that one
> > so that I can base a fix for the problem on top of it.
> 
> Not sure I understand. Is the fix already in bug 1391539 or do you want to
> fix it after that?

After that. Bug 1391539 has been cooking for too long already and it's a much bigger/high impact change.

> 
> > ::: browser/themes/windows/browser.css:12
> > (Diff revision 1)
> > >  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> > >  @namespace html url("http://www.w3.org/1999/xhtml");
> > >  
> > >  %include ../shared/browser.inc.css
> > > +%filter substitution
> > > +%define glassShadowColor hsla(209,67%,12%,0.35)
> > 
> > Shouldn't this be hsla(240, 5%, 5%, 0.3) (--color-chrome-border-30 from
> > http://design.firefox.com/people/shorlander/photon/Mockups/windows-7.html)?
> 
> Not sure. We've been using this value since forever and it seems to work
> well. I suspect Stephen just came up randomly with something different for
> his mockup, not necessarily because it's better than this color. I'll file a
> new bug on potentially changing it.

Ok, please do that.

> 
> > ::: browser/themes/windows/browser.css:117
> > (Diff revision 1)
> > >   * instead of in the tabs toolbar.
> > >   */
> > >  @media (-moz-os-version: windows-win7) {
> > >    @media (-moz-windows-default-theme) {
> > >      :root:not(:-moz-lwtheme) {
> > > -      --tabs-border: #4A4A4F;
> > > +      --tabs-border: @glassShadowColor@;
> > 
> > I think you need to override --tabs-border in lwthemes as well, it would be
> > nice to have it consistent with the borders defined in browser-aero.css
> > (though it's probably hardly perceivable).
> 
> Not sure offhand what the problem is with lwthemes, but it seems orthogonal
> to this patch. Can you file a new bug?

Not sure what you mean, but the border is defined here: https://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/browser/themes/windows/browser.css#56 and it's not being overriden for lwthemes. I was thinking it would be more consistent if we did. Yeah, that could be a follow-up bug as well, I guess.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8913615 [details]
Bug 1404286 - Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7.

https://reviewboard.mozilla.org/r/185006/#review190196

Let's move forward on this, but we should really fix/track the inner-border issue for uplift (assuming this gets uplifted as well).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f78e0d0d331
Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7. r=johannh

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f78e0d0d331

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Does this need a Beta approval request?

Comment 11

[Dão Gottwald [:dao]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Does this need a Beta approval request?

Yes, this was waiting for bug 1366405 to land first.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8913615 [details]
Bug 1404286 - Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1399498
[User impact if declined]: borders around the UI look inconsistent on Win 7
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: straightforward color change
[String changes made/needed]: /

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8913615 [details]
Bug 1404286 - Let the border between tabs and navigation toolbar use the standard chrome-on-glass border color on Windows 7.

recent regression, low risk fix, beta57+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0dd094dc6fd1

Comment 15

[Saheda Reza [:Antora]]

I have reproduced this bug according to (2017-09-29)

Fixing bug is verified on Latest Beta--
Build ID    :20171016185129
User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101 Firefox/57.0
Now it Looks Ok. 

Tested OS--Windows7 32bit

Comment 16

[Valentina Claudia Ona]

I verified this issue using Nightly 58.0a1 on Windows 10 x64 with Build ID 20171017220415. I will mark this as verified fixed.