Closed Bug 1302307 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Site Identity and Permission Panels, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: magicp.jp, Assigned: dao)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(2 files, 1 obsolete file)

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.
Has STR: --- → yes
Component: Untriaged → Site Identity and Permission Panels
OS: Unspecified → All
Hardware: Unspecified → All
Component: Site Identity and Permission Panels → Theme
Keywords: regression
Whiteboard: [fxprivacy] [triage]
Component: Theme → Site Identity and Permission Panels
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
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.
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.
Attached patch patchSplinter Review
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.
(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+
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
Attachment #8793303 - Attachment is obsolete: true
Attachment #8793303 - Flags: review?(dao+bmo)
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
https://hg.mozilla.org/mozilla-central/rev/52fc6a1fb0a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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)
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+
You need to log in before you can comment on or make changes to this bug.