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

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Johann Hofmann [:johannh]]

Currently e.g. the red "plugin blocked" or the "camera sharing" icon get a gray hover state, which looks weird.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1290017 - Override hover state for non-gray permission icons.

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

Comment 2

[Florian Quèze [:florian]]

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?

Comment 3

[Johann Hofmann [:johannh]]

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

Comment 4

[Florian Quèze [:florian]]

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

[Johann Hofmann [:johannh]]

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/

Comment 6

[Johann Hofmann [:johannh]]

Fine by me :)

Comment 7

[Florian Quèze [:florian]]

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!

Comment 8

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/fx-team/rev/2b3f0b8a6318025d4c51818c0b9d65199ec0c4d3
Bug 1290017 - Override hover state for non-gray permission icons. r=florian

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b3f0b8a6318

Comment 10

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50.0a1 (2016-08-01) Win 7