* Face icon in Default browser * Face icon in Firefox update * 4 Category icons * Firefox support

Visual refresh spec: * 4 Category icons (with highlight) https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683136 * Firefox support https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683138 * Sad/Smile Face icons in Default browser https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/245152762 * Sad/Smile Face icons in Firefox update https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683136 Note that we will use the same Sad/Smile Face icons for both Default browser and Firefox update sections.

All new visual refresh CSS changes will be added within %ifdef MOZ_PHOTON_PREFERENCES. This bug will target on all icon relevant change and make sure we match the spec (see comment 2).

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review168220 I've optimized all the SVGs in this gist: https://gist.github.com/nt1m/68ef8886276a72ced78c151bc83c7689 Please use them, they remove a lot of useless paths and flatten some transforms as well. They also use context-fill and context-fill-opacity so you can use what I suggested below. ::: browser/themes/shared/incontentprefs/preferences.inc.css:745 (Diff revision 5) > + > +#category-general > .category-icon { > + list-style-image: url("chrome://browser/skin/preferences/in-content-new/general.svg"); > +} > + > +#category-general[selected=true] > .category-icon { Since all the -focused.svg variants are color and opacity variants. You can just use context-fill and context-fill-opacity and two rules: .category[selected=true] > .category-icon { -moz-context-properties: fill, fill-opacity; fill: #0c0c0d; fill-opacity: 0.8; } .category[selected=true] > .category-icon { fill: #0a84ff; fill-opacity: 1; } and remove all the -focused.svg images.

(In reply to Tim Nguyen :ntim from comment #9) > .category[selected=true] > .category-icon { > -moz-context-properties: fill, fill-opacity; > fill: #0c0c0d; > fill-opacity: 0.8; > } I meant .category > .category-icon here.

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review168222 ::: commit-message-131e1:1 (Diff revision 5) > +Bug 1361957 - Update new icons to match Photon spec r?jaws The commit message should indicate that the change is about preferences.

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review168220 Wow, thanks for doing this! I'm curious about what kind of SVGO tool you used? The current SVG icons have been updated reflecting to visual spec changed. (I'm really appreciate your help:))

(In reply to Ricky Chien [:rickychien] from comment #14) > Comment on attachment 8889822 [details] > Bug 1361957 - Update new icons for about:preferences to match Photon spec > > https://reviewboard.mozilla.org/r/160914/#review168220 > > Wow, thanks for doing this! > > I'm curious about what kind of SVGO tool you used? The current SVG icons > have been updated reflecting to visual spec changed. (I'm really appreciate > your help:)) These are mainly manual changes, I don't think there's an actual tool that does my optimizations. SVGO can do some of it, but only in certain cases. To flatten transforms: - Move the transform attribute from the <g> down to each child, then run the file through SVGO, and the transform disappears without any visual change! fill-rule="evenodd"/nonzero: - fill-rule="nonzero" is default, unless the parent has fill-rule="evenodd" set, so it's usually not useful - fill-rule="evenodd" is sometimes needed, but most of the time isn't, you have to manually check if it affects rendering or not. Useless groups: - Usually groups aren't useful, here's a typical example of it: <g fill="none"> <path fill="blue"/> <path fill="red"/> </g> This can be simplified to: <path fill="blue"/> <path fill="red"/> If you do all the steps properly, you should end up with an SVG with just a series of paths: <svg fill="context-fill" [...]> <path d="..."/> <path d="..."/> </svg> I might consider writing an SVGO plugin (or contributing the changes directly to SVGO) if I have some time.

(In reply to Tim Nguyen :ntim from comment #9) > They also use context-fill and context-fill-opacity so you can use what I > suggested below. After investigating `context-fill` and `context-fill-opacity`, it only support `background-image` but not `list-style-image`. We're going to get rid of `background-image` in order to support high contrast theme (all `background-image` will be disabled in high contrast theme). I'll go back to use `defs` and `use` mechanism to handle different colors in the same svg.

Are you sure ? We seem to be using it for list-style-image in about:preferences: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#572 Also, we'd like to stop using the :not(:target) hack, see bug 1358998.

Ah, you're right! Eventually I figured out using `context-fill` and `context-fill-opacity` to deal with svg colors. Thanks for sharing this trick.

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review168676 ::: browser/themes/shared/incontentprefs/face-sad.svg:1 (Diff revision 9) > +<?xml version="1.0" encoding="utf-8"?> nit: you can remove this ::: browser/themes/shared/incontentprefs/face-sad.svg:6 (Diff revision 9) > +<?xml version="1.0" encoding="utf-8"?> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"> > + <g fill="none" fill-rule="evenodd" transform="translate(0 .8)"> What's the point of this transform? ::: browser/themes/shared/incontentprefs/preferences.inc.css:652 (Diff revision 9) > - height: 14px; > background-position: 15px; > padding-inline-start: 35px; > white-space: nowrap; > + fill: #0C0C0D; > + stroke: #0C0C0D; Looks like you forgot to set -moz-context-properties? ::: browser/themes/shared/incontentprefs/preferences.inc.css:741 (Diff revision 9) > margin: 0; /* Align with the margin of xul:label.menu-iconic-text */ > } > + > +%ifdef MOZ_PHOTON_PREFERENCES > + > +#categories .category-icon { Why #categories rather than just .category-icon? ::: browser/themes/shared/incontentprefs/preferences.inc.css:746 (Diff revision 9) > +#categories .category-icon { > + width: 30px; > + height: 30px; > + padding: 3px; > + -moz-context-properties: fill, fill-opacity; > + fill: #0C0C0D; Can you use currentColor instead of #0C0C0D?

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review168676 > What's the point of this transform? It's literally translate(x y) = `translate(0 0.8)`. > Looks like you forgot to set -moz-context-properties? .help-button has an inherited -moz-context-properties, so it's unnecessary to set it again. > Can you use currentColor instead of #0C0C0D? The currentColor is grey which doesn't equal to #0C0C0D.

(In reply to Ricky Chien [:rickychien] from comment #23) > Comment on attachment 8889822 [details] > Bug 1361957 - Update new icons for about:preferences to match Photon spec > > https://reviewboard.mozilla.org/r/160914/#review168676 > > > What's the point of this transform? > > It's literally translate(x y) = `translate(0 0.8)`. I know that, but why are you shifting stuff down by 0.8 pixels? This seems odd. > > Can you use currentColor instead of #0C0C0D? > > The currentColor is grey which doesn't equal to #0C0C0D. According to <https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683136>, the icon color should match the text color.

> I know that, but why are you shifting stuff down by 0.8 pixels? This seems odd. ntim, do you know how to get rid of `translate(0 0.8)` attribute safely?

(In reply to Ricky Chien [:rickychien] from comment #27) > > I know that, but why are you shifting stuff down by 0.8 pixels? This seems odd. > > ntim, do you know how to get rid of `translate(0 0.8)` attribute safely? You could either "flatten" the transform, which means applying it directly to the path data, which is what I've done in the gist: https://gist.github.com/nt1m/68ef8886276a72ced78c151bc83c7689 . This ensures no visible changes to the SVG. Otherwise, you could just remove the transform attribute, but that would result in a shift upwards 0.8px.

> According to <https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683136>, the icon color should match the text color. The text color will be addressed in bug 1377167. Let's focus on icons in this bug. Thanks

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review168804 ::: browser/themes/shared/incontentprefs/fxa-avatar.svg:6 (Diff revision 12) > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 80 80"> > + <g fill="none" fill-rule="evenodd"> > + <rect width="80" height="80" fill="#E1E1E6" rx="40"/> Here's a clean version of the SVG: https://gist.githubusercontent.com/nt1m/68ef8886276a72ced78c151bc83c7689/raw/caea1d00c7e1bdc122511556a5672f88b40ed8a8/fxa-avatar.svg ::: browser/themes/shared/incontentprefs/help.svg:5 (Diff revision 12) > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"> > + <g fill="context-fill" fill-opacity="context-fill-opacity"> nit: remove the <g> tag and apply the fill and fill-opacity attributes on <svg> ::: browser/themes/shared/incontentprefs/logo-android.svg:5 (Diff revision 12) > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"> > + <path fill="#0C0C0D" fill-rule="evenodd" d="M9.24 16.03v2.73c0 .7-.56 1.24-1.25 1.24-.7 0-1.26-.55-1.26-1.24v-2.73h-.9c-.75 0-1.35-.59-1.35-1.33v-8h11.2v8c0 .74-.6 1.33-1.36 1.33h-.9v2.73c0 .7-.55 1.24-1.25 1.24s-1.26-.55-1.26-1.24v-2.73H9.24zM2.76 6.5c.7 0 1.25.55 1.25 1.24v5.15c0 .7-.56 1.24-1.25 1.24-.7 0-1.26-.55-1.26-1.24V7.73c0-.69.56-1.24 1.26-1.24zm14.64 0c.7 0 1.25.55 1.25 1.24v5.15c0 .7-.55 1.24-1.25 1.24s-1.26-.55-1.26-1.24V7.73c0-.69.56-1.24 1.26-1.24zM6.6 0c.06 0 .12.03.16.1l.9 1.58a6.04 6.04 0 0 1 4.84 0L13.4.1a.18.18 0 0 1 .24-.07c.09.05.12.15.07.24l-.89 1.58a5.04 5.04 0 0 1 2.86 4.43H4.48c0-1.9 1.15-3.56 2.85-4.43L6.45.26a.17.17 0 0 1 .07-.24A.18.18 0 0 1 6.6 0zm.9 3.33c-.26 0-.47.2-.47.46a.47.47 0 0 0 .93 0 .47.47 0 0 0-.47-.46zm5.16 0c-.25 0-.47.2-.47.46a.47.47 0 0 0 .94 0 .47.47 0 0 0-.47-.46z"/> nit: remove fill-rule=evenodd from this SVG and logo-ios.svg

(In reply to Ricky Chien [:rickychien] from comment #29) > > According to <https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683136>, the icon color should match the text color. > > The text color will be addressed in bug 1377167. Let's focus on icons in > this bug. Thanks Well, my comment was about the icons. They should use currentColor. I don't care what patch does it but at the moment neither the patches in this bug nor bug 1377167 do it.

Well, I've answered that on comment 23, the currentColor doesn't equal to the color in spec (#0C0C0D).

Current patch is focusing on changing icon along with its color according to spec. As a result, I'd prefer to change currentColor to spec color (#0C0C0D).

(In reply to Ricky Chien [:rickychien] from comment #36) > Current patch is focusing on changing icon along with its color according to > spec. As a result, I'd prefer to change currentColor to spec color (#0C0C0D). What spec are you talking about?

See No.1 Icon https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683005

As I mentioned, that spec uses the same colors for the icon and the text. The right way to achieve this is to set the color in one place and have the icon adopt it using currentColor, rather than repeating the same color multiple times in different places.

OK, IIRC, you are saying about using CSS variable. I'll change it. Thanks

As opposed to currentColor, CSS variables add overhead that we don't need here.

Can you tell me more details about how to use currentColor? And how to set currentColor to #0C0C0D?

See https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#currentcolor_keyword

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review169150 ::: browser/themes/shared/incontentprefs/preferences.inc.css:638 (Diff revision 17) > - height: 14px; > background-position: 15px; > padding-inline-start: 35px; > white-space: nowrap; > + fill: #0C0C0D; > + stroke: #0C0C0D; Looks like this icon should use currentColor too. ::: browser/themes/shared/incontentprefs/preferences.inc.css:729 (Diff revision 17) > } > + > +%ifdef MOZ_PHOTON_PREFERENCES > + > +.category { > + color: #0C0C0D; Can we make this change in toolkit/themes/shared/in-content/common.inc.css so that the pages using that stylesheet stay in sync? ::: browser/themes/shared/incontentprefs/preferences.inc.css:777 (Diff revision 17) > +#category-general[selected=true] > .category-icon, > +#category-search[selected=true] > .category-icon, > +#category-privacy[selected=true] > .category-icon, > +#category-sync[selected=true] > .category-icon { > + fill: currentColor; > +} This rule shouldn't be needed anymore.

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review169178 ::: browser/themes/shared/incontentprefs/preferences.inc.css:639 (Diff revision 18) > background-position: 15px; > padding-inline-start: 35px; > white-space: nowrap; > + fill: currentColor; > + stroke: currentColor; > } Looks like -moz-context-properties is still missing here.

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review169188 ::: browser/themes/shared/incontentprefs/face-sad.svg:5 (Diff revision 19) > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"> > + <g fill="none" fill-rule="evenodd"> nit: remove the <g> tag ::: browser/themes/shared/incontentprefs/face-smile.svg:5 (Diff revision 19) > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"> > + <g fill="none" fill-rule="evenodd"> nit: remove the <g> tag.

Jared, Do you think it's reasonable that we can ship Photon redesign features without introducing #ifdef MOZ_PHOTON_PREFERENCES flag into our code? There are some XUL file changes involved which might needs to wrap #ifdef or #elseif around XUL element block. The code will be more ugly and much harder to maintain. I'd prefer to work on new feature without using MOZ_PHOTON_PREFERENCES flag, and file a bug to revert bug 1380585. Does it make sense for you?

Yeah, we can remove the MOZ_PHOTON_PREFERENCES flag now that 57 is on mozilla-central. Please feel free to file a bug and include a revert of 1380585.

Thanks for the response! We're going to land all Photon visual changes without adding MOZ_PHOTON_PREFERENCES flag. Bug 1382514 might be renamed for reverting bug 1380585.

I'll update my patch and remove all MOZ_PHOTON_PREFERENCES flags soon.

My mozreview somehow is broken, converted patch into bugzilla review

Comment on attachment 8893652 [details] [diff] [review] Bug 1361957 - Update new icons for about:preferences to match Photon spec >+#categories > .category > .category-icon { >+ width: 24px; >+ height: 24px; >+ padding: 3px; >+ -moz-context-properties: fill, fill-opacity; >+ fill: currentColor; >+ fill-opacity: 0.8; >+} This should eventually be in common.inc.css too. If you don't want to do that here, could you please file a new bug on this?

Comment on attachment 8893796 [details] [diff] [review] Bug 1361957 - Update new icons for about:preferences to match Photon spec thanks!

applying https://bug1361957.bmoattachments.org/attachment.cgi?id=8893796 patching file browser/components/preferences/in-content-new/main.xul Hunk #1 FAILED at 308 Hunk #2 FAILED at 315 Hunk #3 FAILED at 824 Hunk #4 FAILED at 838 4 out of 4 hunks FAILED -- saving rejects to file browser/components/preferences/in-content-new/main.xul.rej patching file browser/themes/shared/incontentprefs/preferences.inc.css Hunk #1 FAILED at 113 Hunk #2 FAILED at 414 Hunk #3 FAILED at 548 Hunk #4 FAILED at 571 Hunk #5 FAILED at 590 Hunk #6 FAILED at 625 Hunk #7 FAILED at 639 7 out of 7 hunks FAILED -- saving rejects to file browser/themes/shared/incontentprefs/preferences.inc.css.rej patching file browser/themes/shared/jar.inc.mn Hunk #1 FAILED at 94 1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/jar.inc.mn.rej patching file toolkit/themes/shared/in-content/common.inc.css Hunk #1 FAILED at 21 Hunk #2 FAILED at 681 2 out of 2 hunks FAILED -- saving rejects to file toolkit/themes/shared/in-content/common.inc.css.rej abort: patch failed to apply

Dao, sorry for asking review request again. I've fixed my mozreview issue and hopefully my current setting will not be glitch again. I'll land this patch as soon as I got r+. Thanks

BTW, touching common.inc.css will affect the style of about:addons as well. The current category color of about:addons looks weird after we touched common.inc.css. Dao, I'd like to revert the changes in common.inc.css in order to avoid breaking other in-content pages. Does it make sense to you?

(In reply to Ricky Chien [:rickychien] from comment #71) > BTW, touching common.inc.css will affect the style of about:addons as well. > The current category color of about:addons looks weird after we touched > common.inc.css. > > Dao, I'd like to revert the changes in common.inc.css in order to avoid > breaking other in-content pages. Does it make sense to you? No, we need to keep about:preferences and about:addons in sync as much as possible rather than letting them diverge further. What's needed to fix about:addons? Do we just need to update more colors? Bug 1380043 is about the icons.

Comment on attachment 8889822 [details] Bug 1361957 - Update new icons for about:preferences to match Photon spec https://reviewboard.mozilla.org/r/160914/#review171130

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/91fbc9dada78 Update new icons for about:preferences to match Photon spec r=dao

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=121708791&repo=autoland https://hg.mozilla.org/integration/autoland/rev/38e3c45d3f9961b3c7bc7e4c47a5885a005c1517

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0d1ee6f3b3f4 Update new icons for about:preferences to match Photon spec r=dao

Build ID: 20170829100404 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.