blocked-permission-icon margin should only apply within the identity block

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Site Identity and Permission Panels
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: magicp, Assigned: dao)

Tracking

({regression})

Trunk
Firefox 52
regression
Points:
---

Firefox Tracking Flags

(firefox49 unaffected, firefox50 unaffected, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8790550 [details]
identity-popup-permission-icon.blocked-permission-icon.png

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

Steps to reproduce:

1. Start Nightly
2. Go to any sites
3. View Page Info > Permissions
4. Check-off "Use Default" and then select "Allow" or "Block" randomly
5. Close Page Info
6. Click "Show site information" icon in identity-box
7. Check horizontal alignment of identity popup permission icons


Actual results:

identity popup permission icons are not horizontal align left. Because identity-popup-permission-icon and blocked-permission-icon have different margin. Please find attached image.


Expected results:

identity-popup-permission-icon and blocked-permission-icon have same margin.
(Reporter)

Updated

2 years ago
Has STR: --- → yes
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
Component: Untriaged → Site Identity and Permission Panels
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Updated

2 years ago
status-firefox50: affected → unaffected
(Assignee)

Updated

2 years ago
Component: Site Identity and Permission Panels → Theme
Keywords: regression
Whiteboard: [fxprivacy] [triage]
Component: Theme → Site Identity and Permission Panels

Updated

2 years ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Comment hidden (mozreview-request)
This undoes removing the "blocked" class in Bug 1297405, the original thought was that "blocked-permission-icon" and "blocked" could be used interchangeably, but we really need separate classes for the strikethrough style and the icon size/positioning.
(Assignee)

Comment 6

2 years ago
I'm not sure I follow why we need a second class here, but in any case please don't re-introduce "blocked", the name is too generic.
(Assignee)

Comment 7

2 years ago
Created attachment 8793326 [details] [diff] [review]
patch

Doesn't this fix the bug? Is there something else I'm missing?
Attachment #8793326 - Flags: feedback?(jhofmann)
Yeah that fixes it, but I'm not a big fan of these two elements sharing the "blocked-permission-icon" class. Anytime you change the blocked icons in the identity-block it affects the control center, and vice-versa. Prepending "#blocked-permissions-container" to "blocked-permission-icon" for the second time hints that we should just have separate classes.

We can figure out a new name.
(Assignee)

Comment 9

2 years ago
(In reply to Johann Hofmann [:johannh] from comment #8)
> Yeah that fixes it, but I'm not a big fan of these two elements sharing the
> "blocked-permission-icon" class. Anytime you change the blocked icons in the
> identity-block it affects the control center, and vice-versa.

That sounds like a feature, not a bug. In what case would we want the icons to be different?

> Prepending
> "#blocked-permissions-container" to "blocked-permission-icon" for the second
> time hints that we should just have separate classes.

Not necessarily. It just means that we have two distinct contexts for the class. This seems fine as long as these contexts don't need significantly different treatment. One or two CSS rules using "#blocked-permissions-container >" doesn't really demonstrate that.
Attachment #8793303 - Flags: review?(florian) → review?(dao+bmo)
(In reply to Johann Hofmann [:johannh] from comment #8)
> Prepending
> "#blocked-permissions-container" to "blocked-permission-icon" for the second
> time hints that we should just have separate classes.

Or more thorough reviews the next time we touch this css file. Or an additional comment to make us careful next time we touch this, if there's a sensible place to put such a comment.
So I initially did what you proposed and thought it was a bit hacky but if you think that this is the better way to go this is fine by me.

You're right in that there are also cases where we might want changes to affect both.

Let's go for your solution :)
Comment on attachment 8793326 [details] [diff] [review]
patch

I'll just straight-up r+ if you don't mind. Not a very complex patch.
Attachment #8793326 - Flags: feedback?(jhofmann) → review+

Comment 13

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52fc6a1fb0a7
Set margin on .blocked-permission-icon only when part of the identity block. r=jhofmann
(Assignee)

Updated

2 years ago
Attachment #8793303 - Attachment is obsolete: true
Attachment #8793303 - Flags: review?(dao+bmo)
(Assignee)

Updated

2 years ago
Assignee: jhofmann → dao+bmo
Summary: Fix margin of identity-popup-permission-icon and blocked-permission-icon for horizontal align left → blocked-permission-icon margin should only apply within the identity block

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52fc6a1fb0a7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

2 years ago
Iteration: --- → 52.1 - Oct 3
Hi :dao,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8793326 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1298530
[User impact if declined]: slight misalignment in the site identity panel
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: CSS selector fixed, low risk
[String/UUID change made/needed]: none
Flags: needinfo?(dao+bmo)
Attachment #8793326 - Flags: approval-mozilla-aurora?
Comment on attachment 8793326 [details] [diff] [review]
patch

Fix a regression. Take it in 51 aurora.
Attachment #8793326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4809591f9c2
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.