Closed Bug 588270 Opened 14 years ago Closed 12 years ago

Remove the favicon from the location bar

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: faaborg, Assigned: jaws)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [secr:curtisk])

Attachments

(6 files, 19 obsolete files)

651.04 KB, image/png
Details
710.68 KB, video/ogg
Details
33.09 KB, patch
Details | Diff | Splinter Review
4.55 KB, patch
Details | Diff | Splinter Review
15.32 KB, patch
Details | Diff | Splinter Review
2.86 KB, patch
Details | Diff | Splinter Review
Attached image Non redundant identity block (obsolete) —
[note, this bug is not planned for Firefox 4.  However, some other changes that we are tracking are dependent on this change so I'm getting this on filed now].

This bug encapsulates the following changes:

-display a grey site identity block in the normal case
-add subdomains to the site identity block
-place the path after the site identity block
-modify the site identity block to indicate ssl
-make sure that when the entire field reverts to editable text on focus, the path does not shift location (we'll probably achieve this by making sure that the text remains the same, and just changing the subdomain and domain section to being text color on window colo, instead of window color on window color.
Blocks: 588272
Blocks: 588274
Note that for the new notification design work we need to have an anchor for site level doorhangers.  So this means that even normal (non-ssl) connections are going to need to have a redundant site identity block.  That work is occurring over in bug 587901.

(We also need to have a site identity block for account manager on non-ssl sites, but that is potentially less of an issue if account manager lands later).
Isn't this bug the same as the bug 578959?
This bug is that bug +3 additional changes.  While it's great for people to file bugs as they test Firefox (especially on problems we haven't found), please don't be surprised if we dupe any bugs related to design work.  We don't always know the bugs exist, or even if we do know that, we sometimes want to add additional information or propose a slightly different approach.  Morphing existing bugs is usually considered a greater offense than duplication.  Also, even if an exact replica exists, searching bugzilla for perfect existing bugs often takes too much time given the quantity of bugs that we need to quickly file.
Oh no, I'm not complaining, I'm just asking if I should dupe my bug myself. I've learned how it works here.
So is the first image in the attachment how the regular locationbar's supposed to look? The site identity box's always gonna be shown?
Yes.
I think this change could cause a problem for color-blind users. On the browser's current iteration, we have the site name in the block, https:// in the url bar, and even the lock icon in the status bar as a visual indication for those users. On the proposed mockup, along with the status bar removal, the normal and https may be hard to distinguish - though I do see the domain text is inverted, which may be enough of an indicator.
>I think this change could cause a problem for color-blind users

There are three types of color blindness:

Protanopia/Protanomaly  (red deficient: L-cone absent/deficient)
Deuteranopia/Deuteranomaly (green deficient: M-cone absent/deficient)
Tritanopia/Tritanomaly (blue deficient: S-cone absent/deficient)

since we are using a progress of grey to blue to green, that will still appear as three different colors for everyone, even if someone has one of the three types of color blindness.

it's technically possible for someone to have only monochromatic vision (in which case they would be looking at the level of contrast from grey to blue), but this is extremely rare.
The concern I have here is the amount of sites on the web that tell you to look for "https://" at the start of the URL, and the amount of users who have been trained to do that. I know that this is partly flawed in that encryption without identity has little value, but this is a big step away from every web browser that has ever existed as far as I know.

The "removing redundancy" part I like though.

Another question: what will it look like on FTP sites? Or any schemes other than http and https? view-source, jar, wyciwyg, chrome, resource, about etc.
Yellow for FTP sites? I like the idea :)
Also, and sorry for the double post, we also need to account for file:// and chrome:// at least. I don't know of other protocols, but I'm sure there are plenty more. We could, theoretically, just don't display the identity button at all in such cases, which definitely makes sense for file:// and chrome://, but maybe not for ftp://
(In reply to comment #11)
> Yellow for FTP sites? I like the idea :)

Can't. Yellow's already take by bug 588274.
>Another question: what will it look like on FTP sites? Or any schemes other
>than http and https? view-source, jar, wyciwyg, chrome, resource, about etc.

For protocols that have a notion of domain, we could either place it in the block before the domain, or after

[ mozilla.org (ftp) ] / path


[ ftp://mozilla.org ] / path

For protocols that do not have a notion of a domain name (file, chrome, jar) we would probably want to just drop the identity block entirely and have a normal plain text URL:

file:///C:/Users/Faaborg/Desktop/
This is way out of scope of pretty much everything (this bug, Firefox 4, perhaps even the next release), but ultimately we would like to introduce action words or commands in the location bar, kind of a simplistic version of ubiquity.  So for instance:

[calendar] event to add
[map] location to map
[wikipedia] search

Visually, these user configurable commands were going to appear as colorful blocks (with the color based on an analysis of the favicon or perhaps the site itself).  So the current site identity blocks could kind of be considered the first instance of these command blocks, you pass the domain the path, and in response it serves up the page you want.

This is of course as I mentioned all beyond the scope of this particular change, but wanted to add a note about our long term thinking about identity/command blocks in the location bar.
(In reply to comment #14)
> For protocols that do not have a notion of a domain name (file, chrome, jar)

chrome and resource protocols do have some notion of a domain, though yeah, I see no real need to handle them fancily and over-complicate things.
The download manager needs to special case for domainless protocols already. Normally it'll try to show eTLD+1, but it does something special for file:// URIs (it shows "local file"), and for other protocols (e.g., data: about: moz-icon: etc.) it shows "data: resource"
The use of gray in some elements in the mockups makes me think they are disabled, but it seems they are supposed to convey the opposite meaning.

I think that a red blinking icon would be appropriate when a recording device (camera and/or mic) is being used. It should be very obvious when you are being recorded.

If the URL is https://hackerville.com/https://www.paypal.com, the site identity block will have "hackerville.com" in blue (indicating "secure"), and the address bar will contain "/https://www.paypal.com". Combine this with the fact that the white-on-blue text in the site identity block is harder to read compared to the grey text in the address bar and a "banner blindness" effect that makes the user's eyes skip over the identity block to read what is in the white box ("/https://www.paypal.com"). It seems like a step backwards in reducing phishing.

On Linux, /path/blah/urlsarehardtoread looks a lot like a file path, especially when you consider how other applications display file paths in their address bars.

More generally, it is quite confusing for URLs to look dramatically different in the address bar than they look everywhere else (other browsers, email, IM, word documents, web pages, TV, magazines, the sides of buses, etc.).
(In reply to comment #14)
> >Another question: what will it look like on FTP sites? Or any schemes other
> >than http and https? view-source, jar, wyciwyg, chrome, resource, about etc.
> 
> For protocols that have a notion of domain, we could either place it in the
> block before the domain, or after
> 
> [ mozilla.org (ftp) ] / path
-1
> 
> 
> [ ftp://mozilla.org ] / path
+1
> 
> For protocols that do not have a notion of a domain name (file, chrome, jar) we
> would probably want to just drop the identity block entirely and have a normal
> plain text URL:
> 
> file:///C:/Users/Faaborg/Desktop/
(In reply to comment #18)
> The use of gray in some elements in the mockups makes me think they are
> disabled, but it seems they are supposed to convey the opposite meaning.
> 
> I think that a red blinking icon would be appropriate when a recording device
> (camera and/or mic) is being used. It should be very obvious when you are being
> recorded.

+1 except for the blinking part, solid red over gray should do for active account, geo, camera.
Similar to the red REC LEDs on video cameras.
See http://lenovoblogs.com/lenovofiles/2010/07/22/watch-that-webcam/ they have it backwards IMHO with red=off when it should be red
=recording.

> If the URL is https://hackerville.com/https://www.paypal.com, the site identity
> block will have "hackerville.com" in blue (indicating "secure"), and the
> address bar will contain "/https://www.paypal.com". Combine this with the fact
> that the white-on-blue text in the site identity block is harder to read
> compared to the grey text in the address bar and a "banner blindness" effect
> that makes the user's eyes skip over the identity block to read what is in the
> white box ("/https://www.paypal.com"). It seems like a step backwards in
> reducing phishing.

What about turning "site identity block" red for any URL with "http(s)://"
 in the middle of it?
Anyone doing that is certainly not up for anything good.
Is this going to be optional?
Drew: limi mentioned that you might be interested in working on this.  The mockup here should describe everything:

https://bugzilla.mozilla.org/attachment.cgi?id=466899

Let me know what questions you have.
I really like this proposal.  My suggestions:

- The protocol and port (when non-default) should always be shown to form a complete URL, like the URLs users see other places (comment #18).  The protocol should probably go inside the block to avoid splitting it in two in the EV case.  I would also suggest putting the port inside the block.

- Minimize the width of the right edge of the block so that the URL reads naturally and to reduce the likelihood that the user is fooled by [https://hackerville.com]/https://www.paypal.com .  The down arrow can move to the left.

- For a more robust appearance, the block should occupy the full height of the URL bar (as it does now) rather than float inside the bar.

- What is the "@site.com" in the account manager item?  Is it part of the site-defined username string?  Keep the person icon visible and/or place some constraints on the username string to make it harder for a site to fool users by automatically logging them in as www.paypal.com .
>- The protocol and port (when non-default) should always be shown to form a
>complete URL, like the URLs users see other places

I agree if the protocol is non-default (https, ftp, etc.)

>- For a more robust appearance, the block should occupy the full height of the
>URL bar (as it does now) rather than float inside the bar.

there's a chome/content distinction that we want to draw here, but having the block float inside of the text field it visually describes that it is a transient thing that is associated with the content area (floating in a white block, instead of being bolted to the chrome).  Also, we want this to convey that it is editable, that the user can clear it by selecting the text and hitting the backspace key.

>- What is the "@site.com" in the account manager item?  Is it part of the
>site-defined username string?  Keep the person icon visible and/or place some
>constraints on the username string to make it harder for a site to fool users
>by automatically logging them in as www.paypal.com .

This string has to match an account actually stored by account manager, so there isn't any risk of sites fooling users with spoofed text.  In this case, the user specified an email address as their account name.
Is there any plan to display broken encryption somehow?  In 2.0 there was the broken lock icon in the urlbar, in 3.0 that moved down to the statusbar and currently it doesn't exist.  The only way to know if site encryption has dropped (or never worked) is that the identity block is grey instead of blue/green.  There is the security alert that appears the first time you view such a page, but it's easily disabled and never seen again.  Also, lately I've seen some websites (Facebook SSL, Lycos Mail) dropping encryption randomly while the page is just sitting... possibly while doing javascript-based refreshes.  So you may start out on a page which claims to be encrypted and then 10 minutes later that same page may drop out and you get no real notification of that.
Alex, thanks for your response.

(In reply to comment #24)
> >- The protocol and port (when non-default) should always be shown to form a
> >complete URL, like the URLs users see other places
> 
> I agree if the protocol is non-default (https, ftp, etc.)

True, the http:// is omitted in many places so the argument for showing it does not hold.  As a purist, I would prefer to see it.

Re this issue and the next, I will trust that you have the best interests of the user base as a whole in mind and will make an add-on if they bother me enough.
I think that is a great feature for Firefox
design review needed
Whiteboard: [secr:curtisk]
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attached patch WIP of patch for bug 588270 (obsolete) — Splinter Review
This is the current WIP of my patch for this bug. I have removed the site identity block when viewing a page over http:, chrome:, about:, and https: where there is mixed content.

The bookmark star is now draggable to create a bookmark on the bookmarks toolbar, set the browser homepage (via dragging to the home button), and making a desktop shortcut.

Known issues with the current patch:
1) There is some extra margin-start on the urlbar text when the identity block is visible. I need to tweak the CSS selector so this margin-start doesn't apply when the identity-block is visible.
2) The first and sometimes second time the bookmark star is dragged, the favicon isn't visible in the drag image. Dragging a third and subsequent time seems to work properly. I'm not sure if this is a bug in nsIDOMCanvasRenderingContext2D::drawImage or the nsIFaviconService::GetFaviconDataAsDataURL. The data URL that is returned from the favicon service is the exact same each time the drag start handler is evaluated, but the drawing is different sometimes. Could it be due to expiring favicons/favicons that aren't cached yet? I'm stuck here. The other way to go about this would be to allow nsIDOMCanvasRenderingContext2D::drawImage to take a xul:image as an argument (it currently only accepts an html:img).
(In reply to Jared Wein [:jwein and :jaws] from comment #29)
> Created attachment 582147 [details] [diff] [review]
> WIP of patch for bug 588270

Pushed to UX: https://hg.mozilla.org/projects/ux/rev/8c6f56e1a30a
Attachment #582147 - Flags: feedback?(fryn)
Comment on attachment 582147 [details] [diff] [review]
WIP of patch for bug 588270

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

Thanks for writing a patch! :)

::: browser/base/content/browser.js
@@ +8179,5 @@
> +    ctx.font = "12px Segoe UI, Helvetica, sans-serif";
> +    ctx.fillText(text, textOffset, 16);
> +    ctx.save();
> +
> +    dt.setDragImage(canvas, 16, 16);

Do we really want to use a canvas for this stuff? If we want to include more than the favicon as the drag image, I'd prefer using a <xul:panel/> with transient contents, which are easily style-able without hard-coding anything.

Also, since the tab is already displaying the favicon, we can probably just grab the src value of the tab's favicon (or even copy the node) instead of all that code to get the icon.
(In reply to Frank Yan (:fryn) from comment #31)
> Comment on attachment 582147 [details] [diff] [review]
> WIP of patch for bug 588270
> 
> Review of attachment 582147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for writing a patch! :)
> 
> ::: browser/base/content/browser.js
> @@ +8179,5 @@
> > +    ctx.font = "12px Segoe UI, Helvetica, sans-serif";
> > +    ctx.fillText(text, textOffset, 16);
> > +    ctx.save();
> > +
> > +    dt.setDragImage(canvas, 16, 16);
> 
> Do we really want to use a canvas for this stuff? If we want to include more
> than the favicon as the drag image, I'd prefer using a <xul:panel/> with
> transient contents, which are easily style-able without hard-coding anything.

I hadn't thought about using a xul:panel for this. This is exactly why I requested feedback :)

Thanks for the great idea, I will spend more time on this shortly (after doing some work on smooth scrolling).

> Also, since the tab is already displaying the favicon, we can probably just
> grab the src value of the tab's favicon (or even copy the node) instead of
> all that code to get the icon.

Grabbing the src value of the tab's favicon and using |img.src = tabFaviconSrc;| isn't instantaneous and will require that code be moved to an onload handler which won't work with the synchronous nature of onDragStart.
(In reply to Jared Wein [:jwein and :jaws] from comment #32)
> Grabbing the src value of the tab's favicon and using |img.src =
> tabFaviconSrc;| isn't instantaneous and will require that code be moved to
> an onload handler which won't work with the synchronous nature of
> onDragStart.

Ah, okay. To be clear, if you use a <xul:panel><xul:image src="..."/></xul:panel>, then the image would still show up, but there might be a delay, which isn't pretty, so let's not do that.

How about background-image: -moz-element(); and document.mozSetImageElement?
That should be instantaneous.
You don't even need the tab's favicon to have an ID to use that.
https://developer.mozilla.org/en/DOM/document.mozSetImageElement
(In reply to Jared Wein [:jwein and :jaws] from comment #30)
> (In reply to Jared Wein [:jwein and :jaws] from comment #29)
> > Created attachment 582147 [details] [diff] [review]
> > WIP of patch for bug 588270
> 
> Pushed to UX: https://hg.mozilla.org/projects/ux/rev/8c6f56e1a30a

This was backed out since I forgot to make the necessary changes to pinstripe and gnomestripe.
Attached patch WIP of patch for bug 588270 v.1 (obsolete) — Splinter Review
I've added the pinstripe and gnomestripe changes and pushed to UX:
https://hg.mozilla.org/projects/ux/rev/f0f24e6d7587
Attachment #582147 - Attachment is obsolete: true
Attachment #582147 - Flags: feedback?(fryn)
In reply to Jared Wein [:jwein and :jaws] from comment #29)
> Created attachment 582147 [details] [diff] [review]
> WIP of patch for bug 588270
> 
> This is the current WIP of my patch for this bug. I have removed the site
> identity block when viewing a page over http:, chrome:, about:, and https:
> where there is mixed content.

Is there a way to activate the identity box again on these pages ? I mean the identity box does not have the collapsed state as true or the hidden state as true. Then by which property we can check (an extension can check) that the identity block is not at all present ?
(In reply to scrapmachines from comment #36)
> In reply to Jared Wein [:jwein and :jaws] from comment #29)
> > Created attachment 582147 [details] [diff] [review]
> > WIP of patch for bug 588270
> > 
> > This is the current WIP of my patch for this bug. I have removed the site
> > identity block when viewing a page over http:, chrome:, about:, and https:
> > where there is mixed content.
> 
> Is there a way to activate the identity box again on these pages ? I mean
> the identity box does not have the collapsed state as true or the hidden
> state as true. Then by which property we can check (an extension can check)
> that the identity block is not at all present ?

There isn't a good way to activate it on these types of sites with this patch. I'll see about adding |collapsed="true"|.
(In reply to Jared Wein [:jwein and :jaws] from comment #37)
> (In reply to scrapmachines from comment #36)
> > In reply to Jared Wein [:jwein and :jaws] from comment #29)
> > > Created attachment 582147 [details] [diff] [review]
> > > WIP of patch for bug 588270
> > > 
> > > This is the current WIP of my patch for this bug. I have removed the site
> > > identity block when viewing a page over http:, chrome:, about:, and https:
> > > where there is mixed content.
> > 
> > Is there a way to activate the identity box again on these pages ? I mean
> > the identity box does not have the collapsed state as true or the hidden
> > state as true. Then by which property we can check (an extension can check)
> > that the identity block is not at all present ?
> 
> There isn't a good way to activate it on these types of sites with this
> patch. I'll see about adding |collapsed="true"|.

Even if I can get to know that there is no Identity block, then it would be fine (as per my extension). Is there a way to know that ?
I don't think we should expose an API specifically for determining whether the identity block is visible or not, but if an add-on could match the visibility by using the same selector that we use -- #identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) -- or determine it by checking getComputedStyle(document.getElementById('identity-box')).visibility, etc.
Depends on: 712184
The patches in bug 499008 fix issues with dragging text out of the address bar. I will continue to work on this bug, but the patches will be dependent on the patches within bug 499008.
Depends on: 499008
Attached patch WIP of patch for bug 588270 v.2 (obsolete) — Splinter Review
This is an updated version of the patch, and requires the patches in bug 499008 and bug 707382 to build.

I will be pushing this to the UX branch shortly.
Attachment #582180 - Attachment is obsolete: true
Attachment #585581 - Flags: feedback?(fryn)
This is a screenshot of what the drag image can look like when the URL is dragged to the desktop. This is the same look and feel if the bookmark star is dragged to the desktop. I will upload a video as well.

Please note: This screenshot was photoshopped to show the drag cursor in the middle of the drag image. The correct positioning of the cursor is blocked by bug 712184.
Attachment #585652 - Flags: ui-review?(limi)
Attached video Video of the patch
Attachment #585653 - Flags: ui-review?(limi)
Attachment #585653 - Attachment is patch: false
Attachment #585653 - Attachment mime type: text/plain → video/ogg
Comment on attachment 585652 [details]
Screenshot of patch (with 712184 pretended to be fixed)

Nice work!
Attachment #585652 - Flags: ui-review?(limi) → ui-review+
Comment on attachment 585653 [details]
Video of the patch

Dragging of the text looks great!

Not sure why we'd make the star draggable, it seems like one of those things that very few people would do, and we'd end up having to support later, would trigger unexpected edge cases, or similar.

I don't have a strong opinion either way, but unless there's a compelling reason for making the star draggable (it's weird when it turns yellow, then back to white again too), I think we should drop that particular part.

Lots of love for making text draggable! :D
Attachment #585653 - Flags: ui-review?(limi) → ui-review+
Comment on attachment 585581 [details] [diff] [review]
WIP of patch for bug 588270 v.2

>+    let text = content.document.title || content.document.location;

gBrowser.selectedTab.label

>+    panelLabel.setAttribute("value", text);
>+
>+    for each(var tab in gBrowser.tabs) {
>+      if (tab.linkedBrowser == gBrowser.selectedBrowser) {
>+        let faviconImage = document.getAnonymousElementByAttribute(tab, "class", "tab-icon-image");

gBrowser.selectedTab

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#identity-box {
>+  visibility: collapse;
> }

Put this in browser/base/content/browser.css
Dropping the dragged text (URL) to the same tab should not cause a reload.
Attached patch WIP of patch for bug 588270 v.3 (obsolete) — Splinter Review
I've updated the patch based on feedback from Dao and fixed the visibility of the identity box for Linux and Mac OS X. I also removed the drag handler for the bookmark star based on the feedback from Limi.
Attachment #585581 - Attachment is obsolete: true
Attachment #586006 - Flags: feedback?(fryn)
Attachment #585581 - Flags: feedback?(fryn)
Comment on attachment 586006 [details] [diff] [review]
WIP of patch for bug 588270 v.3

>+#identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {

>+  max-width: none;
>+  overflow: visible;

This also belongs in browser/base/content/browser.css.

Why are you using max-width and overflow instead of visibility or display? Please document this in the code.
(In reply to Dão Gottwald [:dao] from comment #50)
> Comment on attachment 586006 [details] [diff] [review]
> 
> Why are you using max-width and overflow instead of visibility or display?
> Please document this in the code.

This is because I haven't been able to get margins to take effect when using |visibility: collapse;| (even though https://developer.mozilla.org/en/CSS/visibility says that |visibility: collapse;| will let margins still take effect on XUL elements).

I'll add a comment explaining this in the next version of the patch.
Attachment #466899 - Attachment is obsolete: true
The scope of this bug has been reduced (from what was proposed in comment #0) to remove the favicon from the site identity block but leave the URL display unchanged.
Summary: Reduce redundancy with the site identity block + URL → Reduce redundancy with the favicons in the address bar and location bar
Since the scope of this bug has been changed is there any plans concerning url redundancy (the global idea of Faaborg's mockup was very good) ?
(In reply to GE3K0S from comment #55)
> Since the scope of this bug has been changed is there any plans concerning
> url redundancy (the global idea of Faaborg's mockup was very good) ?

Not that I know of at the moment. If we are going to work on removing URL redundancy, we will need to find a way that is hard for users to get tricked (http://badguy.com/https://www.paypal.com), easy to use, and pleasant on the eyes.
Attached patch Patch for bug 588270 (obsolete) — Splinter Review
Note that this patch is dependent on bug 499008 and bug 707382.
Attachment #586006 - Attachment is obsolete: true
Attachment #589299 - Flags: review?(dao)
Attachment #586006 - Flags: feedback?(fryn)
Attached patch Patch for bug 588270 (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #50)
> Why are you using max-width and overflow instead of visibility or display?
> Please document this in the code.

Added.
Attachment #589299 - Attachment is obsolete: true
Attachment #589300 - Flags: review?(dao)
Attachment #589299 - Flags: review?(dao)
Comment on attachment 589300 [details] [diff] [review]
Patch for bug 588270

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

>> dt.setDragImage(panel, -1, -1);
I recommend including a concise TODO comment explaining that the desired positive offset can't be applied until bug 712184 is fixed.

::: browser/base/content/browser.xul
@@ +344,5 @@
>        </hbox>
>      </panel>
>  
> +    <panel id="identity-drag-panel" allowevents="false" type="drag">
> +      <label id="identity-drag-label"></label>

Nit: <label id="identity-drag-label"/>
Attached patch Patch for bug 588270 v1.1 (obsolete) — Splinter Review
Addressed Frank's comments.
Attachment #589300 - Attachment is obsolete: true
Attachment #589359 - Flags: review?(dao)
Attachment #589300 - Flags: review?(dao)
Is this bug supposed to be titled "Reduce redundancy with the favicons in the location bar and tab" instead of what it is currently titled?

(I am dropping myself from the CC so I won't see the reply to this. This is just a drive-by suggestion in the form of a question.)
(In reply to Brian Smith (:bsmith) from comment #61)
> Is this bug supposed to be titled "Reduce redundancy with the favicons in
> the location bar and tab" instead of what it is currently titled?

Yes. :)
Summary: Reduce redundancy with the favicons in the address bar and location bar → Reduce redundancy with the favicons in the location bar and tab
Dao: review ping?
Comment on attachment 589359 [details] [diff] [review]
Patch for bug 588270 v1.1

>+#identity-box {
>+  /* XXX Setting max-width:0 and overflow:hidden so the margins of the identity-box
>+     are still applied. visibility:collapse did not still apply margins as hoped. */
>+  max-width: 0;
>+  overflow: hidden;
>+}

This leaves the identity box focusable with the tab key. You should get rid of the margins (only affects pinstripe, right?) and use visibility:collapse.

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

> #urlbar {
>   width: 7em;
>   min-width: 7em;
>   -moz-padding-end: 2px;
>+  height: 24px;
>+  max-height: 24px;

What's the max-height doing here?
Attachment #589359 - Flags: review?(dao) → review-
Attached patch Patch for bug 588270 v1.2 (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #64)
> Comment on attachment 589359 [details] [diff] [review]
> Patch for bug 588270 v1.1
> 
> >+#identity-box {
> >+  /* XXX Setting max-width:0 and overflow:hidden so the margins of the identity-box
> >+     are still applied. visibility:collapse did not still apply margins as hoped. */
> >+  max-width: 0;
> >+  overflow: hidden;
> >+}
> 
> This leaves the identity box focusable with the tab key. You should get rid
> of the margins (only affects pinstripe, right?) and use visibility:collapse.

This affects pinstripe and winstripe. Applying the margins in the urlbar would require that the CSS selector have knowledge of the attributes on the identity-box as it is currently implemented.

Would you be OK with these attributes also being set on the URL bar?
 
> >--- a/browser/themes/winstripe/browser.css
> >+++ b/browser/themes/winstripe/browser.css
> 
> > #urlbar {
> >   width: 7em;
> >   min-width: 7em;
> >   -moz-padding-end: 2px;
> >+  height: 24px;
> >+  max-height: 24px;
> 
> What's the max-height doing here?

I think it was left-over crud. I've removed it and didn't see any change.
Attachment #589359 - Attachment is obsolete: true
Attachment #591312 - Flags: review?(dao)
(In reply to Jared Wein [:jaws] from comment #65)
> (In reply to Dão Gottwald [:dao] from comment #64)
> > >+#identity-box {
> > >+  max-width: 0;
> > >+  overflow: hidden;
> > >+}
> > 
> > This leaves the identity box focusable with the tab key. You should get rid
> > of the margins (only affects pinstripe, right?) and use visibility:collapse.
> 
> This affects pinstripe and winstripe. Applying the margins in the urlbar
> would require that the CSS selector have knowledge of the attributes on the
> identity-box as it is currently implemented.
> 
> Would you be OK with these attributes also being set on the URL bar?

In order to get this to look right, the url bar would have to know the identity mode (verified vs. unknown/chrome) as well as if the forward button is disabled. Could I continue to use the |max-width:0; overflow:hidden;| and add |-moz-user-focus: ignore;| when the identity-box is hidden?
Attached patch Patch for bug 588270 v1.3 (obsolete) — Splinter Review
This patch fixes the issue of the identity-box being getting focused when it is hidden.
Attachment #591312 - Attachment is obsolete: true
Attachment #591368 - Flags: review?(dao)
Attachment #591312 - Flags: review?(dao)
(In reply to Jared Wein [:jaws] from comment #65)
> Created attachment 591312 [details] [diff] [review]
> Patch for bug 588270 v1.2
> 
> (In reply to Dão Gottwald [:dao] from comment #64)
> > Comment on attachment 589359 [details] [diff] [review]
> > Patch for bug 588270 v1.1
> > 
> > >+#identity-box {
> > >+  /* XXX Setting max-width:0 and overflow:hidden so the margins of the identity-box
> > >+     are still applied. visibility:collapse did not still apply margins as hoped. */
> > >+  max-width: 0;
> > >+  overflow: hidden;
> > >+}
> > 
> > This leaves the identity box focusable with the tab key. You should get rid
> > of the margins (only affects pinstripe, right?) and use visibility:collapse.
> 
> This affects pinstripe and winstripe.

So this is the fault of your patch, since the identity box currently doesn't have any margin in winstripe...
(In reply to Dão Gottwald [:dao] from comment #68)
> (In reply to Jared Wein [:jaws] from comment #65)
> > Created attachment 591312 [details] [diff] [review]
> > Patch for bug 588270 v1.2
> > 
> > (In reply to Dão Gottwald [:dao] from comment #64)
> > > Comment on attachment 589359 [details] [diff] [review]
> > > Patch for bug 588270 v1.1
> > > 
> > > >+#identity-box {
> > > >+  /* XXX Setting max-width:0 and overflow:hidden so the margins of the identity-box
> > > >+     are still applied. visibility:collapse did not still apply margins as hoped. */
> > > >+  max-width: 0;
> > > >+  overflow: hidden;
> > > >+}
> > > 
> > > This leaves the identity box focusable with the tab key. You should get rid
> > > of the margins (only affects pinstripe, right?) and use visibility:collapse.
> > 
> > This affects pinstripe and winstripe.
> 
> So this is the fault of your patch, since the identity box currently doesn't
> have any margin in winstripe...

Yeah, that's true. As stated in comment #65 and comment #66, the only other way that I can think of is to place attributes (forwarddisabled and identityMode) on the anonymous content generated by the urlbar bindings. That alternative approach seems like a worse solution than what I have proposed.

Is adding this margin an unacceptable change?
With this bug resolved will there be any way to open the Larry Panel for normal http sites ?
Yes, with text.
(In reply to GE3K0S from comment #70)
> With this bug resolved will there be any way to open the Larry Panel for
> normal http sites ?

Yes, but using a different way. The Larry Panel is accessible today via the identity block, page context menu -> View Page Info -> Security tab, and Tools menu -> Page Info -> Security tab.

When this bug is resolved the first way will not be available for normal http sites however the other two ways will remain available.
(In reply to Jared Wein [:jaws] from comment #69)
> Is adding this margin an unacceptable change?

No, but it should be avoided if possible. See bug 721153, by the way.
(In reply to Dão Gottwald [:dao] from comment #74)
> (In reply to Jared Wein [:jaws] from comment #69)
> > Is adding this margin an unacceptable change?
> 
> No, but it should be avoided if possible. See bug 721153, by the way.

Bug 721153 has been fixed in the latest patch that has been attached to this bug. The patch on the UX branch is out of date.
Attached image new style Windows 7 (obsolete) —
I think that the current style of "identity-drag-panel" does not fit the style of Windows 7 It should be similar to the style tooltip.

#identity-drag-panel {
  background: -moz-linear-gradient (top, #FFFFFF 0%, #E4E5F0 99%); 
  border: 1px solid #767676;
  border-radius: 3px;
  pointer-events: none;
  padding: 2px;
}
Attached patch Patch for bug 588270 v1.4 (obsolete) — Splinter Review
(In reply to fx4waldi from comment #76)
> Created attachment 592256 [details]
> new style Windows 7
> 
> I think that the current style of "identity-drag-panel" does not fit the
> style of Windows 7 It should be similar to the style tooltip.

Thanks fx4waldi, I agree with you. I've added these styles for aero with a small tweak to increase the padding.
Attachment #591368 - Attachment is obsolete: true
Attachment #592256 - Attachment is obsolete: true
Attachment #593024 - Flags: review?(dao)
Attachment #591368 - Flags: review?(dao)
Does your patch handle browser.identity.ssl_domain_display = 0 in some reasonable way?
Comment on attachment 593024 [details] [diff] [review]
Patch for bug 588270 v1.4

No, it doesn't handle browser.identity.ssl_domain_display=0 in a reasonable way. Thanks for letting me know about browser.identity.ssl_domain_display=0. I will look in to it.
Attachment #593024 - Flags: review?(dao)
Summary: Reduce redundancy with the favicons in the location bar and tab → Remove the favicon from the location bar
Doesn't this also hide the identity box on non-secure domains?
(In reply to Siddhartha Dugar (sdrocking) from comment #80)
> Doesn't this also hide the identity box on non-secure domains?

It does. Bug 587901 used to avoid this by adding a down arrow.
On this mockup there is a box for non-secure sites, but no arrow : http://f.cl.ly/items/1N1m3j1h0t1P1Y0U0i3q/DevTools-i02-%28OSX%29-Tools-Webconsole-v09.png
(In reply to Guillaume C. [:GE3K0S] from comment #82)
> On this mockup there is a box for non-secure sites, but no arrow :
> http://f.cl.ly/items/1N1m3j1h0t1P1Y0U0i3q/DevTools-i02-%28OSX%29-Tools-
> Webconsole-v09.png

That graphic was only focused on the devtools. The design of the urlbar is out of date.
(In reply to Jared Wein [:jaws] from comment #83)
> (In reply to Guillaume C. [:GE3K0S] from comment #82)
> > On this mockup there is a box for non-secure sites, but no arrow :
> > http://f.cl.ly/items/1N1m3j1h0t1P1Y0U0i3q/DevTools-i02-%28OSX%29-Tools-
> > Webconsole-v09.png
> 
> That graphic was only focused on the devtools. The design of the urlbar is
> out of date.
Where could I find the most recent mockups (or simply more recent ones) ? I thought this was one of the last mockup (clearly for devtools but also with the last changes for australis).
Yes, A current/recent mockup is required for this bug as it is an important take off.
bug 723580 reminded me that we need an identity button for mixed content, as https alone might make users think they're secure.
I was thinking. Why not use same style as in secured identibox? Only for non-secure it will be grey and domain will be in it.
(In reply to Dão Gottwald [:dao] from comment #86)
> bug 723580 reminded me that we need an identity button for mixed content, as
> https alone might make users think they're secure.

One of the suggestions in Bug 62178 is to always show an infobar warning for mixed content. I would rather have us do that then rely on the user to notice that the site identity block is gray instead of blue or green; the gray site identity block is very hard to notice. Also, the site identity block is about the site's identity, and we should not confuse that by mixing into that indicator whether or not the page has non-HTTPS content in it.
Why not use another color ? I was thinking about hazel.
Two main points :
On the latest UX build with this bug landed

1. The margin/padding occupied by the identity box in case of secured site like github.com or gmail.com is vertically 1px more than when you open a page with normal security like google.com. This is due to the fact that on a non-secured site, there is no identity-box, and on a secured site, there is one, and the margins of identity box are not consistent with that of urlbar. So switching from a secured site's tab to a non secured one shift up and down the navigation toolbar by 1 px.

NOTE: This behavior is not seen when I restart in safe-mode with add-ons disable, how much ever customization I do. But If in a normal profile I disable all the add-ons, change the icons and buttons so as to mimic the safe mode structure of icons/buttons/search bar/urlbar , and restart browser (normally) as many times, still the behavior is seen.

2. Why does the identity box does not show up even on a secured site when the unified back forward button is not next to location bar ? IMO it should show up what so ever by the position of back forward button if the site is secured.
(In reply to scrapmachines from comment #90)
> 2. Why does the identity box does not show up even on a secured site when
> the unified back forward button is not next to location bar ? IMO it should
> show up what so ever by the position of back forward button if the site is
> secured.

Agreed... this makes absolutely no sense.  Why would the secure identity box *ever* be hidden?  This is the only code for showing the identity box, correct?

window:not([chromehidden~=toolbar]) #navigator-toolbox[iconsize=large][mode=icons] > :-moz-any(#nav-bar[currentset*="unified-back-forward-button,urlbar-container"],#nav-bar:not([currentset])) > #unified-back-forward-button + #urlbar-container > #urlbar > #identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {
  -moz-border-end: 1px solid hsla(0,0%,0%,.1);
  max-width: none;
  overflow: visible;
  -moz-margin-start: 0;
}

Shouldn't that just be:

#identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {
  -moz-border-end: 1px solid hsla(0,0%,0%,.1);
  max-width: none;
  overflow: visible;
  -moz-margin-start: 0;
}
Attached patch Patch for bug (obsolete) — Splinter Review
Thank you Dao, Guillaume, scrapmachines, and patrickjdempsey for finding issues with the previous patch and letting me know about them.

I believe I have fixed the issues previously mentioned in this version of the patch.
Attachment #593024 - Attachment is obsolete: true
Attachment #597101 - Flags: review?(dao)
(In reply to scrapmachines from comment #90)
> 1. The margin/padding occupied by the identity box in case of secured site
> like github.com or gmail.com is vertically 1px more than when you open a
> page with normal security like google.com...

Isn't this because of Bug 689079?
(In reply to Siddhartha Dugar (sdrocking) from comment #93)
> (In reply to scrapmachines from comment #90)
> > 1. The margin/padding occupied by the identity box in case of secured site
> > like github.com or gmail.com is vertically 1px more than when you open a
> > page with normal security like google.com...
> 
> Isn't this because of Bug 689079?

This instance was because of too much vertical padding on the identity box and wasn't related to that bug.
(In reply to Siddhartha Dugar (sdrocking) from comment #93)
> (In reply to scrapmachines from comment #90)
> > 1. The margin/padding occupied by the identity box in case of secured site
> > like github.com or gmail.com is vertically 1px more than when you open a
> > page with normal security like google.com...
> 
> Isn't this because of Bug 689079?

If it were because of that bug, then Why was it not visible in latest Nightlies, and was visible in latest UX build patched with this bug's previous fix ?
Also, the STR of that bug involves changing some default settings of Windows.
Comment on attachment 597101 [details] [diff] [review]
Patch for bug

Frank and Joshua, can you provide some feedback on this patch?
Attachment #597101 - Flags: feedback?(soapyhamhocks)
Attachment #597101 - Flags: feedback?(fryn)
Comment on attachment 597101 [details] [diff] [review]
Patch for bug

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

Jared, is there any specific feedback you would like?
I don't

::: browser/base/content/browser.xul
@@ +524,1 @@
>                   onblur="setTimeout(function() document.getElementById('identity-box').style.MozUserFocus = '', 0);">

nit: consistent whitespace plz, i.e. idBox.style.MozUserFocus = 'normal'

Why can't these two handlers be done in CSS, e.g.


#urlbar:focus > #identity-box.verifiedIdentity,
#urlbar:focus > #identity-box.verifiedDomain {

  -moz-user-focus: normal;
}
Attachment #597101 - Flags: feedback?(fryn)
(In reply to Jared Wein [:jaws] from comment #96)
> Comment on attachment 597101 [details] [diff] [review]
> Patch for bug
> 
> Frank and Joshua, can you provide some feedback on this patch?

I just tested your patch and it works great. I noticed an area where you could use shorthand padding/margin.

>browser/themes/gnomestripe/browser.css
> /* Identity indicator */
>-#identity-box {
>+#identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {
>   background-image: -moz-linear-gradient(hsl(0,0%,98%), hsl(0,0%,92%));
>   box-shadow: 0 1px 0 hsla(0,0%,0%,.05) inset;
>   -moz-border-end: 1px solid rgba(0,0,0,.1);
>   padding: 1px;
>+  -moz-padding-start: 4px;
>+  -moz-padding-end: 4px;
>   margin: -1px;
>+  -moz-margin-start: 0;
>   -moz-margin-end: 0;
>+  max-width: none;
>+  overflow: visible;
> }

Maybe just use padding: 1px 4px and margin: -1px 0 here?

While not directly related to this bug I thought I would mention it. The idbox seems to cause the urlbar to increase in size when Windows DPI settings are above the default. I tried removing the top/bottom padding and it seemed to fix it, while displaying correctly with normal DPI settings.

First time giving some feedback so I hope it is of some use.
(In reply to Frank Yan (:fryn) from comment #97)
> Comment on attachment 597101 [details] [diff] [review]
> Patch for bug
> 
> Review of attachment 597101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jared, is there any specific feedback you would like?
> I don't
> 
> ::: browser/base/content/browser.xul
> @@ +524,1 @@
> >                   onblur="setTimeout(function() document.getElementById('identity-box').style.MozUserFocus = '', 0);">
> 
> nit: consistent whitespace plz, i.e. idBox.style.MozUserFocus = 'normal'

Are you asking that the event handlers for these two lines have the MozUserFocus assignment horizontally aligned? I don't think adding more code for horizontal alignment here is worth it. I would much rather find a way to make the |onfocus| handler shorter.

> Why can't these two handlers be done in CSS, e.g.
> 
> 
> #urlbar:focus > #identity-box.verifiedIdentity,
> #urlbar:focus > #identity-box.verifiedDomain {
> 
>   -moz-user-focus: normal;
> }

I made these changes and they don't provide the same behavior as the inline event handlers. Without digging deeper in to it, I believe it's because the blur event removes the focus of the #urlbar before focus is applied to the #identity-box. This would explain why we need the setTimeout in the onblur event handler.
Attached patch Patch for bug v2 (obsolete) — Splinter Review
(In reply to Joshua M from comment #98)
> >browser/themes/gnomestripe/browser.css
> > /* Identity indicator */
> >-#identity-box {
> >+#identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {
> >   background-image: -moz-linear-gradient(hsl(0,0%,98%), hsl(0,0%,92%));
> >   box-shadow: 0 1px 0 hsla(0,0%,0%,.05) inset;
> >   -moz-border-end: 1px solid rgba(0,0,0,.1);
> >   padding: 1px;
> >+  -moz-padding-start: 4px;
> >+  -moz-padding-end: 4px;
> >   margin: -1px;
> >+  -moz-margin-start: 0;
> >   -moz-margin-end: 0;
> >+  max-width: none;
> >+  overflow: visible;
> > }
> 
> Maybe just use padding: 1px 4px and margin: -1px 0 here?

Thanks for catching that.

> While not directly related to this bug I thought I would mention it. The
> idbox seems to cause the urlbar to increase in size when Windows DPI
> settings are above the default. I tried removing the top/bottom padding and
> it seemed to fix it, while displaying correctly with normal DPI settings.

Hmmm, when I was working on this earlier I thought I needed the padding here so the urlbar height wouldn't shrink, but I couldn't reproduce it anymore. Thanks for catching this :)
Attachment #597101 - Attachment is obsolete: true
Attachment #599586 - Flags: review?(dao)
Attachment #597101 - Flags: review?(dao)
Attachment #597101 - Flags: feedback?(soapyhamhocks)
Comment on attachment 599586 [details] [diff] [review]
Patch for bug v2

>         case 2 : // Show full domain
>+          if (this._identityBox.hasAttribute("identityMessageHidden"))
>+            this._identityBox.removeAttribute("identityMessageHidden");
>           icon_label = this._lastLocation.hostname;
>           break;
>         case 1 : // Show eTLD.
>+          if (this._identityBox.hasAttribute("identityMessageHidden"))
>+            this._identityBox.removeAttribute("identityMessageHidden");
>           icon_label = this.getEffectiveHost();
>+          break;
>+        case 0 : // Hide the identity-box
>+          if (!this._identityBox.hasAttribute("identityMessageHidden"))
>+            this._identityBox.setAttribute("identityMessageHidden", "true");
>+          break;

>     else if (newMode == this.IDENTITY_MODE_IDENTIFIED) {
>+      if (this._identityBox.hasAttribute("identityMessageHidden"))
>+        this._identityBox.removeAttribute("identityMessageHidden");

The hasAttribute checks aren't useful.

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

> #urlbar {
>   width: 7em;
>   min-width: 7em;
>   -moz-appearance: textfield;
>   padding: 0;
>+  height: 26px;

With an increased font size, this causes the location bar to shrink vertically when the identity box is hidden.

>+#identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {

As far as I remember, -moz-any shouldn't be used at the end of a selector.

On Ubuntu, the identity box doesn't touch the left location bar border as it should.

>+%ifndef WINSTRIPE_AERO
>+#identity-drag-panel {
>+  background-color: InfoBackground;
>+  border: none;
>+  pointer-events: none;
>+  padding: 4px;
>+}
>+%endif
>+
>+#identity-drag-panel {
>+  background-image: -moz-linear-gradient(to bottom, #FFFFFF 0%, #E4E5F0 99%); 
>+  border: 1px solid #767676;
>+  border-radius: 3px;
>+  pointer-events: none;
>+  padding: 4px;
>+}

What exactly is this ifndef trying to do?
Attachment #599586 - Flags: review?(dao) → review-
During customization the identity box disappears and the URL bar becomes empty. This is a regression.
Was the patch that landed on UX few hours ago the recent one ?
(In reply to scrapmachines from comment #103)
> Was the patch that landed on UX few hours ago the recent one ?

No it was there previously also. I was waiting to see if it got fixed.
(In reply to Siddhartha Dugar (sdrocking) from comment #104)
> (In reply to scrapmachines from comment #103)
> > Was the patch that landed on UX few hours ago the recent one ?
> 
> No it was there previously also. I was waiting to see if it got fixed.

That's why I am asking Jared, whether the patch that landed on UX the latest one. Its name sure is the older name of this bug.
(In reply to Girish Sharma from comment #103)
> Was the patch that landed on UX few hours ago the recent one ?

Yes, the patch on the UX branch is the most recent one that I have and is the most recent patch attached to this bug (attachment #599586 [details] [diff] [review]). It doesn't include the changes requested by Dao though, since I haven't finished updating the patch yet.

(In reply to Siddhartha Dugar (sdrocking) from comment #102)
> During customization the identity box disappears and the URL bar becomes
> empty. This is a regression.

Thanks for letting me know about the "identity-box disappearing during customization" bug. I'll try to include a fix for that bug in my next version of the patch.
(In reply to Siddhartha Dugar (sdrocking) from comment #102)
> During customization the identity box disappears and the URL bar becomes
> empty. This is a regression.

The URL bar becomes empty, but the identity box is still visible if the site that was being viewed is over HTTPS without any mixed content. I checked with Firefox 10 and didn't see a regression here.
Attached patch Patch for bug v3 (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #101)
> Comment on attachment 599586 [details] [diff] [review]
> Patch for bug v2
> 
> >         case 2 : // Show full domain
> >+          if (this._identityBox.hasAttribute("identityMessageHidden"))
> >+            this._identityBox.removeAttribute("identityMessageHidden");
> >           icon_label = this._lastLocation.hostname;
> >           break;
> >         case 1 : // Show eTLD.
> >+          if (this._identityBox.hasAttribute("identityMessageHidden"))
> >+            this._identityBox.removeAttribute("identityMessageHidden");
> >           icon_label = this.getEffectiveHost();
> >+          break;
> >+        case 0 : // Hide the identity-box
> >+          if (!this._identityBox.hasAttribute("identityMessageHidden"))
> >+            this._identityBox.setAttribute("identityMessageHidden", "true");
> >+          break;
> 
> >     else if (newMode == this.IDENTITY_MODE_IDENTIFIED) {
> >+      if (this._identityBox.hasAttribute("identityMessageHidden"))
> >+        this._identityBox.removeAttribute("identityMessageHidden");
> 
> The hasAttribute checks aren't useful.

OK, I've removed them. Does the presence of these hasAttribute checks reduce how often style resolution has to occur?

> >--- a/browser/themes/gnomestripe/browser.css
> >+++ b/browser/themes/gnomestripe/browser.css
> 
> > #urlbar {
> >   width: 7em;
> >   min-width: 7em;
> >   -moz-appearance: textfield;
> >   padding: 0;
> >+  height: 26px;
> 
> With an increased font size, this causes the location bar to shrink
> vertically when the identity box is hidden.

I've removed the |height| here and put some vertical padding on the |.urlbar-input|. Without the vertical padding, the location bar shrinks vertically as you have seen.

> >+#identity-box:-moz-any(.verifiedIdentity, .verifiedDomain) {
> 
> As far as I remember, -moz-any shouldn't be used at the end of a selector.

OK, I've switched to just using two separate selectors here.

> On Ubuntu, the identity box doesn't touch the left location bar border as it
> should.

OK, I've fixed this.

> >+%ifndef WINSTRIPE_AERO
> >+#identity-drag-panel {
> >+  background-color: InfoBackground;
> >+  border: none;
> >+  pointer-events: none;
> >+  padding: 4px;
> >+}
> >+%endif
> >+
> >+#identity-drag-panel {
> >+  background-image: -moz-linear-gradient(to bottom, #FFFFFF 0%, #E4E5F0 99%); 
> >+  border: 1px solid #767676;
> >+  border-radius: 3px;
> >+  pointer-events: none;
> >+  padding: 4px;
> >+}
> 
> What exactly is this ifndef trying to do?

This is here so that Aero has a more native look for the drag panel. I've switched the ifndef to ifdef and only apply the rules that are different now.
Attachment #599586 - Attachment is obsolete: true
Attachment #600148 - Flags: review?(dao)
Attached patch Patch for bug v3.1 (obsolete) — Splinter Review
Fixed the |onfocus| handler in browser.xul to use |var| instead of |let| and also a whitespace fix in the handler too.
Attachment #600148 - Attachment is obsolete: true
Attachment #600199 - Flags: review?(dao)
Attachment #600148 - Flags: review?(dao)
Made a small change for the |onblur| handler in browser.xul since the expression closure isn't running.
Attachment #600199 - Attachment is obsolete: true
Attachment #600204 - Flags: review?(dao)
Attachment #600199 - Flags: review?(dao)
(In reply to Jared Wein [:jaws] from comment #107)
> (In reply to Siddhartha Dugar (sdrocking) from comment #102)
> > During customization the identity box disappears and the URL bar becomes
> > empty. This is a regression.
> 
> The URL bar becomes empty, but the identity box is still visible if the site
> that was being viewed is over HTTPS without any mixed content. I checked
> with Firefox 10 and didn't see a regression here.

It was visible in the previous versions but it's gone from the UX builds. That's the regression.
The urlbar has been shown empty in customize mode since the dawn of eternity.  Now that it's basically just a totally empty textbox, there should probably be a customizing placeholder text like what appears for the Bookmarks Toolbar Items, but that would be a different bug.
In Windows Aero, with tabs-on-bottom, no identity block, and conditional Forward, if you browse and then hover the mouse over the Back button, the Forward button appears underneath the domain.
Comment on attachment 600204 [details] [diff] [review]
Patch for bug v3.2

When the notification icons are present, the background of the identity-box doesn't extend behind the notification icon. The urlbar also is about two pixels taller than it should be.
Attachment #600204 - Flags: review?(dao)
Attached patch Main patch for bug (obsolete) — Splinter Review
This patch contains the bulk of the changes along with the styling for windows
Attachment #610689 - Flags: feedback?(dao)
Attached patch Linux styles for bug (obsolete) — Splinter Review
Contains browser.css for Linux (gnomestripe)
Attachment #610692 - Flags: feedback?(dao)
contains browser.css for Mac OSX (pinstripe)
Attachment #610693 - Flags: feedback?(dao)
Attached file Main pat (obsolete) —
Fixed issues brought up in comment 114
Attachment #610689 - Attachment is obsolete: true
Attachment #610763 - Attachment is obsolete: true
Attachment #610765 - Flags: feedback?(dao)
Attachment #610689 - Flags: feedback?(dao)
Fixed issues brought up in comment 114
Attachment #610692 - Attachment is obsolete: true
Attachment #610766 - Flags: feedback?(dao)
Attachment #610692 - Flags: feedback?(dao)
This bug is being closed as we have decided to change directions and keep a drag target in the location bar. This new work will be done in bug 742419.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Attachment #610693 - Flags: feedback?(dao)
Attachment #610765 - Flags: feedback?(dao)
Attachment #610766 - Flags: feedback?(dao)
Flags: sec-review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: