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

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[magicp]

Attachment: 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.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=27ffae997b83a5bfeaf0967e692074cb097d94e8&tochange=212b8ad12f3cc173e4f9b03d8e38909fd31277b9

Comment 2

[Matthew N. [:MattN]]

Regression screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=506facea63169a29e04eb140663da1730052db64&newProject=mozilla-central&newRev=401ea746b1a9d515032b66d1368510d09e4e5a8e&filter=allPermissions

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Bug 1302307 - Separate blocked permission icon styles in identity block and control center.

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

Comment 4

[Johann Hofmann [:johannh]]

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.

Comment 5

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be2fa396fe9

Comment 6

[Dão Gottwald [:dao]]

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.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

Doesn't this fix the bug? Is there something else I'm missing?

Comment 8

[Johann Hofmann [:johannh]]

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.

Comment 9

[Dão Gottwald [:dao]]

(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.

Comment 10

[Florian Quèze [:florian]]

(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.

Comment 11

[Johann Hofmann [:johannh]]

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 12

[Johann Hofmann [:johannh]]

Comment on attachment 8793326 [details] [diff] [review]
patch

I'll just straight-up r+ if you don't mind. Not a very complex patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52fc6a1fb0a7
Set margin on .blocked-permission-icon only when part of the identity block. r=jhofmann

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52fc6a1fb0a7

Comment 15

[Gerry Chang [:gchang]]

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?

Comment 16

[Dão Gottwald [:dao]]

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

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8793326 [details] [diff] [review]
patch

Fix a regression. Take it in 51 aurora.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4809591f9c2