Padlock icon inconsistent

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: alberts, Assigned: daleharvey)

Tracking

({nightly-community, ux-consistency})

57 Branch
Firefox 57
nightly-community, ux-consistency
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-visual],)

Attachments

(14 attachments)

14.26 KB, image/png
Details
6.75 KB, image/png
Details
21.60 KB, image/png
Details
6.57 KB, image/png
Details
21.44 KB, image/png
Details
59 bytes, text/x-review-board-request
johannh
: review+
Details
684 bytes, image/svg+xml
Details
1.06 KB, image/svg+xml
Details
506 bytes, image/svg+xml
Details
674 bytes, image/svg+xml
Details
6.80 KB, patch
daleharvey
: review+
Details | Diff | Splinter Review
14.46 KB, image/png
Details
5.23 KB, image/png
Details
50.01 KB, image/png
Details
(Reporter)

Description

2 years ago
Posted image padlock-icon.png
After bug 1387614 has landed, the padlock icon is not consistent anymore. I added some examples in the image attached.
Yes, please update the other connection icons as well. Piggy-backing on this bug, the green lock with yellow triangle shouldn't exist anymore. It should be grey with a yellow triangle. I can make a new bug or we fix it in this one.
Blocks: 1387614
Flags: qe-verify+
See Also: bug 1387614
Whiteboard: [photon-visual][triage]
(Assignee)

Updated

2 years ago
Assignee: nobody → dharvey
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: -- → P1
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
12 x 14 - secure connection icon size
15 x 15 - mixed content connection icon size

I don't know what this bug intended to do, but there are 2 options:
- make grey padlock icon size to the size of green padlock icon, so warring sign icon will be pushed more to the bottom, to be padlock icon size consistent
- make whole mixed content connection icon size 1 px smaller, to be whole icon size consistent
(Assignee)

Comment 7

2 years ago
Where were you able to get the green icon with the yellow warning? It doesnt seem like that is in current nightly

So far using:
 https://badssl.com/ = Green padlock
 https://https-everywhere.badssl.com/ = Grey padlock + yellow warning
 http://http-password.badssl.com/ = Grey padlock + red cross out

We dont need to change the icons size, just need to make sure the control center uses the svg as the identity box. 

Stephen do we want to update the related icons here to match the changed padlock? there are:

https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/controlcenter/mcb-disabled.svg
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/identity-block/connection-mixed-passive-loaded.svg
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/identity-block/connection-mixed-active-loaded.svg

Cheers
Flags: needinfo?(shorlander)
(In reply to Dale Harvey (:daleharvey) from comment #7)
> Where were you able to get the green icon with the yellow warning? It doesnt seem like that is in current nightly

I saw one case when doing screenshots for bug 1379247:
Look for the big red line in the lower half of attachment 8895827 [details].
https://self-signed.badssl.com/ > Add an exception
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
(Assignee)

Comment 9

2 years ago
Got that, cheers

> Piggy-backing on this bug, the green lock with yellow triangle shouldn't exist anymore. 
> It should be grey with a yellow triangle. I can make a new bug or we fix it in this one.

I was going to make this change but have a few more questions about which states need to change and since this propgates through to the panel etc probably worth splitting into a new bug if you could? feel free to assign to me
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
Figured replacing the related icons doesnt need to block ensuring these icons are consistent
Attachment #8904482 - Flags: review?(dao+bmo) → review?(jhofmann)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8904482 [details]
Bug 1390511 - Replace inconsistent padlock icons.

https://reviewboard.mozilla.org/r/176334/#review181780

This patch doesn't "Fix inconsistent padlock icons" in a way that's meaningful to me. If you want to re-scope the bug to cover only giving the identity popup the same icon as the identity block (which is fine by me), please update the bug description and file the required follow-up bugs to address the other issues mentioned in this bug. Please also update the commit message.

Thanks!
Attachment #8904482 - Flags: review?(jhofmann)
connection-mixed-active-loaded.svg
connection-mixed-passive-loaded.svg

I think I set those two up correctly.

not-secure-24.svg
secure-24.svg

These are just the artwork. Looks like some things need to be hooked up for it to work as it is currently set up: https://searchfox.org/mozilla-central/source/browser/themes/shared/controlcenter/connection.svg
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8904482 [details]
Bug 1390511 - Replace inconsistent padlock icons.

https://reviewboard.mozilla.org/r/176334/#review184082

r=me with comments addressed. Thank you!

We should make a follow-up bug about cleaning up controlcenter/permissions.svg and tracking-protection.svg to get rid of icon-colors.inc.svg.

::: browser/themes/shared/controlcenter/connection.svg:1
(Diff revision 2)
>  <?xml version="1.0" encoding="utf-8"?>

Nit: you can remove this in all .svg files.

::: browser/themes/shared/controlcenter/mcb-disabled.svg:7
(Diff revision 2)
>  <!-- 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="24" height="24" viewBox="0 0 24 24">
> +<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="context-fill" fill-opacity="context-fill-opacity">
> +  <path d="M18.75 9.977h-.727L6 22h12.75A2.25 2.25 0 0 0 21 19.75v-7.523a2.25 2.25 0 0 0-2.25-2.25zm-9.75 0V7a3 3 0 0 1 6 0v1.5l2.838-2.838A5.994 5.994 0 0 0 6 7v2.977h-.75A2.25 2.25 0 0 0 3 12.227v7.523a2.224 2.224 0 0 0 .105.645L13.523 9.977z"/>
> +  <path d="M2.5 23a1.5 1.5 0 0 1-1.061-2.561l19-19A1.5 1.5 0 0 1 22.56 3.56l-19 19A1.5 1.5 0 0 1 2.5 23z" fill="#ff0039"/>

This is currently getting the fill-opacity resulting in the line being slightly transparent, if that's not desired you should set this to fill-opacity="1" explicitly.

::: browser/themes/shared/controlcenter/panel.inc.css:249
(Diff revision 2)
>  #identity-popup[ciphers=weak] #identity-popup-security-content,
>  #identity-popup[mixedcontent~=passive-loaded][isbroken] #identity-popup-securityView,
>  #identity-popup[mixedcontent~=passive-loaded][isbroken] #identity-popup-security-content {
> -  background-image: url(chrome://browser/skin/controlcenter/connection.svg#connection-degraded);
> +  background-image: url(chrome://browser/skin/controlcenter/connection.svg);
> +  -moz-context-properties: fill, fill-opacity;
> +  fill: black;

I don't think you should set this.

These icons are set to fill: currentColor and that sounds appropriate to me. controlcenter/permissions.svg is not honoring that because we haven't converted it to use context-fill yet, but when we do they will have the same color again and we will be able to set the color for these icons from a single value, so I'd prefer us to file a follow-up bug to solve that.

::: browser/themes/shared/controlcenter/panel.inc.css:263
(Diff revision 2)
>  #identity-popup[loginforms=insecure] #identity-popup-securityView,
>  #identity-popup[loginforms=insecure] #identity-popup-security-content,
>  #identity-popup[mixedcontent~=active-loaded][isbroken] #identity-popup-securityView,
>  #identity-popup[mixedcontent~=active-loaded][isbroken] #identity-popup-security-content {
>    background-image: url(chrome://browser/skin/controlcenter/mcb-disabled.svg);
> +  -moz-context-properties: fill-opacity;

This should be -moz-context-properties: fill, fill-opacity;

See above comment.
Attachment #8904482 - Flags: review?(jhofmann) → review+

Comment 20

2 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d68bccc30f7e
Replace inconsistent padlock icons. r=johannh
(Assignee)

Comment 21

2 years ago
Attachment #8908072 - Flags: review+
(Assignee)

Comment 22

2 years ago
Sorry I made a mistake and landed this pre fixing the nits, if someone could land https://bugzilla.mozilla.org/attachment.cgi?id=8908072&action=edit on autoland would be great, thanks
Keywords: checkin-needed
Whiteboard: [reserve-photon-visual] → [reserve-photon-visual],

Comment 23

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/291700116d62
Follow up to fix review nits. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d68bccc30f7e
https://hg.mozilla.org/mozilla-central/rev/291700116d62
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Posted image green padlock.png
Reproduced the initial issue using Nightly 57.0a1 (2017-08-15).

Verified that the identity popup icons are the same as identity block icons on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12, on latest Nightly (2017-09-18) using the websites from comment 7:
 https://badssl.com/ = Green padlock
 https://https-everywhere.badssl.com/ = Grey padlock + yellow warning
 http://http-password.badssl.com/ = Grey padlock + red cross out

Please see the attached screenshots:
-green padlock
-grey+yellow
-grey padlock+red cross out

@Dale, is there any other verification that I could perform for this bug?
Flags: needinfo?(dharvey)
(Assignee)

Comment 28

2 years ago
Nope thats it, cheers
Flags: needinfo?(dharvey)
Based on comment 25 and comment 28 I will mark this Verified Fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.