1297405 - "Blocked" permission icons in the identity block should not get a hover state

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Johann Hofmann [:johannh]]

Small regression from Bug 1022601.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

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

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8783991 [details]
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

Ugh, why are we reusing the notification-anchor-icon class for these? They aren't notification anchors.

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8783991 [details]
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

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

Comment 4

[Johann Hofmann [:johannh]]

Yeah good point, I didn't really think it was a problem since these were the applied the same styles but as we can see it can lead to misunderstandings once they need to behave differently.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8783991 [details]
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

I think you can now get rid of the "blocked" class, too. Just use blocked-permission-icon instead.

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8783991 [details]
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

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

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8783991 [details]
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

Looks good, thanks!

Comment 8

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/fx-team/rev/d51679a8d5588d992d9d710b67cb4e69c58298c7
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block. r=dao

Comment 9

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/fx-team/rev/15242d578a137703e4781a7595b9d3fa8e2a6072
Bug 1297405 followup - Remove unneeded classname assertion for blocked permission icons. r=me, bustage fix

Comment 10

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #9)
> https://hg.mozilla.org/integration/fx-team/rev/
> 15242d578a137703e4781a7595b9d3fa8e2a6072
> Bug 1297405 followup - Remove unneeded classname assertion for blocked
> permission icons. r=me, bustage fix

Hmm, that check doesn't really seem unneeded to me. Shouldn't it just look for blocked-permission-icon instead?

Comment 11

[Johann Hofmann [:johannh]]

Well the check is really unnecessary, because we're using dedicated icons that will always have the classname and blocked styling. If they're shown they will show as blocked.

I think this test wasn't fully updated from an old WIP patch where the notification anchors and the blocked icons where the same element. So yes, we should definitely update the selector to query for .blocked-permission-icon[data-permission-id=....].

Are you ok with making a new (good first?) bug for it? I wouldn't say solving this is super urgent, unless someone introduces a [data-permission-id] attribute somewhere else in the identity block.

Comment 12

[Dão Gottwald [:dao]]

Right, it's not clear at all that only "blocked permission" icons have the data-permission-id attribute. So I guess right now the check is unneeded, but it would make the test more future-proof.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d51679a8d558
https://hg.mozilla.org/mozilla-central/rev/15242d578a13

Comment 14

[Paul Silaghi, QA [:pauly]]

Reproduced on Nightly 2016-08-16.
Verified fixed FX 51.0a1 (2016-08-31) Win 7, Ubuntu 14.04, OS X 10.12.