Differential D301960

Bug 2032244 - toolkit Nova icons r=#desktop-theme-reviewers!
ClosedPublic
Actions

Authored by sthompson on Fri, May 22, 2:11 AM.

Details

Reviewers

dao

Group Reviewers

desktop-theme-reviewers

Commits

rFIREFOXAUTOLAND0a19a7c43867: Bug 2032244 - toolkit Nova icons r=desktop-theme-reviewers,dao

Bugzilla Bug ID

2032244

Summary

Displays the Nova iconography when is true.

Updates 72 existing toolkit icons with the following approach:

wraps the existing <path>(s) in a
appends the Nova redrawn icon's <path>(s) in a
adds a <style> block with rules to show only one group at a time

This is an unorthodox approach. It has several drawbacks:

increases individual SVG size
increases the time required to render each SVG
viewing the SVGs in normal image tools will only show the Proton variant, which is confusing

However, I think this approach is better than alternatives.

do not need to add new Nova icon files and CSS rules to use them conditionally
SVG uses in XHTML and similar contexts doesn't require special handling
this can be undone very easily after Nova is released, leaving only the Nova paths
the SVG rendering performance impact is not large, and alternatives would still have non-zero performance impact
total size of SVGs is not that much larger than if we added separate Nova icons

Diff Detail

Repository: rFIREFOXAUTOLAND firefox-autoland

Event Timeline

sthompson created this revision.Fri, May 22, 2:11 AM

Herald added a project: secure-revision. · View Herald TranscriptFri, May 22, 2:11 AM

sthompson added a child revision: D301961: Bug 2032240 - Firefox theme Nova icons r=#desktop-theme-reviewers!.Fri, May 22, 2:11 AM

phab-bot published this revision for review.Fri, May 22, 2:12 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Harbormaster completed remote builds in B973049: Diff 1280695.Fri, May 22, 2:36 AM

dao requested changes to this revision.Fri, May 22, 4:58 AM

dao added a subscriber: dao.

dao added inline comments.

toolkit/themes/shared/reader/word-spacing-20.svg
18	clip0_8069_32752 doesn't seem to exist? Might apply to other updated files as well.
toolkit/themes/windows/global/icons/menu-check.svg
7	don't we need fill and fill-opacity here like for non-nova? or I guess move those to the svg element? please check if this applies to other updated icons as well.
toolkit/themes/windows/global/icons/search-textbox.svg
18 ↗	(On Diff #1280695)	same here. note that pre-nova uses fill="#939393", not sure if we'd want to keep that.

This revision now requires changes to proceed.Fri, May 22, 4:58 AM

sthompson added inline comments.Fri, May 22, 5:49 PM

toolkit/themes/shared/reader/word-spacing-20.svg
18	This clip path isn't defined in the acorn-icons SVG, either. Looks like 75 of the icons in the acorn-icons repo have this same problem. I'll ask the design team to update their icon tooling. I can handle this special case in my own script by stripping because none of the acorn-icons actually have a clip path defined.
toolkit/themes/windows/global/icons/menu-check.svg
7	Hmm, yeah, this one may be tricky. All of the redrawn acorn-icons have the context drawing attributes defined on <svg>. There are 27 existing icons where context attrs are defined on specific paths. I'll have to go through these to figure out whether the acorn-icon wouldn't render correctly, whether to put the attrs on the <g class="nova"> only, or whether it's OK to promote these attrs to <svg>.
toolkit/themes/windows/global/icons/search-textbox.svg
18 ↗	(On Diff #1280695)	This is a bug in my script. Only the mac version should be updated with the Nova icon because the Nova version matches its style. The Windows one should be left alone because of the color issue and because there was no redrawn version in Nova. I flagged the Windows one with the design team already. I'll fix my script to apply the icon update to the right platform.

rpl mentioned this in D301967: Bug 2041529 - Add-ons Manager Nova icons r=#extension-reviewers!.Fri, May 22, 5:52 PM

sthompson requested review of this revision.Thu, May 28, 1:27 AM

sthompson updated this revision to Diff 1284960.

sthompson edited the summary of this revision. (Show Details)

sthompson marked 2 inline comments as done.Thu, May 28, 1:42 AM

Comment Actions

A number of updates here:

The Acorn team updated their icon exporter to trim clip paths and references to them. I confirmed that there are no longer any in the Nova icons. There are some in the existing Proton icons, but I don't think I need to clean those up in this patch. They should be cleaned up when Nova icons are promoted and the Proton paths are deleted.
thumbs-down-20 and thumbs-up-20 were uploaded to the acorn-icons repo on Github so I was able to include those
I went through all of the icons that set fill/stroke/etc. on specific paths. Among all of the toolkit icons, I found that it was sufficient to promote those attributes to the <svg> element.
I updated the osx search-textbox.svg flavor for Nova and left the Windows one alone. The previous version of this patch inappropriately changed the Windows one.

Through this process of going through all of the icons, I did encounter a few novel issues that hadn't been flagged by anyone yet. Mostly, these were issues where I mapped an inappropriate Acorn icon to the codebase. I think it's likely that there are still some errata in this patch stack, but I think they should be isolated issues rather than systematic ones.

Harbormaster completed remote builds in B976356: Diff 1284960.Thu, May 28, 1:57 AM

dao accepted this revision.Thu, May 28, 11:34 AM

dao added a project: testing-exception-ui (Only changes ui styling, images, or localized strings).

dao added inline comments.

toolkit/themes/osx/global/icons/search-textbox.svg
17	Sorry for the late suggestion, but if it's easy enough to adjust in your script: how do you feel about indenting this by only two spaces instead of four, so that after shipping we can remove the <g> wrapper without reindenting this? This would give us a simpler history on this line.

This revision is now accepted and ready to land.Thu, May 28, 11:34 AM

dao mentioned this in D301961: Bug 2032240 - Firefox theme Nova icons r=#desktop-theme-reviewers!.Thu, May 28, 11:40 AM

sthompson updated this revision to Diff 1285931.Thu, May 28, 7:39 PM

Harbormaster completed remote builds in B977040: Diff 1285931.Thu, May 28, 8:25 PM

sthompson updated this revision to Diff 1286006.Thu, May 28, 8:44 PM

sthompson marked an inline comment as done.Thu, May 28, 8:54 PM

Comment Actions

Made non-content updates -- the only new changes were whitespace (preserving existing indentation where possible, ensuring 2 space indentation on Nova elements, and putting no indentation on new <style> and <g> tags.

toolkit/themes/osx/global/icons/search-textbox.svg
17	I updated the script to add new <g>, <style> elements with no indentation at all. The indentation from the Proton elements is preserved. The indentation from the Nova elements are capped at 2 spaces. For most icons, this means that the <svg> and Proton elements have no changes from this stack of patches, and it means that after cleaning up the Proton + migration boilerplate, we'll be left with Nova elements indented from the <svg> element by 2 spaces. Note that for icons like this where I also moved the fill/fill-opacity attrs from <path> to <svg>, this patch results in changes to basically all of the lines. I think this is OK, and the cleanup should not need to touch the <svg> elements again.

Harbormaster completed remote builds in B977102: Diff 1286006.Thu, May 28, 9:15 PM

sthompson marked an inline comment as done.Fri, May 29, 12:55 AM

Closed by commit rFIREFOXAUTOLAND0a19a7c43867: Bug 2032244 - toolkit Nova icons r=desktop-theme-reviewers,dao (authored by sthompson). · Explain WhyFri, May 29, 1:39 AM

This revision was automatically updated to reflect the committed changes.

sthompson added a commit: rFIREFOXAUTOLAND0a19a7c43867: Bug 2032244 - toolkit Nova icons r=desktop-theme-reviewers,dao.

sthompson mentioned this in rFIREFOXBETA0a19a7c43867: Bug 2032244 - toolkit Nova icons r=desktop-theme-reviewers,dao.Mon, Jun 15, 12:03 PM

Revision Contents
Changeset List

Path

Size

toolkit/

themes/

osx/

global/

icons/

search-textbox.svg

17 lines

shared/

icons/

arrow-counterclockwise-16.svg

15 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

17 lines

13 lines

14 lines

13 lines

13 lines

15 lines

15 lines

13 lines

14 lines

14 lines

13 lines

13 lines

13 lines

14 lines

17 lines

13 lines

14 lines

13 lines

13 lines

15 lines

16 lines

14 lines

13 lines

13 lines

13 lines

15 lines

13 lines

13 lines

13 lines

13 lines

13 lines

13 lines

14 lines

swap-horizontal-20.svg

15 lines

15 lines

15 lines

15 lines

13 lines

13 lines

13 lines

media/

audio-muted.svg

13 lines

audio.svg

14 lines

closed-caption-settings-button.svg

18 lines

pause-fill.svg

13 lines

picture-in-picture-closed.svg

15 lines

picture-in-picture-enter-fullscreen-button.svg

17 lines

picture-in-picture-exit-fullscreen-button.svg

17 lines

picture-in-picture-open.svg

15 lines

play-fill.svg

13 lines

narrate/

skip-backward-20.svg

13 lines

skip-forward-20.svg

13 lines

reader/

align-center-20.svg

13 lines

align-left-20.svg

13 lines

align-right-20.svg

13 lines

character-spacing-20.svg

14 lines

content-width-20.svg

13 lines

line-spacing-20.svg

13 lines

word-spacing-20.svg

14 lines

windows/

global/

icons/

menu-check.svg

17 lines

Commit	Tree	Parents	Author	Summary	Date
0a19a7c43867	343bc237db63	Stephen Thompson	Bug 2032244 - toolkit Nova icons r=desktop-theme-reviewers,dao (Show More…)

	Status	Author
Closed	sthompson	D301965 Bug 2041521 - IP Protection Nova icons r=#ip-protection-reviewers!
Closed	sthompson	D301963 Bug 2041515 - urlbar Nova icons r=#urlbar-reviewers!
Closed	sthompson	D301962 Bug 2041513 - tabbrowser Nova icons r=#tabbrowser-reviewers!
Closed	sthompson	D301961 Bug 2032240 - Firefox theme Nova icons r=#desktop-theme-reviewers!
Closed	sthompson	D301960 Bug 2032244 - toolkit Nova icons r=#desktop-theme-reviewers!