Implement new identity block appearance

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

(Whiteboard: [photon-visual][p1][57])

Attachments

(5 attachments, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → dale
Whiteboard: [photon-visual][p1][57]
Summary: Implement new identity bar appearance → Implement new identity block appearance
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Flags: qe-verify?
Priority: -- → P1
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
(Assignee)

Comment 1

2 years ago
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?
Flags: needinfo?(shorlander)
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?
(Assignee)

Comment 3

2 years ago
I will take a shot at doing the former version and see if I run into problems
(Assignee)

Comment 4

2 years ago
Suspect this isnt exhaustive but working version of the first UX spec, it seems to work well for me.
Flags: needinfo?(shorlander)
Attachment #8867778 - Flags: feedback?(dao+bmo)
Iteration: 55.5 - May 15 → 55.6 - May 29
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
(Assignee)

Comment 6

2 years ago
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)
(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 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).
(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.
Depends on: 1366492
(Assignee)

Comment 10

2 years ago
> 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
Flags: needinfo?(shorlander)
(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...
Attachment #8867778 - Flags: feedback?(dao+bmo)
<shorlander> daleharvey: Updated the colors (at the bottom): https://mozilla.invisionapp.com/share/ENBBK0F9U#/229940647_Toolbars
Flags: needinfo?(shorlander)
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Blocks: 1352366
No longer blocks: photon-visual
(Assignee)

Comment 13

2 years ago
Attachment #8867778 - Attachment is obsolete: true
Attachment #8873762 - Flags: review?(dao+bmo)
(Assignee)

Comment 14

2 years ago
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 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?
(Assignee)

Comment 16

2 years ago
> 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 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.
Attachment #8873762 - Flags: review?(dao+bmo) → review-
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Comment 18

2 years ago
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
Flags: needinfo?(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
Flags: needinfo?(shorlander)
Comment hidden (mozreview-request)
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Attachment #8873762 - Attachment is obsolete: true

Comment 21

2 years ago
mozreview-review
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 {
Attachment #8880315 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 22

2 years ago
Posted patch identity.patch (obsolete) — Splinter Review
Thanks, all those comments made sense + implemented, updated patch and carrying review
Attachment #8880315 - Attachment is obsolete: true
Attachment #8882040 - Flags: review+
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?
Attachment #8882040 - Flags: review+ → review-
(Assignee)

Comment 25

2 years ago
Posted patch identity.patch (obsolete) — Splinter Review
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
Attachment #8882040 - Attachment is obsolete: true
Attachment #8882230 - Flags: review?(dao+bmo)
Attachment #8882230 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 26

2 years ago
Updated selector to fix mochitest failures
Attachment #8882230 - Attachment is obsolete: true
Attachment #8882459 - Flags: review+

Comment 28

2 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2343d4d021
Implement new identity block appearance. r=dao

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc2343d4d021
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1381008
Depends on: 1381117

Updated

2 years ago
Depends on: 1386718
Hi Johann, can you please help us here and tell us what we need to verify? Thanks
Flags: needinfo?(jhofmann)
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
Flags: needinfo?(jhofmann)
Posted video 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.
QA Contact: brindusa.tot → ovidiu.boca
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).
Flags: needinfo?(dharvey)
(Assignee)

Comment 36

2 years ago
Yeh that spacing looks off, filed https://bugzilla.mozilla.org/show_bug.cgi?id=1391279 to investigate / fix
Flags: needinfo?(dharvey)
Depends on: 1391279
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
Flags: needinfo?(dharvey)
(Assignee)

Comment 38

2 years ago
Could you file a new bug for that (and assign me) looks ubuntu specific may need a little more help reproducing, cheers
Flags: needinfo?(dharvey)
Depends on: 1397672
No longer depends on: 1397672
I will close this bug giving the fact that the remaining issue, the one with the Dark theme, is covered by bug 1387726.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.