Closed Bug 1184325 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- verified

People

(Reporter: bgrins, Assigned: ntim)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
current screenshot (notice that the window controls are black on the dark background)
Blocks: windows-10
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Untested patch.
#main-window vs :root on the third selector is for specificity.
Attachment #8635623 - Flags: review?(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.
Attachment #8635623 - Flags: review?(dao) → review-
Attached patch Patch v2Splinter Review
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.
Attachment #8635623 - Attachment is obsolete: true
Attachment #8635685 - Flags: review?(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?
(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.
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?
(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.
(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.
(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.
Attachment #8635685 - Flags: review?(dao) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9494eb8e710
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: qe-verify+
QA Contact: cornel.ionce
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
Attachment #8635685 - Flags: approval-mozilla-beta?
Attachment #8635685 - Flags: approval-mozilla-aurora?
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)
Attachment #8635685 - Flags: approval-mozilla-beta?
Confirming the fix for this issue using regular and HiDPI devices running Windows 10 64-bit, with latest 42.0a1 Nightly (build ID: 20150722030205).
Status: RESOLVED → VERIFIED
Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Verified on nightly, should be safe to uplift to Aurora.
Attachment #8635685 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8635685 - Flags: approval-mozilla-beta?
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+
Attachment #8635685 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This needs rebasing around bug 1172716 (or an uplift request in that bug as well).
Flags: needinfo?(ntim.bugs)
Nominated bug 1172716 for uplift.
Flags: needinfo?(ntim.bugs)
Comment on attachment 8635685 [details] [diff] [review]
Patch v2

Flipping to beta- as this is no longer a prereq for bug 1184934.
Attachment #8635685 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
See Also: → 1189201
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.