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

VERIFIED FIXED in Firefox 51

Status

()

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

People

(Reporter: johannh, Assigned: johannh)

Tracking

Trunk
Firefox 51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [fxprivacy] )

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Small regression from Bug 1022601.
Flags: qe-verify+
Comment hidden (mozreview-request)
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.
Attachment #8783991 - Flags: review?(dao+bmo)

Updated

2 years ago
Iteration: --- → 51.2 - Aug 29
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
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 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.
Attachment #8783991 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8783991 [details]
Bug 1297405 - Don't add hover state to blocked permission icons in the identity block.

Looks good, thanks!
Attachment #8783991 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 8

2 years ago
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
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/15242d578a137703e4781a7595b9d3fa8e2a6072
Bug 1297405 followup - Remove unneeded classname assertion for blocked permission icons. r=me, bustage fix
(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?
(Assignee)

Comment 11

2 years ago
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.
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.
(Assignee)

Updated

2 years ago
See Also: → bug 1297693

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d51679a8d558
https://hg.mozilla.org/mozilla-central/rev/15242d578a13
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
QA Contact: paul.silaghi
Reproduced on Nightly 2016-08-16.
Verified fixed FX 51.0a1 (2016-08-31) Win 7, Ubuntu 14.04, OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.