|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
Small regression from Bug 1022601.
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.
Iteration: --- → 51.2 - Aug 29
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
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.
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+
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
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?
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
2 years ago
Depends on: 1298530
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.