750106 - Use a chrome icon for chrome URLs in the location bar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Marco Castelluccio [:marco]]

For in-content pages (like about:home) it would be better to remove the generic icon, as they are obviously secure.

Comment 1

[Siddhartha Dugar [:sdrocking]]

Bug 685059 may fix this.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I don't think we can simply remove the generic (globe) icon for in-content pages. Doing so would remove the ability to drag the icon to create bookmarks or open in a new tab. However, we could use a different icon for chrome URLs.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Jared Wein [:jaws] from comment #2)
> I don't think we can simply remove the generic (globe) icon for in-content
> pages. Doing so would remove the ability to drag the icon to create
> bookmarks or open in a new tab. However, we could use a different icon for
> chrome URLs.

I agree, but this disagrees with https://bugzilla.mozilla.org/show_bug.cgi?id=685059#c24 (that on the other side was posted before all of the identity box redesign), may you figure out with Stephen what's the final plan?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

That bug is for the tab bar. We talked about removing the site identity block for chrome URLs at the work week but decided against it for the reasons mentioned in comment #2.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Jared Wein [:jaws] from comment #4)
> That bug is for the tab bar.

whoops, my fault. thanks.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Maybe we could use a Home icon for this?

Comment 7

[Florian Bender]

Or a gear icon, this matches most "about:*"s. Esp. interesting when in-content prefs land. 

However, some pages could use even another icon, e. g. the suggested home icon (only) for about:newtab and about:home.

Comment 8

[Daniel Veditz [:dveditz]]

Could also use a yellow /!\ icon for network error pages, and bring back the red/white "do not enter" icon for the malware/phishing blocking page.

Comment 9

[u428464]

You can see what it should look like on this mockup : icon + Firefox (in orange) | about:*

http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/windows7-customizationMode-i02.html

Comment 10

[Florian Bender]

Great work! :)

How will this affect special pages like about:neterror and the phishing warning (I see these pages have been removed, at least from about:about)? They should (IMO) get special icons (not Favicons!), e. g. the yellow warning triangle resp. stop sign – showing the same as mocked up in Comment 9 may confuse users.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Stephen, I see http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/images-win7/identityBlock-firefox.png. Do you have a HiDPI version?

This shouldn't be too hard to implement once we get that other icon and maybe hover/active states if necessary.

FWIW, I don't think hover/active states here are necessary, since I don't know what we would show if the user clicked on it, unless we wanted the states for drag/drop purposes.

Comment 12

[u428464]

A "secure" panel may be useful (like Chrome).

Comment 13

[u428464]

(In reply to Florian Bender from comment #10)
> Great work! :)
> 
> How will this affect special pages like about:neterror and the phishing
> warning (I see these pages have been removed, at least from about:about)?
> They should (IMO) get special icons (not Favicons!), e. g. the yellow
> warning triangle resp. stop sign – showing the same as mocked up in Comment
> 9 may confuse users.

Of course this will only be in use for in-content pages like customize, add-ons or preferences and library in the future. Phishing and neterror will keep their special favicon.

Comment 14

[Florian Bender]

(In reply to Guillaume C. [:ge3k0s] from comment #13)
> Phishing and neterror will keep their special favicon.

Favicons can be spoofed (for this reason separate HTTPS icons were introduced) and may be used to trick users into clicking buttons to initiate an attack (or input credentials) while they believe they are on a Firefox error page. Thus, those pages should get their own icons (+ text) though not the "Firefox" one. 

Shall I file a follow up bug or do you consider implementing those in this Bug, too?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Florian Bender from comment #14)
> (In reply to Guillaume C. [:ge3k0s] from comment #13)
> > Phishing and neterror will keep their special favicon.
> 
> Favicons can be spoofed (for this reason separate HTTPS icons were
> introduced) and may be used to trick users into clicking buttons to initiate
> an attack (or input credentials) while they believe they are on a Firefox
> error page. Thus, those pages should get their own icons (+ text) though not
> the "Firefox" one. 

The icon in the address bar cannot be changed by a webpage.

Comment 16

[Florian Bender]

But Favicons can, which Guillaume was talking about. Plus, in the current Beta at least, those icons are only used as Favicons, not "identity" icons in the Adress Bar.

Comment 17

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jaws] from comment #11)
> Stephen, I see
> http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/images-
> win7/identityBlock-firefox.png. Do you have a HiDPI version?
> 
> This shouldn't be too hard to implement once we get that other icon and
> maybe hover/active states if necessary.
> 
> FWIW, I don't think hover/active states here are necessary, since I don't
> know what we would show if the user clicked on it, unless we wanted the
> states for drag/drop purposes.

We could show a larry like panel: http://people.mozilla.com/~shorlander/bugs/bug750106-firefoxPanel.png

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

I like it! http://media.tumblr.com/tumblr_leeq46TBts1qdlkgg.gif

Comment 19

[Stephen Horlander [:shorlander]]

Attachment: Chrome URL Identity Icons - 01

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (part 1)

This patch makes use of the new icon and gets it looking all spiffy, but doesn't include the identity panel doorhanger.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Also, no reason we can't land this on m-i instead of ux. Stephen, should this wait for Australis or go out in 24?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (part 2)

This patch implements the identity panel bits. I copied mozicon128.png to the content so that we would have an equivalent HiDPI graphic for each of the branding outputs.

Comment 23

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jaws] from comment #21)
> Also, no reason we can't land this on m-i instead of ux. Stephen, should
> this wait for Australis or go out in 24?

Shipping it sooner would be awesome!

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 762349 [details] [diff] [review]
Patch (part 1)

Review of attachment 762349 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/browser.css
@@ +841,5 @@
>    -moz-image-region: rect(0, 16px, 16px, 0);
>  }
>  
> +.chromeUI > #page-proxy-favicon[pageproxystate="valid"] {
> +  list-style-image: url(chrome://browser/skin/identity-icons-firefox.png);

It seems to me like this icon should be in the branding directory as it's pretty close to the trademarked logo. Then we would need another version for unofficial branding or use the generic version. I wouldn't hurt to get superreview on branding-related changes so I'm flagging Gavin.

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 762394 [details] [diff] [review]
Patch (part 2)

Review of attachment 762394 [details] [diff] [review]:
-----------------------------------------------------------------

> diff --git a/browser/branding/aurora/mozicon128.png b/browser/branding/aurora/content/icon128.png
> diff --git a/browser/branding/nightly/mozicon128.png b/browser/branding/nightly/content/icon128.png
> diff --git a/browser/branding/official/mozicon128.png b/browser/branding/official/content/icon128.png
> diff --git a/browser/branding/unofficial/mozicon128.png b/browser/branding/unofficial/content/icon128.png

Copying the images in HG feels dirty to me as I dislike forks. Moving them to /content/ or copying them at build time seems like less of a maintenance issue.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 762349 [details] [diff] [review]
Patch (part 1)

>+#urlbar[pageproxystate="valid"] > .chromeUI,
> #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity {

#identity-box.chromeUI

>+#urlbar[pageproxystate="valid"] > .chromeUI {

ditto

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 762394 [details] [diff] [review]
Patch (part 2)

> /* Identity UI */
>+#identity-popup-content-box:not(.chromeUI) > #identity-popup-brandName,
>+#identity-popup-content-box:not(.chromeUI) > #identity-popup-chromeLabel,
> #identity-popup-content-box.unknownIdentity > #identity-popup-connectedToLabel ,
> #identity-popup-content-box.unknownIdentity > #identity-popup-runByLabel ,
> #identity-popup-content-box.unknownIdentity > #identity-popup-content-host ,
> #identity-popup-content-box.unknownIdentity > #identity-popup-content-owner ,
> #identity-popup-content-box.verifiedIdentity > #identity-popup-connectedToLabel2 ,
> #identity-popup-content-box.verifiedDomain > #identity-popup-connectedToLabel2 {
>   display: none;
> }
> 
>+#identity-popup-content-box.chromeUI > .identity-popup-label:not(#identity-popup-brandName):not(#identity-popup-chromeLabel),
>+#identity-popup-content-box.chromeUI > .identity-popup-description,
>+#identity-popup-content-box.chromeUI > #identity-popup-button-container {
>+  display: none;
>+}

Why is this stuff split across two rules?

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] from comment #24)
> It seems to me like this icon should be in the branding directory as it's
> pretty close to the trademarked logo. Then we would need another version for
> unofficial branding or use the generic version.

That's a good idea. Stephen, can create a HiDPI version of the graphics with all three states for consistency with Linux/Windows?

Also would be cool if we had nightly/aurora/(and shriek, unofficial!) versions too. But for unofficial I don't see a problem with continuing to use the unknownIdentity globe that we currently use.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 762349 [details] [diff] [review]
Patch (part 1)

Review of attachment 762349 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane, but withholding an r+ until we hear more about branding rules.

I also think there are sections that could be put into browser/themes/shared, since they seem common.

::: browser/themes/osx/browser.css
@@ +1251,5 @@
>    padding-right: 10.01px;
>  }
>  
> +#urlbar[pageproxystate="valid"] > .chromeUI,
> +#urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity {

Seems like this rule could go into ../shared. Same with the one in windows/browser.css. linux/browser.css has something similar too - but with an added background-color that we can just override with.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 762394 [details] [diff] [review]
Patch (part 2)

Review of attachment 762394 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me - just a few suggestions (mostly with putting things into browser/themes/shared).

::: browser/base/content/browser.css
@@ +394,5 @@
>  }
>  
>  /* Identity UI */
> +#identity-popup-content-box:not(.chromeUI) > #identity-popup-brandName,
> +#identity-popup-content-box:not(.chromeUI) > #identity-popup-chromeLabel,

As Dao suggested, we might as well merge these two rule blocks.

::: browser/base/content/browser.js
@@ +6544,5 @@
> +      let brandBundle = document.getElementById("bundle_brand");
> +      let brandShortName = brandBundle.getString("brandShortName");
> +      this._identityPopupChromeLabel.textContent = gNavigatorBundle.getFormattedString("identity.chrome",
> +                                                                                       [brandShortName]);
> +    }

Has something gone wrong with the indentation here? It's strange to see two close curly parentheses directly on top of one another...

::: browser/themes/linux/browser.css
@@ +945,5 @@
>    -moz-image-region: rect(128px, 64px, 192px, 0px);
>  }
>  
> +#identity-popup.chromeUI > #identity-popup-container > #identity-popup-icon {
> +  list-style-image: url("chrome://branding/content/icon64.png");

This can probably go into a shared include as well.

::: browser/themes/windows/browser.css
@@ +1830,5 @@
>    -moz-padding-start: 15px;
>    margin: 0;
>  }
>  
> +#identity-popup-brandName {

Here's another thing that could be put into a common file under browser/themes/shared.

Comment 31

[Stephen Horlander [:shorlander]]

Attachment: Chrome URL Identity Icon @2x

(In reply to Jared Wein [:jaws] from comment #28)
> (In reply to Matthew N. [:MattN] from comment #24)
> > It seems to me like this icon should be in the branding directory as it's
> > pretty close to the trademarked logo. Then we would need another version for
> > unofficial branding or use the generic version.
> 
> That's a good idea. Stephen, can create a HiDPI version of the graphics with
> all three states for consistency with Linux/Windows?

Attached.

> Also would be cool if we had nightly/aurora/(and shriek, unofficial!)
> versions too. But for unofficial I don't see a problem with continuing to
> use the unknownIdentity globe that we currently use.

I would prefer to do that in a follow-up if possible.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This patch updates the icons to be used, moved them to branding, split out the CSS to a shared file, and includes a kitten.

I agree with Stephen, that we can update the icons for unofficial, nightly, and aurora in a follow-up.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.1

Forgot to hg add the identity-icons-brand.png files.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 765198 [details] [diff] [review]
Patch v2.1

Review of attachment 765198 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks fine. Thanks Jared!

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 765198 [details] [diff] [review]
Patch v2.1

I filed bug 888563 to track updating the images for unofficial, Nightly, and Aurora. I changed the unofficial branding to use the globe instead of the Firefox icon so it can remain generic. This should drop the need for s-r since it won't introduce any branding issues.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5133764596

Comment 36

[u428464]

Shouldn't this be also displayed on new tab or will it be too distracting for the user ?

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3c5133764596

Comment 38

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] from comment #35)
> I changed the unofficial branding to use the globe instead of the
> Firefox icon so it can remain generic. This should drop the need for s-r
> since it won't introduce any branding issues.

FWIW, the unofficial branding package is currently unused, so this did introduce branding issues for Nightly/Aurora. We'll probably fix that as part of bug 840883, though.

Comment 39

[Justin Dolske [:Dolske]]

This is already on m-c, so not tracking for Australis.