1373341 - Two different blue highlight states needed for light and dark theme (downloading, bookmarking)

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Amy Lee [:amylee] UX]

Attachment: Highlight Colour Spec.png

We will need two different blue highlight states for downloads in the toolbar for light and dark themes in order to pass the WCAG 2.0 Level AA guidelines for colour contrast. See attached spec

Comment 1

[Sam Foster [:sfoster] (he/him)]

This is the "attention" color, defined as --toolbarbutton-icon-fill-attention in: 
http://searchfox.org/mozilla-central/source/browser/themes/shared/toolbarbutton-icons.inc.css#4

Comment 2

[Sam Foster [:sfoster] (he/him)]

The color changes are simple enough, but this blue is also baked into bookmark-animation.svg and library-bookmark-animation.svg. We'll need to use context-fill and context-stroke instead of hard-coding the color values into the SVG so that the theme is able to extend into the animations. That means we have a strictly limited palette of 2 colors (plus black & white) and degrees of transparency to work with. Our optimization tools can do the actual substitutions, but we may need to re-draw these animations to fit these constraints?

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

* Correct blue used as attention/accent color in the toolbar to #0a84ff (#45a1ff for brighttext) as specified.
* Use context-fill and context-stroke consistently in the animation svgs to pick up the current value of the attention color.
* Use --toolbarbutton-icon-fill-attention consistently to pick up the current value of the attention color.

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

Comment 4

[Sam Foster [:sfoster] (he/him)]

I double-checked the blue color change. This new blue is in fact the one we should be using for light themes. 
In bookmark-animation.svg, there were several shades of blue. I've replaced these with the single context-stroke with no visible (to my eyes) impact. 
The same blue was used in #customization-done-button, so for consistency's sake I've used the --toolbarbutton-icon-fill-attention here too, but probably we either need another variable here, or to rename the existing one as it is a bit nonsensical in this context. 

:johannh I'm going to pick on you for review as you reviewed bug 1387723 (and your queue is shorter than :dao's)

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

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

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

https://reviewboard.mozilla.org/r/166822/#review172088

::: browser/themes/shared/toolbarbutton-icons.inc.css:3
(Diff revision 2)
>  :root {
> -  --toolbarbutton-icon-fill-attention: #177ee5;
> +  --toolbarbutton-icon-fill-attention: #0a84ff;
>  }

Add a toolbar[brighttext] rule here for the alternative color, rather than doing this in compacttheme.inc.css

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

https://reviewboard.mozilla.org/r/166822/#review172150

r=me with the issues addressed (including dao's comment). I don't think you'll need another review for those, but of course feel free to flag me again in case of doubt :)

Thanks!

::: browser/themes/shared/customizableui/customizeMode.inc.css:82
(Diff revision 2)
>  
>  #customization-done-button {
>    color: #fff;
>    font-weight: 700;
>    border-color: #0060df;
> -  background-color: #0a84ff;
> +  background-color: var(--toolbarbutton-icon-fill-attention);

> The same blue was used in #customization-done-button, so for consistency's sake I've used the --toolbarbutton-icon-fill-attention here too, but probably we either need another variable here, or to rename the existing one as it is a bit nonsensical in this context. 

I agree that it's nonsensical in the context. Why not drop this change from this patch and open a follow-up bug that formulates the problem you're trying to solve here? :)

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

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

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

https://reviewboard.mozilla.org/r/166822/#review172398

::: browser/themes/shared/toolbarbutton-icons.inc.css:3
(Diff revision 2)
>  :root {
> -  --toolbarbutton-icon-fill-attention: #177ee5;
> +  --toolbarbutton-icon-fill-attention: #0a84ff;
>  }

This patch doesn't seem to set --toolbarbutton-icon-fill-attention to #45a1ff anywhere..?

Comment 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

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

Comment 11

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8895605 [details]
Bug 1373341 - Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes.

https://reviewboard.mozilla.org/r/166822/#review172398

> This patch doesn't seem to set --toolbarbutton-icon-fill-attention to #45a1ff anywhere..?

Yeah, I messed up a rebase. Fixed now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdc9b689b8b3
Use --toolbarbutton-icon-fill-attention consistently; correct its values in light & dark themes. r=johannh

Comment 13

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

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

Comment 14

[Abe - QA (:Abe_LV)]

I tested this on latest nightly (Build Id:20170810100255) and it is verified as fixed.

Comment 15

[Fahima Zulfath]

Managed to reproduce the issue on an affected build. Hence fixed and verified.
Browser : Firefox Nightly 57.0a1
Build ID: 20170816100153
OS: Windows 10.0

Comment 16

[Fahima Zulfath]

 [bugday-20170816]