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

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[:Gijs (he/him)]

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?

Comment 2

[Justin Dolske [:Dolske]]

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.

Comment 3

[Stephen Horlander [:shorlander]]

(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

Comment 4

[Justin Dolske [:Dolske]]

NI Steven for the icons. :)

Comment 5

[Stephen Horlander [: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.

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: 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)

- 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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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>

Comment 9

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

Tracking 40 as dolske listed this as a bug that we need for release in bug 1173725 comment 118.

Comment 10

[Justin Dolske [:Dolske]]

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?

Comment 11

[Justin Dolske [:Dolske]]

(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

[Tim Nguyen :ntim]

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"])

Comment 13

[Stephen Horlander [:shorlander]]

(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.

Comment 14

[Stephen Horlander [:shorlander]]

Attachment: caption-buttons-02.patch

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.

Comment 15

[Tim Nguyen :ntim]

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

Comment 16

[Justin Dolske [:Dolske]]

(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

[Tim Nguyen :ntim]

(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).

Comment 18

[Justin Dolske [:Dolske]]

url:        https://hg.mozilla.org/integration/fx-team/rev/da4b14a3178a2fa0bd58eb52ea1a38e4787267f5
changeset:  da4b14a3178a2fa0bd58eb52ea1a38e4787267f5
user:       Stephen Horlander <[e-mail]>
date:       Sat Jul 25 13:56:18 2015 -0700
description:
Bug 1184934 - Add caption buttons for themes and high contrast mode. r=dolske

Comment 19

[Justin Dolske [: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 20

[Justin Dolske [:Dolske]]

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.

Comment 21

[Justin Dolske [:Dolske]]

(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

[Tim Nguyen :ntim]

(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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

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

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+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ac67bf8a46a

Comment 26

[Stephen Horlander [:shorlander]]

Attachment: caption-buttons-beta.patch

Patch based on beta.

Comment 27

[Tim Nguyen :ntim]

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

Bug 1172716 is going to be uplifted, so this is no longer needed.

Comment 28

[Justin Dolske [: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.

Comment 29

[Justin Dolske [:Dolske]]

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

(Moving approval-beta flag over)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cd04493e243d