Closed Bug 1392416 Opened 6 years ago Closed 6 years ago

Update notification icons to use photon assets

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(2 files)

Bug 1392416 - Update notification icons to use photon assets.
59 bytes, text/x-review-board-request
dao
: review+
Details
Screen Shot 2017-09-19 at 12.43.21 PM.png
12.35 KB, image/png
Details 
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.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Whiteboard: [photon-visual][triage]
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
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)
(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 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 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+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c268fc166a1
Update notification icons to use photon assets. r=dao
https://hg.mozilla.org/mozilla-central/rev/4c268fc166a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.2 - Aug 29
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)
(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.