Closed
Bug 1390511
Opened 7 years ago
Closed 7 years ago
Padlock icon inconsistent
Categories
(Firefox :: Theme, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: alberts, Assigned: daleharvey)
References
Details
(Keywords: nightly-community, ux-consistency, Whiteboard: [reserve-photon-visual],)
Attachments
(14 files)
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 |
After bug 1387614 has landed, the padlock icon is not consistent anymore. I added some examples in the image attached.
Comment 1•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dharvey
Updated•7 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: -- → P1
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Comment 6•7 years ago
|
||
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•7 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)
Comment 8•7 years ago
|
||
(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
Updated•7 years ago
|
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Assignee | ||
Comment 9•7 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•7 years ago
|
||
Figured replacing the related icons doesnt need to block ensuring these icons are consistent
Updated•7 years ago
|
Attachment #8904482 -
Flags: review?(dao+bmo) → review?(jhofmann)
Comment 12•7 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)
Comment 13•7 years ago
|
||
Flags: needinfo?(shorlander)
Comment 14•7 years ago
|
||
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
Comment 17•7 years ago
|
||
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•7 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•7 years ago
|
||
Pushed by dharvey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d68bccc30f7e Replace inconsistent padlock icons. r=johannh
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8908072 -
Flags: review+
Assignee | ||
Comment 22•7 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•7 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
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d68bccc30f7e https://hg.mozilla.org/mozilla-central/rev/291700116d62
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 25•7 years ago
|
||
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)
Comment 26•7 years ago
|
||
Comment 27•7 years ago
|
||
Comment 29•7 years ago
|
||
Based on comment 25 and comment 28 I will mark this Verified Fixed.
You need to log in
before you can comment on or make changes to this bug.
Description
•