Update new icons to match Photon spec

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
4 months ago
12 days ago

People

(Reporter: chsiang, Assigned: rickychien)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

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

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 months ago
Blocks: 1357306
No longer blocks: 1357308

Updated

2 months ago
Target Milestone: --- → Firefox 57
(Assignee)

Comment 1

2 months ago
* Face icon in Default browser
* Face icon in Firefox update
* 4 Category icons
* Firefox support
Summary: Change the sad face to the smiley face when user updates Firefox to the latest version → Update new icons to match Photon spec

Updated

2 months ago
Priority: P3 → P2

Updated

2 months ago
Flags: qe-verify+
QA Contact: hani.yacoub
(Assignee)

Updated

a month ago
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 2

28 days ago
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 4

28 days ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

22 days ago
mozreview-review
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.

Comment 10

22 days ago
(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 11

22 days ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

22 days ago
mozreview-review-reply
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:))
(Assignee)

Updated

22 days ago
Flags: needinfo?(ntim.bugs)

Comment 15

22 days ago
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 16

22 days ago
(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.
Comment hidden (mozreview-request)

Comment 18

21 days ago
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.
Flags: needinfo?(rchien)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

21 days ago
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.
Flags: needinfo?(rchien)

Comment 21

21 days ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 23

21 days ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
(Assignee)

Comment 27

21 days ago
> 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?
Flags: needinfo?(ntim.bugs)

Comment 28

21 days ago
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 29

21 days ago
> 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 30

21 days ago
mozreview-review
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
(Assignee)

Comment 34

21 days ago
Well, I've answered that on comment 23, the currentColor doesn't equal to the color in spec (#0C0C0D).
Comment hidden (mozreview-request)
(Assignee)

Comment 36

21 days ago
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).
Comment hidden (mozreview-request)

Updated

20 days ago
Duplicate of this bug: 1386531
(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?
(Assignee)

Comment 40

20 days ago
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.
(Assignee)

Comment 42

20 days ago
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.
(Assignee)

Comment 44

20 days ago
Can you tell me more details about how to use currentColor? And how to set currentColor to #0C0C0D?
Flags: needinfo?(dao+bmo)
See https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#currentcolor_keyword
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)

Comment 47

20 days ago
mozreview-review
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.
Attachment #8889822 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 49

20 days ago
mozreview-review
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.
Attachment #8889822 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 51

20 days ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 57

19 days ago
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?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
(Assignee)

Comment 59

19 days ago
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.
(Assignee)

Comment 60

19 days ago
I'll update my patch and remove all MOZ_PHOTON_PREFERENCES flags soon.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

19 days ago
Attachment #8889822 - Attachment is obsolete: true
Attachment #8889822 - Flags: review?(dao+bmo)
(Assignee)

Comment 63

19 days ago
Created attachment 8893652 [details] [diff] [review]
Bug 1361957 - Update new icons for about:preferences to match Photon spec

My mozreview somehow is broken, converted patch into bugzilla review
Attachment #8893652 - Flags: review?(dao+bmo)
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?
Attachment #8893652 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 65

18 days ago
Created attachment 8893796 [details] [diff] [review]
Bug 1361957 - Update new icons for about:preferences to match Photon spec
Attachment #8893652 - Attachment is obsolete: true
Attachment #8893796 - Flags: review?(dao+bmo)
(Assignee)

Updated

18 days ago
Attachment #8893796 - Attachment is patch: true
Comment on attachment 8893796 [details] [diff] [review]
Bug 1361957 - Update new icons for about:preferences to match Photon spec

thanks!
Attachment #8893796 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

17 days ago
Keywords: checkin-needed
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
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Updated

16 days ago
Attachment #8893796 - Attachment is obsolete: true
(Assignee)

Comment 69

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

Comment 71

16 days ago
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?
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 78

14 days ago
mozreview-review
Comment on attachment 8889822 [details]
Bug 1361957 - Update new icons for about:preferences to match Photon spec

https://reviewboard.mozilla.org/r/160914/#review171130
Attachment #8889822 - Flags: review?(dao+bmo) → review+

Comment 79

14 days ago
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
Flags: needinfo?(rchien)
Comment hidden (mozreview-request)
(Assignee)

Updated

14 days ago
Flags: needinfo?(rchien)

Comment 82

14 days ago
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

Comment 83

13 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d1ee6f3b3f4
Status: ASSIGNED → RESOLVED
Last Resolved: 13 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED

Updated

13 days ago
Depends on: 1388761
Depends on: 1388821

Updated

12 days ago
No longer depends on: 1388821
You need to log in before you can comment on or make changes to this bug.