Closed
Bug 1206244
Opened 9 years ago
Closed 9 years ago
Replace globe with the "i" icon, keep separate lock for secure pages
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
firefox45 | --- | verified |
People
(Reporter: MarcoM, Assigned: Paolo)
References
Details
(Keywords: addon-compat, Whiteboard: [fxprivacy])
User Story
This bug implements the new unified anchor icon, which is always visible and linked to the main panel with information about the current site. The main goal is to create a consistent visual point of reference and iconography that does not change when a site with different a connection type is visited, as the connection type is only one of the various possible privacy and security characteristics of the current page. Over time, the privacy and security settings relevant to the experience on the site will converge into the Control Center anchored to the new "i" icon. This is the live mockup for the icon and the rest of the Control Center: https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96586951
Attachments
(3 files)
No description provided.
Flags: qe-verify?
Reporter | ||
Updated•9 years ago
|
Priority: P3 → P1
Reporter | ||
Updated•9 years ago
|
Priority: P1 → P2
Reporter | ||
Updated•9 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 1•9 years ago
|
||
The live mockup for this bug is here: https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96586951 The live document with technical information about the location bar structure is here: https://www.lucidchart.com/documents/view/ca6d9936-81ef-4349-b68e-78023efe50a4 The latter should be updated when this bug lands.
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1206244 - Replace globe with the "i" icon, keep separate lock for secure pages. r=past
Attachment #8684238 -
Flags: review?(past)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 45.1 - Nov 16
Assignee | ||
Comment 3•9 years ago
|
||
The required changes ended up being fairly simple. I've cleaned up some unnecessary code and identifiers in the process. Note that I've not kept the "active" state from the original SVG asset because it didn't work well with the background color transition. The globe iconography is still present inside the panel for the "neutral" security indication, as well as being the default icon for pinned "about:" tabs. Various tests still have to be updated, but I think the review of the code itself can start. Everything should work as indicated in the mockup. Panos, do you think you could review this, or would you prefer redirecting to another reviewer?
Assignee | ||
Updated•9 years ago
|
Flags: qe-verify? → qe-verify+
Comment 4•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato https://reviewboard.mozilla.org/r/24507/#review22211 Everything here looks fine to me apart from the hover state colors, but I'm not very familiar with this part of the code so let's get an extra pair of eyes on it. You mention that the blue active state of the "i" icon didn't work well, care to elaborate? In the invision mockup I see every other color grayed out in the hover state so that the blue "i" stands out indicating the actual thing the user is interacting with. That includes the lock and EV cert label, but presumably also the strike-through lock and the lock with a warning triangle. That design reinforces the "i" as the Control Center anchor, while I feel that inverting the "i" only works well in the cases where all other identity box icons are also gray. Even in that case it may not be that clear when you have something like the location-blocked icon or an icon without many transparent regions, like the plugin-blocked icon. Is Ash or Bryan OK with us using a gray hover state for the "i"?
Attachment #8684238 -
Flags: review?(past)
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
Ash, please take a look at comment 4 and the screenshots I've attached and let us know what you think.
Flags: needinfo?(agrigas)
Assignee | ||
Comment 8•9 years ago
|
||
Ah, okay, so two different issues here. The SVG asset I found in the shared Dropbox folder had three states: normal, hover, and active. All were gray, it did not have a blue version. Here I'm not differentiating between hover and active because it looked strange to have a different intensity of gray for the icon before and after clicking the icon in the panel, because the background color remains the same. The other issue is that I missed that EV cases are grayed out in the mockup and the icon is blue. I guess we still want the icon to be blue? In that case, I just need a confirmation of the RGB code of the blue color and I can change it in the SVG for the hover state. For graying out the other icons while the "i" is in the hover state, from the UX team I need the correct gray color to use, but I'll probably have to rework the SVG files of all the connection security icons, either to add another gray version in them or to allow styling, whichever is preferred. Maybe we can file a follow-up bug for this part, still to be completed in this release?
Assignee | ||
Comment 9•9 years ago
|
||
Okay, so we filed the follow-up for graying out the other icons on hover. On this bug, I'm just waiting for Bryan to provide the right color code or SVG asset for the "i" icon on hover.
Flags: needinfo?(agrigas) → needinfo?(bbell)
Assignee | ||
Updated•9 years ago
|
Attachment #8684238 -
Flags: review?(past)
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24507/diff/1-2/
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato Brian, I know Panos wanted another pair of eyes on this patch. Can you make some time for an additional review between today and tomorrow?
Attachment #8684238 -
Flags: review?(bgrinstead)
Updated•9 years ago
|
Attachment #8684238 -
Flags: review?(past) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato https://reviewboard.mozilla.org/r/24507/#review22463
Assignee | ||
Comment 13•9 years ago
|
||
- <use xlink:href="#shape-circle-base" fill="#808080" /> + <use xlink:href="#shape-circle-base" fill="#4c9ed9" /> I've updated the SVG hover state with the color that Aislinn provided.
Flags: needinfo?(bbell)
Assignee | ||
Comment 14•9 years ago
|
||
The try run is taking a long time as usual, but browser-chrome tests seem OK on Mac and Linux and are still running on Windows. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda9b851e296
Updated•9 years ago
|
Attachment #8684238 -
Flags: review?(bgrinstead)
Comment 15•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato https://reviewboard.mozilla.org/r/24507/#review22477 This looks good, but I have a question about the element caching before r+ ::: browser/base/content/browser.js (Diff revision 2) > -var gProxyFavIcon = null; Might want to mark this as addon-compat.. there are a few hits for gProxyFavIcon in addons mxr ::: browser/base/content/browser.js (Diff revision 2) > - _cacheElements : function() { Why isn't this needed anymore? I got the impression it was called from customize mode because some elements could be added/removed in that process. Have you tested this with customize mode after adding and removing the location bar?
Comment 16•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #15) > ::: browser/base/content/browser.js > (Diff revision 2) > > - _cacheElements : function() { > > Why isn't this needed anymore? I got the impression it was called from > customize mode because some elements could be added/removed in that process. > Have you tested this with customize mode after adding and removing the > location bar? If you've considered this situation and tested it then feel free to treat the review as an r+. But if the code comments are right and I'm following the logic right, it looks like this will be an issue.
Comment 17•9 years ago
|
||
(In reply to :Paolo Amadini [away soon] from comment #8) > Ah, okay, so two different issues here. > > The SVG asset I found in the shared Dropbox folder had three states: normal, > hover, and active. All were gray, it did not have a blue version. Here I'm > not differentiating between hover and active because it looked strange to > have a different intensity of gray for the icon before and after clicking > the icon in the panel, because the background color remains the same. > > The other issue is that I missed that EV cases are grayed out in the mockup > and the icon is blue. I guess we still want the icon to be blue? In that > case, I just need a confirmation of the RGB code of the blue color and I can > change it in the SVG for the hover state. > > For graying out the other icons while the "i" is in the hover state, from > the UX team I need the correct gray color to use, but I'll probably have to > rework the SVG files of all the connection security icons, either to add > another gray version in them or to allow styling, whichever is preferred. > Maybe we can file a follow-up bug for this part, still to be completed in > this release? Here is a full set of assets https://www.dropbox.com/sh/pjzjir04m22kw9z/AAA_9jKFgPtNdJ8kcZjxQJIDa?dl=0 and a tech-demo of how the masks and coloring needs to work http://people.mozilla.org/~shorlander/icons-svg/SVG-Icon-Template-Test/svg-icon-template-test.html
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #15) > Might want to mark this as addon-compat.. there are a few hits for > gProxyFavIcon in addons mxr Good idea, in fact in this case we have a simple pattern for the validator (gProxyFavIcon). I did the MXR search as well and saw the few references, not sure how many of those add-ons are still maintained though. > > - _cacheElements : function() { > > Why isn't this needed anymore? I got the impression it was called from > customize mode because some elements could be added/removed in that process. > Have you tested this with customize mode after adding and removing the > location bar? One major feature of the Australis redesign was to make the address bar not removable. A lot of code that does not assume that the identity block is always visible is still around, though, and we can take the opportunity to remove it while we work on related areas. I had tested both normal and popup windows. I had also checked the file history, and the code was originally written before Australis, even though a few lines were updated for consistency later.
Keywords: addon-compat
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #16) > If you've considered this situation and tested it then feel free to treat > the review as an r+. Thanks, I'll land this now so I still have tomorrow available in case of unexpected results.
Assignee | ||
Updated•9 years ago
|
User Story: (updated)
Assignee | ||
Updated•9 years ago
|
Attachment #8684238 -
Attachment description: MozReview Request: Bug 1206244 - Replace globe with the "i" icon, keep separate lock for secure pages. r=past → MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato
Attachment #8684238 -
Flags: review?(ato)
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24507/diff/2-3/
Updated•9 years ago
|
Attachment #8684238 -
Flags: review?(ato) → review+
Comment 22•9 years ago
|
||
Comment on attachment 8684238 [details] MozReview Request: Bug 1206244 - Fix Marionette test that used the identity icon element to test a framework function. r=ato https://reviewboard.mozilla.org/r/24507/#review22555
Comment 24•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73119a9a7da2 https://hg.mozilla.org/mozilla-central/rev/5a6d166dea08
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Updated•9 years ago
|
QA Contact: paul.silaghi
Comment 25•8 years ago
|
||
Verified fixed FF 45.0a1 (2015-11-16) Win 7, Ubuntu 14.04, OS X 10.11
Status: RESOLVED → VERIFIED
Comment 26•8 years ago
|
||
Any chance someone can share the "i" graphic for use in our Firefox blog post? Ideally in both inactive and hover states?
Comment 27•8 years ago
|
||
You could just visit the following link in your Nightly: chrome://browser/skin/identity-icon.svg
Comment 28•8 years ago
|
||
(In reply to Panos Astithas [:past] from comment #27) > You could just visit the following link in your Nightly: > > chrome://browser/skin/identity-icon.svg Thank you!
Comment 29•5 years ago
|
||
May someone advice how to hide this icon?
You need to log in
before you can comment on or make changes to this bug.
Description
•