1184325 - Invert the Windows 10 titlebar controls for the dark Dev Edition theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Brian Grinstead [:bgrins]]

Attachment: DevFox-(Win10)-MainWindow-01.png

From Bug 1172716 Comment 10 and 12:

shorlander:

  If we can make the caption button inverted and mesh better in that layout it would be nice.

ntim:

  We won't be able to achieve (01) without bug 1173725.  Let's do (02) for now and do (01) once bug 1173725 is fixed.

See the attachment for the desired effect for the minimize / maximize / close buttons in Win10 in the Dev Edition dark theme.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: deved-win10-dark-window-controls.png

current screenshot (notice that the window controls are black on the dark background)

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch

Untested patch.
#main-window vs :root on the third selector is for specificity.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8635623 [details] [diff] [review]
Patch

>+  /* Force white caption buttons for the dark theme on Windows 10 */
>+  :root[devtoolstheme="dark"] #titlebar-min {
>+    list-style-image: url(chrome://browser/skin/caption-buttons.svg#minimize-highlight);
>+  }
>+  :root[devtoolstheme="dark"] #titlebar-max {
>+    list-style-image: url(chrome://browser/skin/caption-buttons.svg#maximize-highlight);
>+  }
>+  #main-window[devtoolstheme="dark"][sizemode="maximized"] #titlebar-max {
>+    list-style-image: url(chrome://browser/skin/caption-buttons.svg#restore-highlight);
>+  }
>+  :root[devtoolstheme="dark"] #titlebar-close {
>+    list-style-image: url(chrome://browser/skin/caption-buttons.svg#close-highlight);
>+  }

The *-highlight variants use the highlighttext color, so they aren't guaranteed to be white. I guess we should add another set of those icons that actually uses white.

Comment 4

[Tim Nguyen :ntim]

Attachment: Patch v2

Also changed the hover state of the close button on default theme to use the white variant instead of the Highlight one. High Contrast themes will still use the HighlightText variant for the close button hover state.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #4)
> Created attachment 8635685 [details] [diff] [review]
> Patch v2
> 
> Also changed the hover state of the close button on default theme to use the
> white variant instead of the Highlight one.

Why?

Comment 6

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Tim Nguyen [:ntim] from comment #4)
> > Created attachment 8635685 [details] [diff] [review]
> > Patch v2
> > 
> > Also changed the hover state of the close button on default theme to use the
> > white variant instead of the Highlight one.
> 
> Why?
Because the hover state of the close button is a hard coded red, and not a system color.

Comment 7

[Dão Gottwald [:dao]]

Hmm, but for the default theme we have a pretty good idea that HighlightText will be white. That's less true for non-default themes. Maybe we should use white in both cases?

Comment 8

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [:dao] from comment #7)
> Hmm, but for the default theme we have a pretty good idea that HighlightText
> will be white. That's less true for non-default themes. Maybe we should use
> white in both cases?

Non-default themes have "highlight" as hover state background. So it makes sense to me to keep using HighlightText there.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > Hmm, but for the default theme we have a pretty good idea that HighlightText
> > will be white. That's less true for non-default themes. Maybe we should use
> > white in both cases?
> 
> Non-default themes have "highlight" as hover state background.

Ok, I hadn't realized that red isn't used there.

Comment 10

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be979c6ee6d7

Comment 11

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=be979c6ee6d7

Just tested this on my Windows 10 laptop, works fine.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e9494eb8e710

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/e9494eb8e710

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: Windows 10 and bug 1173725
[User impact if declined]: people will see black caption buttons on devedition dark theme.
[Describe test coverage new/current, TreeHerder]: on m-c, local testing
[Risks and why]: low, simple css fix
[String/UUID change made/needed]: none

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8635685 [details] [diff] [review]
Patch v2

No need for beta uplift since the DE theme is only available up to Aurora right now (see bug 1181721)

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

Confirming the fix for this issue using regular and HiDPI devices running Windows 10 64-bit, with latest 42.0a1 Nightly (build ID: 20150722030205).

Comment 17

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

Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Verified on nightly, should be safe to uplift to Aurora.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ab54987779c

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: windows 10
[User impact if declined]: bug 1184934 can't be uplifted
[Describe test coverage new/current, TreeHerder]: on m-c and aurora, tested by QA
[Risks and why]: low, see test coverage.
[String/UUID change made/needed]: none

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Turns out we do need this fix as it's a prereq for bug 1184934. Beta+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing around bug 1172716 (or an uplift request in that bug as well).

Comment 22

[Tim Nguyen :ntim]

Nominated bug 1172716 for uplift.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Flipping to beta- as this is no longer a prereq for bug 1184934.

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Confirming the fix on Windows 10 64-bit.
Tested on latest 42.0a2 Aurora (build ID: 20150910004036) as DevEdition in not available in Firefox 41.