Update notification icons to use photon assets

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years 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])

Attachments

(2 attachments)

Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years 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

2 years 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

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

Comment 9

2 years 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

2 years 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

2 years 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)
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Comment hidden (mozreview-request)

Comment 14

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c268fc166a1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.2 - Aug 29
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1319467
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

2 years 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)
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+
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.