1397115 - [Form Autofill] Fine tune the autofill doorhanger's anchor position

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P5)

Description

[Steve Chung [:steveck]]

Attachment: icon_margin.png

Since visual designer feels the anchor is not align to the center, we might need to replace the icon image and adjust the margin between icons.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

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

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8911730 [details]
Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

https://reviewboard.mozilla.org/r/183138/#review188572

::: browser/extensions/formautofill/content/formfill-anchor.svg:4
(Diff revision 1)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> -<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="context-fill" fill-opacity="context-fill-opacity">
> +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16">

Are you sure we want to revert @fill @fill-opacity and @width and @height?

::: browser/themes/shared/notification-icons.inc.css:16
(Diff revision 1)
>    margin-inline-end: -5px;
> -  padding-inline-end: 5px;
> +  padding-inline-start: 2px;
> +  padding-inline-end: 6px;
>  }

Since this affects more than autofill and there are specs for this for Photon, I think johannh should review this patch.

Comment 3

[Matthew N. [:MattN]]

It would also be really helpful if the screenshot tests were update to be able to see what this looks like on all platforms. It could be added to:
https://dxr.mozilla.org/mozilla-central/rev/7e962631ba4298bcefa571008661983d77c3e652/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm#29

It's probably worth running a try push to capture the screenshots using the syntax at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots

Comment 4

[Johann Hofmann [:johannh]]

Unfortunately there aren't really Photon specs for the spacing of notification/permission icons in the identity block. Bryan, do you think you can give us a quick mockup of what it should look like, considering that Form Autofill UX thinks the items are too close?

It would be quite helpful to get an exact number for horizontal margin between the items instead of "2px more" and "1px more".

Comment 5

[Johann Hofmann [:johannh]]

So the Photon spec says there should be 4px between the identity icon and the green lock, which I'll take as a general distance for icons in the identity block:

https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229940647

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8911730 [details]
Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

https://reviewboard.mozilla.org/r/183138/#review188906

::: browser/themes/shared/notification-icons.inc.css:17
(Diff revision 1)
>  
>  #notification-popup-box {
>    padding: 5px 0px;
>    margin: -5px 0px;
>    margin-inline-end: -5px;
> -  padding-inline-end: 5px;
> +  padding-inline-start: 2px;

This will cause permission icons to jump when the permission is denied, so #blocked-permissions-container would need to get the same padding.

I'm wondering, however, if you rather want to apply this padding to all notification icons (https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/themes/shared/identity-block/identity-block.inc.css#85) and not the notification container. 

We should probably wait with that until we have a number from Bryan or someone else.

::: browser/themes/shared/notification-icons.inc.css:17
(Diff revision 1)
>  
>  #notification-popup-box {
>    padding: 5px 0px;
>    margin: -5px 0px;
>    margin-inline-end: -5px;
> -  padding-inline-end: 5px;
> +  padding-inline-start: 2px;

Setting this alone makes permission icons jump when they're denied because the blocked permission container doesn't have the same padding. It will also cause the identity block icons to have inconsistent padding. Instead, I'd suggest just increasing this value:

https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/themes/shared/identity-block/identity-block.inc.css#85

to 4px.

You will also have to adjust the negative margin of the hidden tracking protection icon by two:

https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/themes/shared/identity-block/identity-block.inc.css#172

This should make the identity icons have the expected Photon padding, which is luckily also a bit larger and should be what you want.

::: browser/themes/shared/notification-icons.inc.css:18
(Diff revision 1)
>  #notification-popup-box {
>    padding: 5px 0px;
>    margin: -5px 0px;
>    margin-inline-end: -5px;
> -  padding-inline-end: 5px;
> +  padding-inline-start: 2px;
> +  padding-inline-end: 6px;

Hm, I'm not sure you should do this. The identity icon has 2px (4px with the changes above) of left padding and that should be consistent with the padding of the other items. Are you sure that it's off?

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #6)
> Comment on attachment 8911730 [details]
> Bug 1397115 - [Form Autofill] Fine tune the autofill doorhanger's anchor
> position.
> 
> https://reviewboard.mozilla.org/r/183138/#review188906
> 
> ::: browser/themes/shared/notification-icons.inc.css:17
> (Diff revision 1)
> >  
> >  #notification-popup-box {
> >    padding: 5px 0px;
> >    margin: -5px 0px;
> >    margin-inline-end: -5px;
> > -  padding-inline-end: 5px;
> > +  padding-inline-start: 2px;
> 
> This will cause permission icons to jump when the permission is denied, so
> #blocked-permissions-container would need to get the same padding.
> 
> I'm wondering, however, if you rather want to apply this padding to all
> notification icons
> (https://searchfox.org/mozilla-central/rev/
> f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/themes/shared/identity-
> block/identity-block.inc.css#85) and not the notification container. 
> 
> We should probably wait with that until we have a number from Bryan or
> someone else.

Huh, ignore this comment. I initially wrote it and then it didn't show up in mozreview and I thought I had lost it, so I wrote this again in the next review comment. Also we can probably use 4px as mentioned in comment 5.

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8911730 [details]
Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

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

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8911730 [details]
Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

https://reviewboard.mozilla.org/r/183138/#review188906

> Hm, I'm not sure you should do this. The identity icon has 2px (4px with the changes above) of left padding and that should be consistent with the padding of the other items. Are you sure that it's off?

After look into the image icon in id box, I think the root cause is the connection-secure svg left more blank margin than the identity-icon. I guess the better solution is update all the icons and unify the margin in image.

Comment 10

[Steve Chung [:steveck]]

Hi Fang, do you think we should polish other icons in identity block per comment 9? You can enlarge the secure icon(https://raw.githubusercontent.com/mozilla/gecko-dev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/themes/shared/identity-block/connection-secure.svg) a little bit and the issue would be fixed, otherwise you could probably find the asymmetric margin space between other icons if we only update the notification anchor's position.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8911730 [details]
Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

https://reviewboard.mozilla.org/r/183138/#review192656

r=me with the correct margin, thank you!

::: commit-message-8e818:1
(Diff revision 2)
> +Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identify box. r=johannh

Nit: identity box

::: browser/themes/shared/identity-block/identity-block.inc.css:172
(Diff revision 2)
>  #tracking-protection-icon[animate] {
>    transition: margin-left 200ms ease-out, margin-right 200ms ease-out;
>  }
>  
>  #tracking-protection-icon:not([state]) {
> -  margin-inline-end: -18px;
> +  margin-inline-end: -22px;

This should be -20px, you're only increasing the margin by two.

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8911730 [details]
Bug 1397115 - Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box.

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

Comment 13

[Steve Chung [:steveck]]

Hi Johann,
Thanks for the review and suggestions! BTW Fang is wondering who is in charge of the current identity box visual design, because she wants to discuss about the design of other icon. Do you think that Bryan is the best candidate for answering this question?

Comment 14

[Fang Shih [:fang]]

I think the new Photon icon should be center already. If we gonna make any change of the icon padding, we need to confirm with the designer first. Thanks

Comment 15

[Johann Hofmann [:johannh]]

You should probably talk to Bryan Bell or Stephen Horlander, yes. :)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aae33bcfef8b
Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box. r=johannh

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/aae33bcfef8b

Comment 18

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=2ca37e969460f5dfe50b26ba94d55a6182f70d81&newProject=mozilla-central&newRev=a32c32d9631c0bf7c7c283c57c299cd056e85814&filter=permission

Looks good to me.