1359631 - Convert a bunch of toolkit SVG icons to use context-fill

Status: RESOLVED FIXED

(Toolkit :: Themes, enhancement)

Description

[Jonathan Watt [:jwatt]]

As part of fixing bug 1358998 we should stop using the :not(:target) selector in toolkit SVG icons. I don't have cycles to convert all of them but I've done a bunch.

Comment 1

[Jonathan Watt [:jwatt]]

Ho, hum. I tried to use reviewboard to request review, but as per my previous experiences with reviewboard things have not gone as I expected. The patches are visible at:

https://reviewboard.mozilla.org/r/133640/diff/1/

but for whatever reason haven't shown up here as review requests. Dao, do you need me to figure out how to fix whatever's going wrong here or are you happy to just go ahead with the reviews?

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

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

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's media/playButton.svg to use context paint to improve performance.

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

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

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

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's media/playButton.svg to use context paint to improve performance.

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

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's narrate/back.svg to use context paint to improve performance.

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

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's narrate/forward.svg to use context paint to improve performance.

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

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's reader/RM-Close-24x24.svg to use context paint to improve performance.

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

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8861675 [details]
Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/133640/#review136678

::: browser/themes/linux/browser.css:993
(Diff revision 1)
>  
>  .ac-type-icon[type=keyword],
>  .ac-site-icon[type=searchengine] {
> -  list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg#search-icon);
> +  list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg);
> +  /* uncomment after bug 1350010 lands: context-properties: fill; */
> +  fill: GrayText; /* context-fill for image */

Does currentColor work here?

I don't think we need to repeat the "context-fill for image" comment all over the place.

::: toolkit/themes/linux/global/jar.mn
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  #include ../../shared/non-mac.jar.inc.mn
>  
>  toolkit.jar:
> -   skin/classic/global/autocomplete.css

What's going on here?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8861675 [details]
Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/133640/#review136680

::: toolkit/themes/shared/jar.inc.mn:34
(Diff revision 1)
>    skin/classic/global/icons/calendar-arrows.svg            (../../shared/icons/calendar-arrows.svg)
>    skin/classic/global/icons/find-arrows.svg                (../../shared/icons/find-arrows.svg)
>    skin/classic/global/icons/info.svg                       (../../shared/incontent-icons/info.svg)
> +#ifndef XP_MACOSX
> +  skin/classic/global/icons/autocomplete-search.svg        (../../shared/icons/autocomplete-search.svg)
> +#endif

This can probably go into non-mac.jar.inc.mn.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8861675 [details]
Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/133640/#review136678

> Does currentColor work here?
> 
> I don't think we need to repeat the "context-fill for image" comment all over the place.

To me it seems like the purpose of the 'fill' properties will be really non-obvious to most/many people who may read this code, especially at the points where we're not also [going to be, once uncommented] setting 'context-properties'. So I'd expect these to be helpful. If you're sure you would rather they're removed I'll remove them all.

> What's going on here?

I noticed the contents of the Windows and Linux files are identical, so I moved the windows file to the shared directory toolkit/themes/shared/icons/, and removed the linux file. With the linux file removed I need to remove this line which references it.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> > I don't think we need to repeat the "context-fill for image" comment all over the place.
> 
> To me it seems like the purpose of the 'fill' properties will be really
> non-obvious to most/many people who may read this code, especially at the
> points where we're not also [going to be, once uncommented] setting
> 'context-properties'. So I'd expect these to be helpful. If you're sure you
> would rather they're removed I'll remove them all.

I think context-properties: fill; helps. More generally, this is going to be the most common if not only use of fill in chrome stylesheets. People will get used to it. Adding the comment everywhere seems rather cumbersome.

> > What's going on here?
> 
> I noticed the contents of the Windows and Linux files are identical, so I
> moved the windows file to the shared directory toolkit/themes/shared/icons/,
> and removed the linux file. With the linux file removed I need to remove
> this line which references it.

You accidentally removed autocomplete.css rather than the icon.

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Dão Gottwald [::dao] from comment #9)
> Does currentColor work here?

As discussed on IRC, in other patches I have set the value to currentColor if the color is being obviously set nearby. Otherwise, testing each platform adds a lot more time to an already tricky and time consuming conversion task, and is a straw too far for this camel. ;)

(In reply to Dão Gottwald [::dao] from comment #12)
> I think context-properties: fill; helps. More generally, this is going to be
> the most common if not only use of fill in chrome stylesheets. People will
> get used to it. Adding the comment everywhere seems rather cumbersome.

Fair enough.

> You accidentally removed autocomplete.css rather than the icon.

Ah, thanks for catching.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8861675 [details]
Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

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

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8861676 [details]
Bug 1359631 - Convert Toolkit's media/playButton.svg to use context paint to improve performance.

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

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8861675 [details]
Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/133640/#review136954

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8861676 [details]
Bug 1359631 - Convert Toolkit's media/playButton.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/133642/#review136958

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa50d29ff246
Convert Toolkit's icons/autocomplete-search.svg to use context paint to improve performance. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8fcfead17d1
Convert Toolkit's extensions/utilities.svg to use context paint to improve performance. r=dao

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa50d29ff246
https://hg.mozilla.org/mozilla-central/rev/d8fcfead17d1

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's narrate/back.svg to use context paint to improve performance.

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

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's narrate/forward.svg to use context paint to improve performance.

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

Comment 24

[Jonathan Watt [:jwatt]]

Attachment: Bug 1359631 - Convert Toolkit's reader/RM-Close-24x24.svg to use context paint to improve performance.

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

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 8861675 [details]
Bug 1359631 - Convert Toolkit's media/pauseButton.svg to use context paint to improve performance.

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

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 8861676 [details]
Bug 1359631 - Convert Toolkit's media/playButton.svg to use context paint to improve performance.

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

Comment 27

[Jonathan Watt [:jwatt]]

Note these patches require the patch from bug 1359762 to work.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8862961 [details]
Bug 1359631 - Convert Toolkit's narrate/back.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/134820/#review139612

::: toolkit/themes/shared/narrateControls.css:59
(Diff revision 1)
>    border-top-right-radius: 3px;
>    background-image: url("chrome://global/skin/narrate/forward.svg#enabled");
>  }
>  
>  #narrate-skip-previous:disabled {
> -  background-image: url("chrome://global/skin/narrate/back.svg#disabled");
> +  fill: rgb(128 128 128 / 50%);

Wow, I hadn't seen this notation before. The "/" is confusing, as 1 / 50% == 200% :(

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 8862962 [details]
Bug 1359631 - Convert Toolkit's narrate/forward.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/134822/#review139614

::: toolkit/themes/shared/narrateControls.css:65
(Diff revision 1)
>  #narrate-skip-previous:disabled {
>    fill: rgb(128 128 128 / 50%);
>  }
>  
>  #narrate-skip-next:disabled {
> -  background-image: url("chrome://global/skin/narrate/forward.svg#disabled");
> +  fill: rgb(128 128 128 / 50%);

Please merge this with the previous rule.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 8862963 [details]
Bug 1359631 - Convert Toolkit's reader/RM-Close-24x24.svg to use context paint to improve performance.

https://reviewboard.mozilla.org/r/134824/#review139616

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36f3300731f1
Convert Toolkit's media/pauseButton.svg to use context paint to improve performance. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc27c1db27f5
Convert Toolkit's media/playButton.svg to use context paint to improve performance. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/7522e1fc66dd
Convert Toolkit's narrate/back.svg to use context paint to improve performance. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0ab8d3d5bd
Convert Toolkit's narrate/forward.svg to use context paint to improve performance. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7c2780cbeb
Convert Toolkit's reader/RM-Close-24x24.svg to use context paint to improve performance. r=dao

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36f3300731f1
https://hg.mozilla.org/mozilla-central/rev/dc27c1db27f5
https://hg.mozilla.org/mozilla-central/rev/7522e1fc66dd
https://hg.mozilla.org/mozilla-central/rev/cf0ab8d3d5bd
https://hg.mozilla.org/mozilla-central/rev/7d7c2780cbeb