1363502 - Implement new identity block appearance

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Comment 1

[Dale Harvey (:daleharvey)]

Hey was wondering what was up with this, 

https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html
differs from https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/231292511

And Dao mentioned there were maybe changes from what was originally planned, should I hang off on this?

Comment 2

[Johann Hofmann [:johannh]]

The main thing to decide is if we want a grey background on hover (like in shorlanders live mockup) or keep the old behavior and fill the (i) icon. Visually I'm a fan of the former option (grey background), but you need to be careful not to apply the effect when hovering notification anchors.

Is there any other change that I'm missing?

Comment 3

[Dale Harvey (:daleharvey)]

I will take a shot at doing the former version and see if I run into problems

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: Implement new identity block appearance

Suspect this isnt exhaustive but working version of the first UX spec, it seems to work well for me.

Comment 5

[Johann Hofmann [:johannh]]

Please make sure to increase the font size of the identity block to 1em (it's currently 0.9 which makes the EV text smaller and slightly higher than the url bar text).

https://searchfox.org/mozilla-central/source/browser/themes/shared/identity-block/identity-block.inc.css#18

Comment 6

[Dale Harvey (:daleharvey)]

So this patch does take out the .9em, however I did miss that I should add #identity-box to anywhere we set the urlbar font size, they should be the same according to spec (so 1.15em on windows, 1.25em on osx)

Comment 7

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> Please make sure to increase the font size of the identity block to 1em
> (it's currently 0.9 which makes the EV text smaller and slightly higher than
> the url bar text).

Dale's patch was already doing this.

(In reply to Dale Harvey (:daleharvey) from comment #6)
> So this patch does take out the .9em, however I did miss that I should add
> #identity-box to anywhere we set the urlbar font size, they should be the
> same according to spec (so 1.15em on windows, 1.25em on osx)

No, #identity-box is inside #urlbar and will automatically pick up that font size.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8867778 [details] [diff] [review]
Implement new identity block appearance

>--- a/browser/themes/shared/identity-block/connection-secure.svg
>+++ b/browser/themes/shared/identity-block/connection-secure.svg
>@@ -1,17 +1,17 @@
> <?xml version="1.0" encoding="utf-8"?>
> <!-- 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" xmlns:xlink="http://www.w3.org/1999/xlink"
>      width="16" height="16" viewBox="0 0 16 16">
>   <style>
>     .icon-default {
>-      fill: #4d9a26;
>+      fill: #16DA00;

Need to update the label color too, not just the icon.

According to http://contrastchecker.com/, #16DA00 won't provide sufficient contrast on a white background. We need to pick a darker green (without regressing the black background case).

Comment 9

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Johann Hofmann [:johannh] from comment #5)
> > Please make sure to increase the font size of the identity block to 1em
> > (it's currently 0.9 which makes the EV text smaller and slightly higher than
> > the url bar text).
> 
> Dale's patch was already doing this.

I spun this off to bug 1366492 in an effort start landing bits that don't depend on the more complicated changes.

Comment 10

[Dale Harvey (:daleharvey)]

> Need to update the label color too, not just the icon.

> According to http://contrastchecker.com/, #16DA00 won't provide sufficient 
> contrast on a white background. We need to pick a darker green 
> (without regressing the black background case).

Contrast checker seems like it would be for text and not iconography right?

From the spec it was hard to tell whether the label was to change color at all, https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/231292511 looks similiar and too hard to color pick, Steven could you confirm the icon + label colors here? Cheers

Comment 11

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) from comment #10)
> > Need to update the label color too, not just the icon.
> 
> > According to http://contrastchecker.com/, #16DA00 won't provide sufficient 
> > contrast on a white background. We need to pick a darker green 
> > (without regressing the black background case).
> 
> Contrast checker seems like it would be for text and not iconography right?

Mostly yes, although obviously the icon needs to have sufficient contrast too so that people with less than perfect eyesight can see it properly. Also, I was working under the assumption that the icon and label should use the same color...

Comment 12

[Dão Gottwald [:dao]]

<shorlander> daleharvey: Updated the colors (at the bottom): https://mozilla.invisionapp.com/share/ENBBK0F9U#/229940647_Toolbars

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Implement new identity block appearance

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 8873762 [details] [diff] [review]
Implement new identity block appearance

Looking at this again, I wasnt sure what to do with 

#identity-box.grantedPermissions > #identity-icon {
  list-style-image: url(chrome://browser/skin/identity-icon-notice.svg);
}

#identity-box.grantedPermissions:not(.no-hover):hover > #identity-icon,
#identity-box.grantedPermissions[open=true] > #identity-icon {
  list-style-image: url(chrome://browser/skin/identity-icon-notice-hover.svg);
}

but should probably leave -notice.svg as it, but put notice-hover behind non photon only to be removed

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8873762 [details] [diff] [review]
Implement new identity block appearance

>+%ifndef MOZ_PHOTON_THEME
> .urlbar-input-box {
>   margin: 0;
> }

What's the story behind this change?

Comment 16

[Dale Harvey (:daleharvey)]

> What's the story behind this change?

The margin: 0 collapses the default margin between the identity icon and the urlbar text and replaces it with padding so the seperator can be set as a left aligned background image and appears visually in the middle of the identity icon and the text. This patch / the mockup gets rid of the seperator so this isnt needed

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8873762 [details] [diff] [review]
Implement new identity block appearance

>+#connection-icon {
>+  -moz-context-properties: fill;

-moz-context-properties is already set.

>+%ifdef MOZ_PHOTON_THEME
>+  fill: #12BC00;
>+%else
>+  fill: #4d9a26;
>+%endif
>+}

Looks like you're trying to make #connection-icon green regardless of what icon it shows? We only want this for the lock.

> .urlbar-input-box,
> #urlbar-display-box {
>   padding-inline-start: 4px;
>   border-inline-start: 1px solid var(--urlbar-separator-color);
>   border-image: linear-gradient(transparent 15%, var(--urlbar-separator-color) 15%, var(--urlbar-separator-color) 85%, transparent 85%);
>   border-image-slice: 1;
> }

According to the mockup, we still need the separator for the verified identity case.

Comment 18

[Dale Harvey (:daleharvey)]

Stephen, can you clarify what we should be doing here, the seperator is in the invision mockup but not in the live mockups (https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html) having it only show up in the verified identity case seems wrong. Cheers

Comment 19

[Stephen Horlander [:shorlander]]

(In reply to Dale Harvey (:daleharvey) from comment #18)
> Stephen, can you clarify what we should be doing here, the seperator is in
> the invision mockup but not in the live mockups
> (https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html)
> having it only show up in the verified identity case seems wrong. Cheers

Yeah that was intentional. It's really only needed as a visual separator between two disparate blocks of text. I updated the spec: https://cl.ly/2A3o2v15070d

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: Bug 1363502 - Implement new identity block appearance.

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

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8880315 [details]
Bug 1363502 - Implement new identity block appearance.

https://reviewboard.mozilla.org/r/151666/#review158110

::: browser/themes/shared/controlcenter/connection.svg:32
(Diff revision 1)
>        <use xlink:href="#shape-lock-base" fill="#fff"/>
>      </mask>
>    </defs>
>  
>    <rect id="connection-degraded" class="fieldtext" width="24" height="24" mask="url(#mask-lock)"/>
> -  <rect id="connection-secure" width="24" height="24" mask="url(#mask-lock)"/>
> +  <rect id="connection-secure" width="24" height="24" mask="url(#mask-lock)" fill="context-fill" />

nit: drop space before />

::: browser/themes/shared/identity-block/connection-secure.svg:20
(Diff revision 1)
>        <use xlink:href="#shape-lock-clasp-inner" fill="#000" />
>      </mask>
>    </defs>
>  
> -  <use xlink:href="#shape-lock-clasp-outer" mask="url(#mask-clasp-cutout)" class="icon-default" />
> -  <use xlink:href="#shape-lock-base" class="icon-default" />
> +  <use xlink:href="#shape-lock-clasp-outer" mask="url(#mask-clasp-cutout)" class="icon-default" fill="context-fill" />
> +  <use xlink:href="#shape-lock-base" class="icon-default" fill="context-fill" />

ditto

::: browser/themes/shared/identity-block/identity-block.inc.css:58
(Diff revision 1)
>  
> +%ifdef MOZ_PHOTON_THEME
> +#identity-box:not(#identity-box.chromeUI) {
> +  --urlbar-separator-color: transparent;
> +}
> +#urlbar[pageproxystate=valid] #identity-box.chromeUI {

Use > please

::: browser/themes/shared/identity-block/identity-block.inc.css:59
(Diff revision 1)
> +%ifdef MOZ_PHOTON_THEME
> +#identity-box:not(#identity-box.chromeUI) {
> +  --urlbar-separator-color: transparent;
> +}
> +#urlbar[pageproxystate=valid] #identity-box.chromeUI {
> +  --urlbar-separator-color: var(--urlbar-light-separator-color);

It looks like you can get rid of this rule and the --urlbar-light-separator-color variable.

::: browser/themes/shared/identity-block/identity-block.inc.css:61
(Diff revision 1)
> +  --urlbar-separator-color: transparent;
> +}
> +#urlbar[pageproxystate=valid] #identity-box.chromeUI {
> +  --urlbar-separator-color: var(--urlbar-light-separator-color);
> +}
> +#urlbar[pageproxystate=valid] #identity-box.verifiedIdentity {

>

::: browser/themes/shared/identity-block/identity-block.inc.css:72
(Diff revision 1)
> +  padding-inline-end: 2px;
> +  margin-inline-end: 2px;
> +}
> +
> +#urlbar[pageproxystate=valid] #identity-box.verifiedIdentity,
> +#urlbar[pageproxystate=valid] #identity-box.chromeUI {

>

::: browser/themes/shared/identity-block/identity-block.inc.css:87
(Diff revision 1)
> +  border-inline-end: 1px solid var(--urlbar-light-separator-color);
> +  border-image: linear-gradient(transparent 15%, var(--urlbar-light-separator-color) 15%, var(--urlbar-light-separator-color) 85%, transparent 85%);
> +  border-image-slice: 1;
> +}
> +
> +#urlbar-display-box label {

.urlbar-display {

Comment 22

[Dale Harvey (:daleharvey)]

Attachment: identity.patch

Thanks, all those comments made sense + implemented, updated patch and carrying review

Comment 23

[Dale Harvey (:daleharvey)]

Pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e9b342c6ad9b4c2071c94fd0e0567c97ef15cd

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8882040 [details] [diff] [review]
identity.patch

>+#urlbar[pageproxystate=valid] > #identity-box.verifiedIdentity,
>+#urlbar[pageproxystate=valid] > #identity-box.chromeUI {
>+  padding-inline-end: 4px;
>+  margin-inline-end: 4px;
>+  border-inline-end: 1px solid var(--urlbar-separator-color);
>+  border-image: linear-gradient(transparent 15%, var(--urlbar-separator-color) 15%, var(--urlbar-separator-color) 85%, transparent 85%);
>+  border-image-slice: 1;
>+}
>+
>+#urlbar-display-box {
>+  margin-inline-end: 4px;
>+  border-inline-end: 1px solid var(--urlbar-light-separator-color);
>+  border-image: linear-gradient(transparent 15%, var(--urlbar-light-separator-color) 15%, var(--urlbar-light-separator-color) 85%, transparent 85%);
>+  border-image-slice: 1;
>+}

The second rule is using the wrong variable. Can you merge it with the first rule? The variable aside, the only difference seems to be the padding, but #urlbar-display-box should probably have that too?

Comment 25

[Dale Harvey (:daleharvey)]

Attachment: identity.patch

Made changes based on daos feedback, also restored the double seperators in the "Switch to tab" state. Try run showed failures due to now unreferenced files so ifdeffed those out

Comment 26

[Dale Harvey (:daleharvey)]

Attachment: identity.patch

Updated selector to fix mochitest failures

Comment 27

[Dale Harvey (:daleharvey)]

try run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fa563f5e50e1004a5d82080c41bf5478b393922&selectedJob=111162274 (1 unrelated failure)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2343d4d021
Implement new identity block appearance. r=dao

Comment 29

[Phil Ringnalda (:philor)]

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

Comment 30

[ovidiu boca[:Ovidiu]]

Hi Johann, can you please help us here and tell us what we need to verify? Thanks

Comment 31

[Johann Hofmann [:johannh]]

The identity block (the area with the green lock on the left of the urlbar) should look like in the spec: https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647

Comment 32

[ovidiu boca[:Ovidiu]]

Attachment: out.ogv

Thanks Johann, I verified this on Mac OS X 10.10, Windows 7 x64, Windows 10 x64 and Ubuntu 16.04 with the latest Nightly 57.0a1(2017-08-15) and I can confirm that the identity block looks like in the specs. On Ubuntu, I think there is a problem when you hover the mouse over the identity block area the color from URL bar changes and the color from the identity block remains the same, please see the attached video.

Comment 33

[ovidiu boca[:Ovidiu]]

Attachment: Screen Shot 2017-08-17 at 12.29.51 PM.png

I also verified the distances from the green lock to the "i", also space between the green line and the first leather from the URL bar and the green line and the word (US), from spec there need to have 4px and 8px. Please see the attachments with the actual results.
Note: The test were made on Mac OS X 10.12 with Nightly 57.0a1(2017-08-16).

Comment 34

[ovidiu boca[:Ovidiu]]

Attachment: Screen Shot 2017-08-17 at 12.31.16 PM.png

Comment 35

[ovidiu boca[:Ovidiu]]

Attachment: Screen Shot 2017-08-17 at 12.31.40 PM.png

Comment 36

[Dale Harvey (:daleharvey)]

Yeh that spacing looks off, filed https://bugzilla.mozilla.org/show_bug.cgi?id=1391279 to investigate / fix

Comment 37

[ovidiu boca[:Ovidiu]]

For comment 33 I see that bug 1291279 covers this, but please tell me what about comment 32, what should be done with this? Filed a new bug? Thanks

Comment 38

[Dale Harvey (:daleharvey)]

Could you file a new bug for that (and assign me) looks ubuntu specific may need a little more help reproducing, cheers

Comment 39

[ovidiu boca[:Ovidiu]]

I will close this bug giving the fact that the remaining issue, the one with the Dark theme, is covered by bug 1387726.