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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks: 1 bug, {regression})

unspecified
Firefox 55
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [photon-visual])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
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

Updated

3 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [photon-visual]
(Assignee)

Updated

3 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED

Updated

3 months ago
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

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

Comment 3

3 months ago
mozreview-review
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"?
Attachment #8873401 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
mozreview-review-reply
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

3 months ago
mozreview-review
Comment on attachment 8873401 [details]
Bug 1367565 - Make notification icons use context-fill.

https://reviewboard.mozilla.org/r/144828/#review149248
Attachment #8873401 - Flags: review?(dao+bmo) → review+

Comment 7

3 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/344e28229035
Make notification icons use context-fill. r=dao

Comment 8

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/344e28229035
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 9

2 months ago
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]

Updated

2 months ago
Blocks: 1359073
(Assignee)

Comment 10

2 months ago
Looks good on screenshots, too:

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

Updated

2 months ago
Flags: qe-verify+
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.