1391279 - Fix identity block spacing

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Comment 1

[Dale Harvey (:daleharvey)]

Spacing @ https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647

Comment 2

[Dale Harvey (:daleharvey)]

Attachment: Bug 1391279 - Fix identity box spacing.

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

Comment 3

[Dale Harvey (:daleharvey)]

So #identity-box here has 8px for both start padding and end padding + margin. Measuring with th a ruler this comes out to 9px of empty space in each case

For the (i) its 16px wide but 14px visible, for the end margin / padding it seems like its just an invisble 1px as part of the calculated area of the text, not certain we want to give them 7px margins to account for that

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

https://reviewboard.mozilla.org/r/169726/#review175280

::: browser/themes/shared/identity-block/identity-block.inc.css:53
(Diff revision 1)
>  #urlbar[pageproxystate=valid] > #identity-box.verifiedIdentity {
>    --urlbar-separator-color: rgba(18, 188, 0, .5);
>  }
>  
>  #identity-box {
>    padding-inline-end: 2px;

Please merge this into the other #identity-box rule and fix up the end padding, because it still isn't right with this patch in the non-verifiedIdentity/chromeUI/extensionPage case.

The space between the (i) and the lock icon also still seems too small.

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: Screen Shot 2017-08-25 at 15.21.51.png

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

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

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

https://reviewboard.mozilla.org/r/169726/#review177980

::: browser/themes/shared/identity-block/identity-block.inc.css:8
(Diff revision 2)
>   * 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/. */
>  %endif
>  
>  #identity-box {
> -  padding: 0 5px;
> +  padding-inline-end: 2px;

You can probably remove this, as it gets overriden by padding: 0 8px;

::: browser/themes/shared/identity-block/identity-block.inc.css:10
(Diff revision 2)
>  %endif
>  
>  #identity-box {
> -  padding: 0 5px;
> +  padding-inline-end: 2px;
> +  margin-inline-end: 2px;
> +  padding: 0 8px;

When hovering the identity box without a label, the end padding looks unbalanced, and doesn't seem to match the mockup: http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html

::: browser/themes/shared/identity-block/identity-block.inc.css:10
(Diff revision 2)
>  %endif
>  
>  #identity-box {
> -  padding: 0 5px;
> +  padding-inline-end: 2px;
> +  margin-inline-end: 2px;
> +  padding: 0 8px;

This also regresses bug 1386718

Comment 8

[Dale Harvey (:daleharvey)]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

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

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

https://reviewboard.mozilla.org/r/169726/#review177984

see my previous review comments (you seem to have fixed the last one -- good!)

Comment 10

[Dale Harvey (:daleharvey)]

Attachment: Screen Shot 2017-08-25 at 16.34.37.png

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

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

Comment 12

[Dale Harvey (:daleharvey)]

Yup sorry was commenting on the last patch when I realised I needed to fix the awesomebar alignment

So the space between the (i) is 4px which is to spec (it actually renders to 5)

> You can probably remove this, as it gets overriden by padding: 0 8px;
Done

> When hovering the identity box without a label, the end padding looks unbalanced, 
> and doesn't seem to match the 
> mockup: http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html

Yup I forgot we had a special case for that, should be fixed in all cases now

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

https://reviewboard.mozilla.org/r/169726/#review178110

::: browser/themes/shared/identity-block/identity-block.inc.css:9
(Diff revision 4)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  %endif
>  
>  #identity-box {
> -  padding: 0 5px;
> +  margin-inline-end: 4px;
> +  padding: 0 4px 0 8px;

This is wrong for RTL. You need to use padding-inline-start and padding-inline-end.

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169726/diff/4-5/

Comment 15

[Dale Harvey (:daleharvey)]

Yeh I wondered since it was editing an existing |padding:| rule, but I guess we only use |padding|  when both sides are equal?

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

https://reviewboard.mozilla.org/r/169726/#review178272

::: browser/themes/shared/identity-block/identity-block.inc.css:10
(Diff revision 5)
>  %endif
>  
>  #identity-box {
> -  padding: 0 5px;
> +  margin-inline-end: 4px;
> +  padding-inline-end: 4px;
> +  padding-inline-start: 8px;

nit, I'd prefer this ordering as it matches the left-to-right position of these spaces:

  padding-inline-start: 8px;
  padding-inline-end: 4px;
  margin-inline-end: 4px;

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 8898358 [details]
Bug 1391279 - Fix identity box spacing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169726/diff/5-6/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fce13e8cc56a
Fix identity box spacing. r=dao

Comment 19

[Phil Ringnalda (:philor)]

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

Comment 20

[Hani Yacoub, Desktop QA]

While trying to verify this bug on the latest nightly (Firefox Nightly 58.0a1) I noted down the results to see if they are the desirable ones:

1. start padding: 9px
2. end padding: 9px
3. distance between "show site information" and "verified by(lock sign)" is 5px
4. distance between "verified by(lock sign)" and "Mozilla corporation (US)" is 7px

After I read all the comments above I still can't figure out if these are the expected results and if there are another areas that should be covered by the bug.

Thanks.

Comment 21

[Dale Harvey (:daleharvey)]

Some of those are off by 1 than the spec, but those are rendering issues, the padding values are as to spec, and yup that is all this bug covers

Comment 22

[Hani Yacoub, Desktop QA]

Based on comment 21 I'll mark this one as verified on Nightly 58.0a1.

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

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 23

[Hani Yacoub, Desktop QA]

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

Verified as fixed on Firefox Beta 57.0b5 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.