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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Themes
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {perf})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 5 obsolete attachments)

59 bytes, text/x-review-board-request
dao
: review+
Details | Review
59 bytes, text/x-review-board-request
dao
: review+
Details | Review
59 bytes, text/x-review-board-request
dao
: review+
Details | Review
59 bytes, text/x-review-board-request
dao
: review+
Details | Review
59 bytes, text/x-review-board-request
dao
: review+
Details | Review
(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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?
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

a year ago
Assignee: nobody → jwatt
(Assignee)

Updated

a year ago
Keywords: perf

Comment 9

a year ago
mozreview-review
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?
Attachment #8861675 - Flags: review?(dao+bmo) → review-

Comment 10

a year ago
mozreview-review
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.
(Assignee)

Updated

a year ago
Depends on: 1359762
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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.
(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.
(Assignee)

Comment 13

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8861677 - Attachment is obsolete: true
Attachment #8861677 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8861678 - Attachment is obsolete: true
Attachment #8861678 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8861679 - Attachment is obsolete: true
Attachment #8861679 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8861680 - Attachment is obsolete: true
Attachment #8861680 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8861681 - Attachment is obsolete: true
Attachment #8861681 - Flags: review?(dao+bmo)

Comment 16

a year ago
mozreview-review
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
Attachment #8861675 - Flags: review?(dao+bmo) → review+

Comment 17

a year ago
mozreview-review
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
Attachment #8861676 - Flags: review?(dao+bmo) → review+

Comment 18

a year ago
Pushed by jwatt@jwatt.org:
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
(Assignee)

Updated

a year ago
Keywords: leave-open
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

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

Comment 28

a year ago
mozreview-review
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% :(
Attachment #8862961 - Flags: review?(dao+bmo) → review+

Comment 29

a year ago
mozreview-review
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.
Attachment #8862962 - Flags: review?(dao+bmo) → review+

Comment 30

a year ago
mozreview-review
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
Attachment #8862963 - Flags: review?(dao+bmo) → review+

Comment 31

a year ago
Pushed by jwatt@jwatt.org:
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
(Assignee)

Updated

a year ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.