Closed
Bug 750106
Opened 13 years ago
Closed 12 years ago
Use a chrome icon for chrome URLs in the location bar
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 25
People
(Reporter: marco, Assigned: jaws)
References
Details
Attachments
(3 files, 3 obsolete files)
For in-content pages (like about:home) it would be better to remove the generic icon, as they are obviously secure.
Comment 1•13 years ago
|
||
Bug 685059 may fix this.
Assignee | ||
Comment 2•13 years ago
|
||
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.
Summary: Remove the generic icon for in-content pages → Use a chrome icon for chrome URLs in the location bar
Comment 3•13 years ago
|
||
(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?
Assignee | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #4)
> That bug is for the tab bar.
whoops, my fault. thanks.
Assignee | ||
Comment 6•13 years ago
|
||
Maybe we could use a Home icon for this?
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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.
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•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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.
Flags: needinfo?(shorlander)
Comment 12•12 years ago
|
||
A "secure" panel may be useful (like Chrome).
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
(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?
Assignee | ||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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
Flags: needinfo?(shorlander)
Assignee | ||
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Blocks: australis-cust
Whiteboard: [Australis:M?]
Assignee | ||
Comment 20•12 years ago
|
||
This patch makes use of the new icon and gets it looking all spiffy, but doesn't include the identity panel doorhanger.
Assignee | ||
Comment 21•12 years ago
|
||
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?
Flags: needinfo?(shorlander)
Assignee | ||
Comment 22•12 years ago
|
||
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.
Attachment #762394 -
Flags: review?(mconley)
Comment 23•12 years ago
|
||
(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!
Flags: needinfo?(shorlander)
Comment 24•12 years ago
|
||
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.
Attachment #762349 -
Flags: superreview?(gavin.sharp)
Attachment #762349 -
Flags: feedback-
Comment 25•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 28•12 years ago
|
||
(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.
Flags: needinfo?(shorlander)
Comment 29•12 years ago
|
||
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.
Attachment #762349 -
Flags: review?(mconley)
Comment 30•12 years ago
|
||
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.
Attachment #762394 -
Flags: review?(mconley)
Comment 31•12 years ago
|
||
(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.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 32•12 years ago
|
||
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.
Attachment #762349 -
Attachment is obsolete: true
Attachment #762394 -
Attachment is obsolete: true
Attachment #762349 -
Flags: superreview?(gavin.sharp)
Attachment #765195 -
Flags: superreview?(gavin.sharp)
Attachment #765195 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 33•12 years ago
|
||
Forgot to hg add the identity-icons-brand.png files.
Attachment #765195 -
Attachment is obsolete: true
Attachment #765195 -
Flags: superreview?(gavin.sharp)
Attachment #765195 -
Flags: review?(mnoorenberghe+bmo)
Attachment #765198 -
Flags: superreview?(gavin.sharp)
Attachment #765198 -
Flags: review?(mnoorenberghe+bmo)
Updated•12 years ago
|
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
Assignee | ||
Updated•12 years ago
|
Attachment #765198 -
Flags: review?(mnoorenberghe+bmo) → review?(mconley)
Comment 34•12 years ago
|
||
Comment on attachment 765198 [details] [diff] [review]
Patch v2.1
Review of attachment 765198 [details] [diff] [review]:
-----------------------------------------------------------------
Yeah, this looks fine. Thanks Jared!
Attachment #765198 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 35•12 years ago
|
||
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
Attachment #765198 -
Flags: superreview?(gavin.sharp)
Comment 36•12 years ago
|
||
Shouldn't this be also displayed on new tab or will it be too distracting for the user ?
Comment 37•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Comment 38•12 years ago
|
||
(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.
Updated•12 years ago
|
Whiteboard: [Australis:M?][Australis:P5]
Updated•12 years ago
|
Comment 39•12 years ago
|
||
This is already on m-c, so not tracking for Australis.
No longer blocks: australis-cust
You need to log in
before you can comment on or make changes to this bug.
Description
•