Implement new identity block appearance

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
7 months ago
2 months 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

7 months ago
Assignee: nobody → dale
Blocks: 1325171
Whiteboard: [photon-visual][p1][57]
Summary: Implement new identity bar appearance → Implement new identity block appearance

Updated

7 months ago
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Flags: qe-verify?
Priority: -- → P1

Updated

7 months ago
Flags: qe-verify? → qe-verify+

Updated

7 months ago
QA Contact: brindusa.tot
(Assignee)

Comment 1

7 months 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

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

Comment 4

6 months ago
Created attachment 8867778 [details] [diff] [review]
Implement new identity block appearance

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)

Updated

6 months ago
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

6 months 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)

Comment 7

6 months ago
(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

6 months ago
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

6 months ago
(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

6 months 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...

Updated

6 months ago
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)

Updated

6 months ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12

Updated

6 months ago
Blocks: 1352366
No longer blocks: 1325171
(Assignee)

Comment 13

6 months ago
Created attachment 8873762 [details] [diff] [review]
Implement new identity block appearance
Attachment #8867778 - Attachment is obsolete: true
Attachment #8873762 - Flags: review?(dao+bmo)
(Assignee)

Comment 14

6 months 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

6 months 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-

Updated

6 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Comment 18

5 months 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)

Updated

5 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Updated

5 months ago
Attachment #8873762 - Attachment is obsolete: true

Comment 21

5 months 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

5 months ago
Created attachment 8882040 [details] [diff] [review]
identity.patch

Thanks, all those comments made sense + implemented, updated patch and carrying review
Attachment #8880315 - Attachment is obsolete: true
Attachment #8882040 - Flags: review+
(Assignee)

Comment 23

5 months ago
Pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e9b342c6ad9b4c2071c94fd0e0567c97ef15cd
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

5 months ago
Created attachment 8882230 [details] [diff] [review]
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
Attachment #8882040 - Attachment is obsolete: true
Attachment #8882230 - Flags: review?(dao+bmo)

Updated

5 months ago
Attachment #8882230 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 26

5 months ago
Created attachment 8882459 [details] [diff] [review]
identity.patch

Updated selector to fix mochitest failures
Attachment #8882230 - Attachment is obsolete: true
Attachment #8882459 - Flags: review+
(Assignee)

Comment 27

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

Comment 28

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

Comment 29

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc2343d4d021
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1377785
Depends on: 1377786
Depends on: 1377794
Depends on: 1377931
Depends on: 1381008
Depends on: 1381117

Updated

4 months ago
Depends on: 1386718

Comment 30

3 months ago
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)

Comment 32

3 months ago
Created attachment 8897829 [details]
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.

Updated

3 months ago
QA Contact: brindusa.tot → ovidiu.boca

Comment 33

3 months ago
Created attachment 8898218 [details]
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).
Flags: needinfo?(dharvey)

Comment 34

3 months ago
Created attachment 8898219 [details]
Screen Shot 2017-08-17 at 12.31.16 PM.png

Comment 35

3 months ago
Created attachment 8898220 [details]
Screen Shot 2017-08-17 at 12.31.40 PM.png
(Assignee)

Comment 36

3 months ago
Yeh that spacing looks off, filed https://bugzilla.mozilla.org/show_bug.cgi?id=1391279 to investigate / fix
Flags: needinfo?(dharvey)

Updated

3 months ago
Depends on: 1391279

Comment 37

3 months ago
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

3 months 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)

Updated

3 months ago
Depends on: 1397672
Depends on: 1387726

Updated

3 months ago
No longer depends on: 1397672

Comment 39

2 months ago
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
status-firefox57: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.