Only gray permission icons in the identity block should get a gray hover state

VERIFIED FIXED in Firefox 50

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

Trunk
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment)

Currently e.g. the red "plugin blocked" or the "camera sharing" icon get a gray hover state, which looks weird.
Flags: qe-verify+
https://reviewboard.mozilla.org/r/67678/#review64782

::: browser/themes/shared/notification-icons.inc.css:44
(Diff revision 1)
>    width: 64px;
>    height: 64px;
>    margin-inline-end: 10px;
>  }
>  
>  #notification-popup-box > .notification-anchor-icon:hover {

Would a :not(.in-use) here...

::: browser/themes/shared/notification-icons.inc.css:281
(Diff revision 1)
>  
> +/* Override hover color (bug 1290017) */
> +#notification-popup-box > .plugin-icon.plugin-blocked:hover,
>  .plugin-icon.plugin-blocked {
>    list-style-image: url(chrome://browser/skin/notification-icons.svg#plugin-blocked);
>    fill: #d92215;

and a !important here be all we need?
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> https://reviewboard.mozilla.org/r/67678/#review64782
> 
> ::: browser/themes/shared/notification-icons.inc.css:44
> (Diff revision 1)
> >    width: 64px;
> >    height: 64px;
> >    margin-inline-end: 10px;
> >  }
> >  
> >  #notification-popup-box > .notification-anchor-icon:hover {
> 
> Would a :not(.in-use) here...

Yes, that would work as well. I'll change it.

> 
> ::: browser/themes/shared/notification-icons.inc.css:281
> (Diff revision 1)
> >  
> > +/* Override hover color (bug 1290017) */
> > +#notification-popup-box > .plugin-icon.plugin-blocked:hover,
> >  .plugin-icon.plugin-blocked {
> >    list-style-image: url(chrome://browser/skin/notification-icons.svg#plugin-blocked);
> >    fill: #d92215;
> 
> and a !important here be all we need?

I think that depends on how you see it. I personally think that !important doesn't convey WHY it has to be important and will cause issues and questions in the long run. That rule is a bit more verbose but at least it clearly shows what use case we're overriding instead doing a catch-all.

What do you think?
(In reply to Johann Hofmann [:johannh] from comment #3)

> > > +/* Override hover color (bug 1290017) */
> > > +#notification-popup-box > .plugin-icon.plugin-blocked:hover,
> > >  .plugin-icon.plugin-blocked {
> > >    list-style-image: url(chrome://browser/skin/notification-icons.svg#plugin-blocked);
> > >    fill: #d92215;
> > 
> > and a !important here be all we need?
> 
> I think that depends on how you see it. I personally think that !important
> doesn't convey WHY it has to be important and will cause issues and
> questions in the long run. That rule is a bit more verbose but at least it
> clearly shows what use case we're overriding instead doing a catch-all.
> 
> What do you think?

I think the !important needs a comment on the line before, something like: /* !important to preserve this color even on hover */
Comment on attachment 8775500 [details]
Bug 1290017 - Override hover state for non-gray permission icons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67678/diff/1-2/
Fine by me :)
Comment on attachment 8775500 [details]
Bug 1290017 - Override hover state for non-gray permission icons.

https://reviewboard.mozilla.org/r/67678/#review64818

Looks good, thanks!
Attachment #8775500 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/2b3f0b8a6318025d4c51818c0b9d65199ec0c4d3
Bug 1290017 - Override hover state for non-gray permission icons. r=florian
https://hg.mozilla.org/mozilla-central/rev/2b3f0b8a6318
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Iteration: --- → 50.4 - Aug 1
Verified fixed FX 50.0a1 (2016-08-01) Win 7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.