1399500 - [Windows 7 & 8] Default, Light and Dark Themes should be structurally consistent

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Stephen Horlander [:shorlander]]

The Default, Light and Dark themes should be structurally consistent. The only difference should be in color value.

Currently there is some shifting of elements between the default and the light / dark.

http://design.firefox.com/people/shorlander/photon/Mockups/windows-7.html

Comment 1

[Johann Hofmann [:johannh]]

This will need some more thorough investigation, but I don't think I can prioritize this over other stuff that's happening right now, so moving back to reserve backlog.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

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

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186316/diff/1-2/

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186316/diff/2-3/

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

https://reviewboard.mozilla.org/r/186316/#review192286

::: browser/themes/windows/browser.css:82
(Diff revision 3)
>    display: none;
>  }
>  
>  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
>  :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar {
> -  margin-top: var(--space-above-tabbar);
> +  padding-top: var(--space-above-tabbar);

Is this change required to solve the bug? I can't perceive differences in vertical spacing on Windows 7 between the themes.

This leads (afaict) to a regression on Mac where the tab line is 1px too thin. I haven't really figured out why yet, but I'd prefer to avoid doing that (also to avoid complexity for uplift) if this was just an optimization that we can simply drop from the patch.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> Comment on attachment 8915049 [details]
> Bug 1399500 - Light and Dark themes should use the same toolbar borders as
> the default theme.
> 
> https://reviewboard.mozilla.org/r/186316/#review192286
> 
> ::: browser/themes/windows/browser.css:82
> (Diff revision 3)
> >    display: none;
> >  }
> >  
> >  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
> >  :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar {
> > -  margin-top: var(--space-above-tabbar);
> > +  padding-top: var(--space-above-tabbar);
> 
> Is this change required to solve the bug? I can't perceive differences in
> vertical spacing on Windows 7 between the themes.
> 
> This leads (afaict) to a regression on Mac where the tab line is 1px too
> thin. I haven't really figured out why yet, but I'd prefer to avoid doing
> that (also to avoid complexity for uplift) if this was just an optimization
> that we can simply drop from the patch.

To be clear, the problem rather lies in the adjusted calculation in tabsintitlebar.js, not in the Windows-specific browser.css file.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> Comment on attachment 8915049 [details]
> Bug 1399500 - Light and Dark themes should use the same toolbar borders as
> the default theme.
> 
> https://reviewboard.mozilla.org/r/186316/#review192286
> 
> ::: browser/themes/windows/browser.css:82
> (Diff revision 3)
> >    display: none;
> >  }
> >  
> >  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
> >  :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar {
> > -  margin-top: var(--space-above-tabbar);
> > +  padding-top: var(--space-above-tabbar);
> 
> Is this change required to solve the bug?

Yes. Margin is outside the toolbar's border box, padding is inside.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186316/diff/3-4/

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

https://reviewboard.mozilla.org/r/186316/#review192306

Great, thank you!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59aef3210d5e
Light and Dark themes should use the same toolbar borders as the default theme. r=johannh

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/59aef3210d5e

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

Approval Request Comment
[Feature/Bug causing the regression]: photon polish
[User impact if declined]: toolbars lack borders with the Light and Dark themes in non-maximized windows in Windows 7 and 8
[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?]: basically just moving a few CSS borders around, and replacing the tabs toolbar's margin with padding on Windows (we already did this before on Mac)
[String changes made/needed]: /

Comment 13

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

Comment on attachment 8915049 [details]
Bug 1399500 - Light and Dark themes should use the same toolbar borders as the default theme.

Photon polish, Beta57+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/55322dd4aa31

Comment 15

[Valentina Claudia Ona]

I verified this issue using Nightly 58.0a1 and Firefox Beta 57.0b9 on Windows 7 x64 and Windows 8 x64 with Build ID  	20171016185129.
I can confirm the fix.