1383776 - Identity icon isn't horizontally centered on hover

Status: NEW

(Firefox :: Theme, defect, P3)

Description

[afnankhan]

Attachment: Identity icon

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170724030204

Comment 1

[Johann Hofmann [:johannh]]

Yeah I've been annoyed by that as well already but didn't get around to filing it. Thanks for reporting the bug. :)

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1383776 - Shift identity box hover state slightly to the right when there are no security indicators.

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

Comment 3

[Johann Hofmann [:johannh]]

This patch doesn't perfectly center it (it's not possible without making the identity block larger or having no space between the hover background and the url text), but it improves things to the point where I'm personally ok with it.

Dale, what do you think?

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8913612 [details]
Bug 1383776 - Shift identity box hover state slightly to the right when there are no security indicators.

https://reviewboard.mozilla.org/r/185000/#review190080

::: browser/themes/shared/identity-block/identity-block.inc.css:21
(Diff revision 1)
>    overflow: hidden;
>  }
>  
> +#identity-box.unknownIdentity {
> +  /* Give a little more space to the right when
> +   * there's a single icon in the identity block, to

#identity-box.unknownIdentity != "single icon in the identity block"

E.g. there's the insecure login forms icon, tracking protection, permission and notification icons...

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #4)
> Comment on attachment 8913612 [details]
> Bug 1383776 - Shift identity box hover state slightly to the right in
> unknown identity mode.
> 
> https://reviewboard.mozilla.org/r/185000/#review190080
> 
> ::: browser/themes/shared/identity-block/identity-block.inc.css:21
> (Diff revision 1)
> >    overflow: hidden;
> >  }
> >  
> > +#identity-box.unknownIdentity {
> > +  /* Give a little more space to the right when
> > +   * there's a single icon in the identity block, to
> 
> #identity-box.unknownIdentity != "single icon in the identity block"
> 
> E.g. there's the insecure login forms icon, tracking protection, permission
> and notification icons...

Ah, yes, you're right, for some reason I thought the classnames get replaced, e.g. insecure login forms only has the insecureLoginForms classname, not unknownIdentity.

Comment 6

[Johann Hofmann [:johannh]]

In retrospective I'm not really sure what made me think this patch was gonna work, there are also the permission icons (which I added myself) to consider. I'm gonna put up a patch that I'm not really proud of just to make sure I've exhausted all the options available here.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8913612 [details]
Bug 1383776 - Shift identity box hover state slightly to the right when there are no security indicators.

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

Comment 8

[Johann Hofmann [:johannh]]

So this patch has two flaws:

- It's horrible and breaks once anyone changes things in the identity block and doesn't pay attention. (It's hard to test on top of that)
- It doesn't cover anchor icons for permission prompts (because that's a bit harder I think), which is an edge case I could live with.

I'm not putting it up for review because I'm not really sure I want to go through with it but I'm also kind of out of ideas on how to solve this best.

I'll un-assign myself and put this back into the backlog (with low prio). If we think this is indeed a bigger problem I'm happy to push this patch through (or a better patch if anyone has a great idea).