[Windows 10] Add different SVG files for lightweight theme support for the caption buttons

RESOLVED FIXED in Firefox 40

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gijs, Assigned: shorlander)

Tracking

(Blocks 1 bug)

38 Branch
Firefox 42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40+ fixed, firefox41 fixed, firefox42 fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
It's not clear to me from the design if this is meant to be for both light and dark lightweight themes, or just dark ones. Either way, we need more icons. Stephen?
Flags: needinfo?(shorlander)
Reporter

Updated

4 years ago
Blocks: windows-10
Reporter

Updated

4 years ago
Duplicate of this bug: 1150711
I'd assume both light and dark? The glyphs are thin and a single color, so anything that's not a flat color background is potentially going to cause issues.
(In reply to Justin Dolske [:Dolske] from comment #2)
> I'd assume both light and dark? The glyphs are thin and a single color, so
> anything that's not a flat color background is potentially going to cause
> issues.

Yeah, it's for both
Flags: needinfo?(shorlander)
NI Steven for the icons. :)
Flags: needinfo?(shorlander)
Working on this. Have a WIP patch, but trying to get it to look right with the interaction between the SVG and CSS and the rounding is… frustrating.
Posted patch caption-buttons.patch (obsolete) — Splinter Review
This takes care of bug 1184929 also because they are pretty closely linked.

- I modified the SVG to remove the <use> elements, since that's apparently bad for memory? (https://bugzilla.mozilla.org/show_bug.cgi?id=1153615#c7)

- Made the High Contrast icons thicker and had to tweak the color values and add an extra state because it was breaking in two of the default High Contrast theme settings

- Added a set of icons that works in Light and Dark themes

I noticed that it doesn't round perfectly in some combinations of zoom factor and screen resolution settings. But I think it mostly looks ok.
Flags: needinfo?(shorlander)
Attachment #8637269 - Flags: review?(dolske)
Comment on attachment 8637269 [details] [diff] [review]
caption-buttons.patch

Review of attachment 8637269 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/caption-buttons.svg
@@ +46,4 @@
>    </g>
> +  <g id="close-inverted" class="inverted">
> +    <path d="M1,1 l 10,10 M1,11 l 10,-10"/>
> +  </g>

We don't need to duplicate manually. We can use the text preprocessor to do the duplication for us.

In /browser/themes/windows/jar.mn, add an asterisk at the beginning of the caption-buttons.svg line. Then, in this file, we can use the preprocessor like so,

#define close <line x1="1" y1="1" x2="11" y2="11"/><line x1="11" y1="1" x2="1" y2="11"/>
  <g id="close">
#expand __close__
  </g>
  <g id="close-inverted" class="inverted">
#expand __close__
  </g>
Attachment #8637269 - Flags: feedback+

Updated

4 years ago
Blocks: theme-win10
No longer blocks: windows-10

Updated

4 years ago
Duplicate of this bug: 1184929
Tracking 40 as dolske listed this as a bug that we need for release in bug 1173725 comment 118.
Comment on attachment 8637269 [details] [diff] [review]
caption-buttons.patch

Review of attachment 8637269 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of small nits but otherwise fine. I didn't actually test this in the different modes as I presume you've already done so extensively. :)

::: browser/themes/windows/caption-buttons.svg
@@ +10,5 @@
>      }
>  
> +    g.inverted {
> +      stroke: #fff;
> +      stroke-width: 0.9px;

Seems like stoke-width isn't needed here, as it should pick up the same from the plain g{} rule above?

@@ -26,1 @@
>      }

Hmm, I feel like I'm missing something with how this is currently working... Looks like |HighlightText| should be white (assuming default theme), which is why close-highlight is a white glyph when hovering the close button? But why are the other *-highlight buttons showing as black instead of white? They're all using is same .highlight class, but the close button is the only one that becomes white on hover.

@@ +77,5 @@
>      <polyline points="3.5,3.5 3.5,1.5 10.5,1.5 10.5,8.5 8.5,8.5"/>
>    </g>
> +  <g id="restore-highcontrast" class="highcontrast">
> +    <rect x="2" y="4" width="6" height="6"/>
> +    <polyline points="3.5,1.5 10.5,1.5 10.5,8.5" stroke-width=".9"/>

Can you move the stroke-width on the 3 polylines up into the CSS?
Attachment #8637269 - Flags: review?(dolske) → review+
Assignee: nobody → shorlander
(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #6)
> Created attachment 8637269 [details] [diff] [review]
> caption-buttons.patch
> 
> This takes care of bug 1184929 also because they are pretty closely linked.
> 
> - I modified the SVG to remove the <use> elements, since that's apparently
> bad for memory? (https://bugzilla.mozilla.org/show_bug.cgi?id=1153615#c7)

AIUI, this is more of an issue for way the SVG in that bug was being used (a bunch of icons with almost 200 <use> variants), whereas here we just have a handful of icons/variants -- and so I'd expect the overhead to have minimal actual impact. Fine for me either way, would probably be good to see if Robert can expand on his comment.


(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> We don't need to duplicate manually. We can use the text preprocessor to do
> the duplication for us.

We could, but I don't think we should. There's value in being able to test/use the SVG directly as it exists in the tree. (Especially for the trivial duplication here, I might feel differently if this was a more complex SVG where the duplication was non-obvious.)

Comment 12

4 years ago
Comment on attachment 8637269 [details] [diff] [review]
caption-buttons.patch

Review of attachment 8637269 [details] [diff] [review]:
-----------------------------------------------------------------

This will need to be rebased on top of bug 1184325.

::: browser/themes/windows/caption-buttons.svg
@@ +35,5 @@
>      g:not(:target) {
>        display: none;
>      }
>  
> +    g:not(#close):not(#close-inverted):not(#close-highcontrast):not(#close-highcontrast-hover):not(#close-themes) {

nit : use g:not([id|="close"])
(In reply to Justin Dolske [:Dolske] from comment #10)
> Couple of small nits but otherwise fine. I didn't actually test this in the
> different modes as I presume you've already done so extensively. :)

I did test it from 100% – 250% with the default system theme, with several highcontrast themes and with LWTs. Looked fine to me with the caveat that I tested it on a 1x device and a 2x device and not a native device for each zoom factor.


> Hmm, I feel like I'm missing something with how this is currently working...
> Looks like |HighlightText| should be white (assuming default theme), which
> is why close-highlight is a white glyph when hovering the close button? But
> why are the other *-highlight buttons showing as black instead of white?
> They're all using is same .highlight class, but the close button is the only
> one that becomes white on hover.

I am using id="close-inverted" for the default close hover/active icon for two reasons:

1) The highcontrast icons are thicker than the default icons
2) The background hover is a known color (red) and HighlightText is not always white.


> @@ +77,5 @@
> >      <polyline points="3.5,3.5 3.5,1.5 10.5,1.5 10.5,8.5 8.5,8.5"/>
> >    </g>
> > +  <g id="restore-highcontrast" class="highcontrast">
> > +    <rect x="2" y="4" width="6" height="6"/>
> > +    <polyline points="3.5,1.5 10.5,1.5 10.5,8.5" stroke-width=".9"/>
> 
> Can you move the stroke-width on the 3 polylines up into the CSS?

Sure thing.
Didn't realize that bug 1184325 was a thing. Incorporated those changes, moved back to using <use> but still had to add some extra stuff for thicker icons and theme icons.
Attachment #8637269 - Attachment is obsolete: true
Attachment #8638104 - Flags: review?(dolske)

Comment 15

4 years ago
Comment on attachment 8638104 [details] [diff] [review]
caption-buttons-02.patch

Review of attachment 8638104 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/caption-buttons.svg
@@ +31,5 @@
> +    }
> +
> +    .outer-stroke {
> +      stroke: #000;
> +      stroke-width: 3.6;

You're not using an unit here, but you are doing so in the CSS above, can you be consistent here ?

@@ +67,5 @@
>    <use id="maximize-white" xlink:href="#maximize"/>
>    <use id="minimize-white" xlink:href="#minimize"/>
>    <use id="restore-white" xlink:href="#restore"/>
> +
> +  <g id="close-highcontrast" class="highlight">

I'm not sure if the highlight class is appropriate for the highcontrast shapes.
Can you use the [id$="-highcontrast"] selector and remove the highlight class ?

@@ +86,5 @@
> +  <use id="maximize-highcontrast-hover" xlink:href="#maximize-highcontrast"/>
> +  <use id="minimize-highcontrast-hover" xlink:href="#minimize-highcontrast"/>
> +  <use id="restore-highcontrast-hover" xlink:href="#restore-highcontrast"/>
> +
> +  <g id="close-themes" class="themes">

Let's also be consistent here too and use the id selector
Attachment #8638104 - Flags: review?(dolske) → review+
(In reply to Tim Nguyen [:ntim] from comment #15)

> Can you use the [id$="-highcontrast"] selector and remove the highlight
> class ?

I'd generally prefer using class names for stuff like this. Using the attribute hyphen selector is a little hacky, and someone changing the ID probably wouldn't be checking to see if substrings mattered somewhere. IE, it's basically reinventing classes to save a little bit of verbosity.

Comment 17

4 years ago
(In reply to Justin Dolske [:Dolske] from comment #16)
> (In reply to Tim Nguyen [:ntim] from comment #15)
> 
> > Can you use the [id$="-highcontrast"] selector and remove the highlight
> > class ?
> 
> I'd generally prefer using class names for stuff like this. Using the
> attribute hyphen selector is a little hacky, and someone changing the ID
> probably wouldn't be checking to see if substrings mattered somewhere. IE,
> it's basically reinventing classes to save a little bit of verbosity.

I'm fine either way, but we should be consistent and settle with one selector type (with this patch, half on the file is using the ID selector, the other half is using classes).
url:        https://hg.mozilla.org/integration/fx-team/rev/da4b14a3178a2fa0bd58eb52ea1a38e4787267f5
changeset:  da4b14a3178a2fa0bd58eb52ea1a38e4787267f5
user:       Stephen Horlander <shorlander@mozilla.com>
date:       Sat Jul 25 13:56:18 2015 -0700
description:
Bug 1184934 - Add caption buttons for themes and high contrast mode. r=dolske
(In reply to Tim Nguyen [:ntim] from comment #17)

> I'm fine either way, but we should be consistent and settle with one
> selector type (with this patch, half on the file is using the ID selector,
> the other half is using classes).

Steven says he'll do this in a followup next week. Since it's not really a functional difference, I just landed it to help make sure it can make Monday's Firefox 40 beta.
Comment on attachment 8638104 [details] [diff] [review]
caption-buttons-02.patch

Approval Request Comment
[Feature/regressing bug #]: Windows 10
[User impact if declined]: poor UX for the main window controls for users using a LWT or high-contrast mode.
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: fiarly low risk, Steven and I have both tested this with LWTs and HC themes, and at worst it would be an improvement over what's currently landed.
[String/UUID change made/needed]: None.
Attachment #8638104 - Flags: approval-mozilla-beta?
Attachment #8638104 - Flags: approval-mozilla-aurora?
(I've also been testing at a variety of scaling factors over in bug 1187631 with normal themes, and it looks fine there too.)

Comment 22

4 years ago
(In reply to Justin Dolske [:Dolske] from comment #19)
> Steven says he'll do this in a followup next week. Since it's not really a
> functional difference, I just landed it to help make sure it can make
> Monday's Firefox 40 beta.

Sounds good, please don't forget to address other nits from comment 15 too.

Updated

4 years ago
Depends on: 1187905
https://hg.mozilla.org/mozilla-central/rev/da4b14a3178a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment on attachment 8638104 [details] [diff] [review]
caption-buttons-02.patch

We're running out of time to complete Windows 10 work in 40. Let's take this style change in beta8 even though it has only recently landed on m-c. As Dolske said, the worst case for this change is still a better state than we're in today. Beta+ Aurora+
Attachment #8638104 - Flags: approval-mozilla-beta?
Attachment #8638104 - Flags: approval-mozilla-beta+
Attachment #8638104 - Flags: approval-mozilla-aurora?
Attachment #8638104 - Flags: approval-mozilla-aurora+

Comment 27

4 years ago
Comment on attachment 8640284 [details] [diff] [review]
caption-buttons-beta.patch

Bug 1172716 is going to be uplifted, so this is no longer needed.
Attachment #8640284 - Attachment is obsolete: true
Attachment #8640284 - Flags: review?(dolske)
Comment on attachment 8640284 [details] [diff] [review]
caption-buttons-beta.patch

LGTM, and tested locally on a beta build with LWTs and high-contrast OS themes.

Per followup comments in bug 1172716, we _do_ want to land this patch instead of uplifting that.
Attachment #8640284 - Attachment is obsolete: false
Attachment #8640284 - Flags: review+
Comment on attachment 8638104 [details] [diff] [review]
caption-buttons-02.patch

(Moving approval-beta flag over)
Attachment #8638104 - Flags: approval-mozilla-beta+
Attachment #8640284 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.