Closed Bug 1397115 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4])

Attachments

(2 files)

Attached image 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.
Assignee: nobody → schung
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.
Attachment #8911730 - Flags: review?(MattN+bmo)
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
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".
Flags: needinfo?(bbell)
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
Flags: needinfo?(bbell)
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?
Attachment #8911730 - Flags: review?(jhofmann) → review-
(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 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.
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.
Flags: needinfo?(fshih)
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.
Attachment #8911730 - Flags: review?(jhofmann) → review+
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?
Flags: needinfo?(fshih) → needinfo?(jhofmann)
Keywords: checkin-needed
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
You should probably talk to Bryan Bell or Stephen Horlander, yes. :)
Flags: needinfo?(jhofmann)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aae33bcfef8b
Update formautofill doorhanger anchor icon and adjust the icon margin space in identity box. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aae33bcfef8b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.