Update notification icons to use photon assets

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

MozReview Requests

()

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

Attachments

(2 attachments)

Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
I've removed the pre-processing for the SVG, since the blocked SVGs were using <use> + <clipPath> which is not great for performance.

Also, this allows viewing the SVGs in the browser.
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

11 months ago
Whiteboard: [photon-visual][triage]

Updated

11 months ago
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]

Comment 9

11 months ago
mozreview-review
Comment on attachment 8899709 [details]
Bug 1392416 - Update notification icons to use photon assets.

https://reviewboard.mozilla.org/r/171030/#review176242

::: browser/base/content/browser.js:670
(Diff revision 7)
>              callback: null
>            }];
>  
>            const priority = notificationBox.PRIORITY_WARNING_MEDIUM;
>            notificationBox.appendNotification(message, "popup-blocked",
> -                                             "chrome://browser/skin/Info.png",
> +                                             "chrome://browser/skin/notification-icons/popup.svg",

Does this already have -moz-context-properties and fill?

::: browser/themes/osx/webRTC-indicator.css:7
(Diff revision 7)
>   * 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/. */
>  
>  #webRTC-sharingCamera-menu {
> -  list-style-image: url("chrome://browser/skin/webRTC-sharingDevice-menubar.png");
> +  list-style-image: url("chrome://browser/skin/notification-icons/camera.svg");
>  }

Presumably this needs -moz-context-properties and fill set.

::: browser/themes/shared/contextmenu.inc.css:50
(Diff revision 7)
>  #context-reload:-moz-locale-dir(rtl) {
>    transform: scaleX(-1);
>  }
>  
>  #context-media-eme-learnmore {
> -  list-style-image: url("chrome://browser/skin/drm-icon.svg#chains");
> +  list-style-image: url("chrome://browser/skin/drm-icon.svg");

ditto
Attachment #8899709 - Flags: review?(dao+bmo)
(Assignee)

Comment 10

11 months ago
(In reply to Dão Gottwald [::dao] from comment #9)
> Comment on attachment 8899709 [details]
> Bug 1392416 - Update notification icons to use photon assets.
> 
> https://reviewboard.mozilla.org/r/171030/#review176242
> 
> ::: browser/base/content/browser.js:670
> (Diff revision 7)
> >              callback: null
> >            }];
> >  
> >            const priority = notificationBox.PRIORITY_WARNING_MEDIUM;
> >            notificationBox.appendNotification(message, "popup-blocked",
> > -                                             "chrome://browser/skin/Info.png",
> > +                                             "chrome://browser/skin/notification-icons/popup.svg",
> 
> Does this already have -moz-context-properties and fill?
Since bug 1363056, yes.
Comment hidden (mozreview-request)

Comment 12

11 months ago
mozreview-review
Comment on attachment 8899709 [details]
Bug 1392416 - Update notification icons to use photon assets.

https://reviewboard.mozilla.org/r/171030/#review176248

::: browser/themes/shared/contextmenu.inc.css:21
(Diff revision 8)
> +
> +#context-navigation > .menuitem-iconic > .menu-iconic-left > .menu-iconic-icon,
> +#context-media-eme-learnmore .menu-iconic-icon {
>    -moz-context-properties: fill;
>    fill: currentColor;
>  }

Hmm, no, let's just add these two properties to the #context-media-eme-learnmore rule and leave this one alone.
Attachment #8899709 - Flags: review?(dao+bmo)

Updated

11 months ago
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Comment hidden (mozreview-request)

Comment 14

11 months ago
mozreview-review
Comment on attachment 8899709 [details]
Bug 1392416 - Update notification icons to use photon assets.

https://reviewboard.mozilla.org/r/171030/#review176274

::: browser/themes/shared/contextmenu.inc.css:17
(Diff revision 9)
>    width: 16px;
>    height: 16px;
>    margin: 7px;
> +}
> +
> +#context-navigation > .menuitem-iconic > .menu-iconic-left > .menu-iconic-icon {

Please revert this change. Thanks!
Attachment #8899709 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)

Comment 16

11 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c268fc166a1
Update notification icons to use photon assets. r=dao

Comment 17

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c268fc166a1
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

11 months ago
Iteration: --- → 57.2 - Aug 29
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1319467

Comment 19

10 months ago
Created attachment 8909700 [details]
Screen Shot 2017-09-19 at 12.43.21 PM.png

I tested on Mac Os X 10.10 with FF Nightly 57.0a1(2017-09-18) and when I want to open a flash game I have this icon(see the attachment) in the identity block. This is the expected one? Another question, we need to verify if the icons are the one from the link posted in the description, the icons should match the colors of the different theme(dark and light)?
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 20

10 months ago
(In reply to ovidiu boca[:Ovidiu] from comment #19)
> Created attachment 8909700 [details]
> Screen Shot 2017-09-19 at 12.43.21 PM.png
> 
> I tested on Mac Os X 10.10 with FF Nightly 57.0a1(2017-09-18) and when I
> want to open a flash game I have this icon(see the attachment) in the
> identity block. This is the expected one? 

Looks like the expected icon to me.

> Another question, we need to
> verify if the icons are the one from the link posted in the description, the
> icons should match the colors of the different theme(dark and light)?

Yes that sounds good, thanks. Maybe testing other situations such as lightweight themes (dark and light) would also be nice.
Flags: needinfo?(ntim.bugs)

Comment 21

10 months ago
I tested this on Mac OS X 10.12, Windows 7 and 10 and Ubuntu 16.04 with the latest Nightly 58.0a1(2017-10-01) and I can confirm that the icons from comment 0 are the same with the ones from the browser. I also tested using different themes.
Status: RESOLVED → VERIFIED
status-firefox58: --- → verified
Flags: qe-verify+

Comment 22

10 months ago
I also verified this on Mac OS X 10.12, Windows 10 and Ubuntu 16.04 with Firefox 57.0b4 and the specifications from description are respected.
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.