Closed Bug 1206244 Opened 6 years ago Closed 6 years ago

Replace globe with the "i" icon, keep separate lock for secure pages

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 45
Iteration:
45.1 - Nov 16
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?
Priority: P3 → P1
Priority: P1 → P2
Priority: P2 → P1
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.
Bug 1206244 - Replace globe with the "i" icon, keep separate lock for secure pages. r=past
Attachment #8684238 - Flags: review?(past)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 45.1 - Nov 16
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?
Flags: qe-verify? → qe-verify+
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)
Ash, please take a look at comment 4 and the screenshots I've attached and let us know what you think.
Flags: needinfo?(agrigas)
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?
Blocks: 1223049
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)
Attachment #8684238 - Flags: review?(past)
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/
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)
Attachment #8684238 - Flags: review?(past) → review+
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
-    <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)
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
Attachment #8684238 - Flags: review?(bgrinstead)
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?
(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.
(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
(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
(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.
User Story: (updated)
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)
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/
Attachment #8684238 - Flags: review?(ato) → review+
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
https://hg.mozilla.org/mozilla-central/rev/73119a9a7da2
https://hg.mozilla.org/mozilla-central/rev/5a6d166dea08
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
QA Contact: paul.silaghi
Verified fixed FF 45.0a1 (2015-11-16) Win 7, Ubuntu 14.04, OS X 10.11
Status: RESOLVED → VERIFIED
Depends on: 1231025
Any chance someone can share the "i" graphic for use in our Firefox blog post? Ideally in both inactive and hover states?
You could just visit the following link in your Nightly:

chrome://browser/skin/identity-icon.svg
(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!
Depends on: 1305676

May someone advice how to hide this icon?

You need to log in before you can comment on or make changes to this bug.