1367565 - Permission icons in the identity block are black instead of gray on LWTs

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Johann Hofmann [:johannh]]

Mozscreenshots reports that (likely) bug 1365846 caused this and I can confirm it's broken in Nightly.

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=cc65f9233e5b16265ae44b643c7d255556da14fb&newProject=mozilla-central&newRev=3801e9bd76ad91b5754faacd3fec9ad4bc3fc782

STR:

- Use a lightweight theme
- Go to https://permission.site and trigger a permission prompt

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1367565 - Make notification icons use context-fill.

This applies the color rules for other icons in the identity block
to the notification icons via context-fill.

Review commit: https://reviewboard.mozilla.org/r/144828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144828/

Comment 2

[Johann Hofmann [:johannh]]

We should probably split up notification-icons.svg into several files, I'd make a follow-up bug for that.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8873401 [details]
Bug 1367565 - Make notification icons use context-fill.

https://reviewboard.mozilla.org/r/144828/#review148790

::: browser/themes/shared/notification-icons.inc.css:22
(Diff revision 1)
>  
>  .notification-anchor-icon,
>  #blocked-permissions-container > .blocked-permission-icon {
> -  width: 16px;
> -  height: 16px;
>    margin-inline-start: 2px;

This margin should probably be consistent with other icons in the identity block, so can it move to identity-block.inc.css as well?

::: browser/themes/shared/notification-icons.svg
(Diff revision 1)
>  <!-- 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"
> -     xmlns:xlink="http://www.w3.org/1999/xlink"
> -     class="fieldtext"

Just set fill="context-fill" here rather than on every path?

::: browser/themes/shared/notification-icons.svg:20
(Diff revision 1)
>      .blocked {
>        clip-path: url(#blocked-clipPath);
>      }
>  
>      #login-highlighted {
>        fill: HighlightText;

Why did you move the plugin-blocked color to the stylesheet while leaving this in the SVG?

::: browser/themes/shared/notification-icons.svg:27
(Diff revision 1)
> -    }
> -
>      #camera-sharing,
>      #microphone-sharing,
>      #screen-sharing {
>        fill: rgb(224, 41, 29);

ditto

::: browser/themes/shared/notification-icons.svg:34
(Diff revision 1)
>      }
>  
>      #camera-indicator,
>      #microphone-indicator,
>      #screen-indicator {
>        fill: white;

Same question here I suppose, although I'm not even sure what exactly this rule is doing.

::: browser/themes/shared/notification-icons.svg:98
(Diff revision 1)
> -  <use id="geo-windows-blocked" class="blocked" xlink:href="#geo-windows-icon" />
> -  <use id="geo-windows-detailed" xlink:href="#geo-windows-detailed-icon" />
> -  <use id="indexedDB" xlink:href="#indexedDB-icon" />
> -  <use id="indexedDB-blocked" class="blocked" xlink:href="#indexedDB-icon" />
> -  <use id="login" xlink:href="#login-icon" />
> -  <use id="login-highlighted" class="highlighted" xlink:href="#login-icon" />
> +  <use id="geo-windows-blocked" class="blocked" href="#geo-windows-icon" />
> +  <use id="geo-windows-detailed" href="#geo-windows-detailed-icon" />
> +  <use id="indexedDB" href="#indexedDB-icon" />
> +  <use id="indexedDB-blocked" class="blocked" href="#indexedDB-icon" />
> +  <use id="login" href="#login-icon" />
> +  <use id="login-highlighted" class="highlighted" href="#login-icon" />

Remove unused class="highlighted" while you're already touching this?

::: browser/themes/shared/notification-icons.svg:108
(Diff revision 1)
> -  <use id="microphone-blocked" class="blocked" xlink:href="#microphone-icon" />
> -  <use id="microphone-detailed" xlink:href="#microphone-detailed-icon" />
> -  <use id="persistent-storage" xlink:href="#persistent-storage-icon" />
> -  <use id="persistent-storage-blocked" class="blocked" xlink:href="#persistent-storage-icon" />
> -  <use id="plugin" xlink:href="#plugin-icon" />
> -  <use id="plugin-blocked" class="blocked" xlink:href="#plugin-icon" />
> +  <use id="microphone-blocked" class="blocked" href="#microphone-icon" />
> +  <use id="microphone-detailed" href="#microphone-detailed-icon" />
> +  <use id="persistent-storage" href="#persistent-storage-icon" />
> +  <use id="persistent-storage-blocked" class="blocked" href="#persistent-storage-icon" />
> +  <use id="plugin" href="#plugin-icon" />
> +  <use id="plugin-blocked" class="blocked" href="#plugin-icon" />

Remove class="blocked"?

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8873401 [details]
Bug 1367565 - Make notification icons use context-fill.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144828/diff/1-2/

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8873401 [details]
Bug 1367565 - Make notification icons use context-fill.

https://reviewboard.mozilla.org/r/144828/#review148790

> Why did you move the plugin-blocked color to the stylesheet while leaving this in the SVG?

I thought that it was fine and that I could do this when splitting up that SVG in a later bug. But it turns out I need to move these out as well.

> Remove class="blocked"?

Blocked is needed to display the strikethrough path.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8873401 [details]
Bug 1367565 - Make notification icons use context-fill.

https://reviewboard.mozilla.org/r/144828/#review149248

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/344e28229035
Make notification icons use context-fill. r=dao

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/344e28229035

Comment 9

[Sajedul Islam]

I have reproduced this bug with Nightly 55.0a1 (2017-05-24) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 55.0a1 !

Build   ID    20170604030205
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170607]

Comment 10

[Johann Hofmann [:johannh]]

Looks good on screenshots, too:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=c67d811399a1408dd4c77a92584ecf480e3c972b&newProject=mozilla-central&newRev=43039280fe464869428f03b047bb7c762784f44b&filter=permission