Last Comment Bug 634065 - Implement design for identity block and persistent indicators
: Implement design for identity block and persistent indicators
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: Firefox 6
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 680811 641892 651326
Blocks: 579521 639155
  Show dependency treegraph
 
Reported: 2011-02-14 13:31 PST by John Ford [:jhford]
Modified: 2013-11-13 02:21 PST (History)
38 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
existing awesomebar with white border (4.57 KB, image/png)
2011-02-14 13:31 PST, John Ford [:jhford]
no flags Details
Extended Site Identity Button (514.66 KB, image/jpeg)
2011-02-18 14:20 PST, Stephen Horlander [:shorlander]
no flags Details
wip winstripe patch (6.20 KB, patch)
2011-02-24 15:53 PST, :Margaret Leibovic
no flags Details | Diff | Review
wip winstripe patch (7.06 KB, patch)
2011-02-24 15:56 PST, :Margaret Leibovic
no flags Details | Diff | Review
patch (20.57 KB, patch)
2011-02-28 18:04 PST, :Margaret Leibovic
fryn: feedback+
Details | Diff | Review
margarets patch on gnome (5.68 KB, image/png)
2011-03-01 17:55 PST, John Ford [:jhford]
no flags Details
patch addendum to use favicon as drag image (748 bytes, patch)
2011-03-12 23:31 PST, Frank Yan (:fryn)
no flags Details | Diff | Review
updated patch (24.30 KB, patch)
2011-03-16 16:17 PDT, :Margaret Leibovic
dao+bmo: feedback-
Details | Diff | Review
patch using -moz-image-rect for different arrow images (26.97 KB, patch)
2011-03-17 16:45 PDT, :Margaret Leibovic
no flags Details | Diff | Review
patch with two images (31.58 KB, patch)
2011-03-18 14:32 PDT, :Margaret Leibovic
dao+bmo: feedback-
Details | Diff | Review
patch (26.09 KB, patch)
2011-03-30 17:50 PDT, :Margaret Leibovic
dao+bmo: review-
Details | Diff | Review
patch v2 (27.37 KB, patch)
2011-03-31 12:59 PDT, :Margaret Leibovic
dao+bmo: review-
Details | Diff | Review
patch v3 (27.53 KB, patch)
2011-04-07 14:55 PDT, :Margaret Leibovic
dao+bmo: review-
Details | Diff | Review
patch v4 (27.55 KB, patch)
2011-04-08 09:35 PDT, :Margaret Leibovic
dao+bmo: review-
Details | Diff | Review
patch v5 (25.96 KB, patch)
2011-04-11 16:22 PDT, :Margaret Leibovic
dao+bmo: review-
Details | Diff | Review
patch v6 (25.11 KB, patch)
2011-04-13 14:40 PDT, Frank Yan (:fryn)
dao+bmo: review-
Details | Diff | Review
patch v7 (24.21 KB, patch)
2011-04-19 10:37 PDT, :Margaret Leibovic
dao+bmo: review+
Details | Diff | Review

Description John Ford [:jhford] 2011-02-14 13:31:46 PST
Created attachment 512259 [details]
existing awesomebar with white border

The left side of the awesomebar is visually inconsistent with the right side of the awesomebar because of a 1px white border around the domain name button.  This is on Linux, Mac and Windows.  On some platforms, the white border doesn't look equally sized, which makes it look like the button isn't aligned in the text box.

The picture that I have attached to this bug shows an example of this border.
Comment 1 Jennifer Morrow [:Boriss] (UX) 2011-02-14 13:38:45 PST
The problem (as I view it) is the 1-pixel thick "gutter" that currently surrounds the site identity button.  John is right: this is inconsistent with the right side of the URL bar.  However, it's a problem even beyond the inconsistency.  The 1-pixel gutter is visually awkward, creating a small white track that traps the eye and makes the identity button look as though it's rendering too small.  Also, because of the small shadow on the top of the URL bar, the gutter itself seems to thin at the top of the identity button and widen on the left and bottom.  We need to expand the identity button 2px on the top, left, and bottom to remove this gutter and bring it into visual consistency with the right side of the URL bar.
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-14 20:25:17 PST
This doesn't block release without an agreed-upon UX design fix. Renominate once that's worked through, if you'd like.
Comment 3 Jennifer Morrow [:Boriss] (UX) 2011-02-17 16:24:58 PST
UX talked about this bug on 2/14/2011.  Some discussion is still needed to decide what, if any, action will be taken in Firefox 4.

As Faaborg correctly pointed out, there's a lot we'd ideally like to do to improve the URL bar, and a lot of it will have to wait for future versions of Firefox.

However, I think we'd all agree that the narrow gutter that currently surrounds the site identity button is a problem.

We could extend the site identity button to eliminate this gutter.  That would mean that if a site asked for a permission, such as a password or geolocation, the site identity button would still slide to the right and the permission icon would appear on the left.  I think that  extending the site identity button to the bottom and top of the URL bar, even in this permission state, is a huge improvement visually over what we have now.  I propose that we extend this button for Firefox 4 and develop a more complete solution for future versions.

Thoughts, UX?
Comment 4 Stephen Horlander [:shorlander] 2011-02-18 14:20:28 PST
Created attachment 513594 [details]
Extended Site Identity Button

(In reply to comment #3)
> We could extend the site identity button to eliminate this gutter.  That would
> mean that if a site asked for a permission, such as a password or geolocation,
> the site identity button would still slide to the right and the permission icon
> would appear on the left.  I think that  extending the site identity button to
> the bottom and top of the URL bar, even in this permission state, is a huge
> improvement visually over what we have now.  I propose that we extend this
> button for Firefox 4 and develop a more complete solution for future versions.
> 
> Thoughts, UX?

I agree that extending the site identity button to eliminate the gutter would be the best short-term fix.

In addition to that I think using a flatter more subtle style would give the left and right side more visual consistency. Using a lighter color scheme would also be more friendly to a wider range of favicons.
Comment 5 Jennifer Morrow [:Boriss] (UX) 2011-02-23 13:12:38 PST
(In reply to comment #4)
> Created attachment 513594 [details]
> Extended Site Identity Button
> 
> (In reply to comment #3)
> > We could extend the site identity button to eliminate this gutter.  That would
> > mean that if a site asked for a permission, such as a password or geolocation,
> > the site identity button would still slide to the right and the permission icon
> > would appear on the left.  I think that  extending the site identity button to
> > the bottom and top of the URL bar, even in this permission state, is a huge
> > improvement visually over what we have now.  I propose that we extend this
> > button for Firefox 4 and develop a more complete solution for future versions.
> > 
> > Thoughts, UX?
> 
> I agree that extending the site identity button to eliminate the gutter would
> be the best short-term fix.
> 
> In addition to that I think using a flatter more subtle style would give the
> left and right side more visual consistency. Using a lighter color scheme would
> also be more friendly to a wider range of favicons.

That attached screenshot is such an improvement, Shorlander!  Wow!  It also  puts us in a good position to make future improvements to the URL bar and site identity button.  Shall we patch it?
Comment 6 :Margaret Leibovic 2011-02-24 15:31:57 PST
I'm working on this! Patch almost done for winstripe.
Comment 7 :Margaret Leibovic 2011-02-24 15:53:52 PST
Created attachment 514941 [details] [diff] [review]
wip winstripe patch

I just realized that this arrow approach is not completely correct because the urlbar is not white when it is not hovered/focused, so we would want the left side of the arrow to reflect that as well. I'm not sure of the best way to do that off the top of my head.

Also, I'm not sure what we should do about the hover/active styles for the identity box. It feels like keeping the current gradient styles makes the box look dirty for the verified states. Maybe we should just lighten it up?
Comment 8 :Margaret Leibovic 2011-02-24 15:56:16 PST
Created attachment 514942 [details] [diff] [review]
wip winstripe patch

Oops, forgot to add arrow image.
Comment 9 Jennifer Morrow [:Boriss] (UX) 2011-02-24 16:03:14 PST
(In reply to comment #8)
> Created attachment 514942 [details] [diff] [review]
> wip winstripe patch
> 
> Oops, forgot to add arrow image.

This looks fantastic, with dialogs left and without.  As Margaret pointed out to me, for now she's using a graphic for the arrow that isn't transparent, so there's a slight mismatch on the arrow when the left icons are hovered over between the white background of the arrow and the slightly blue mouseover color.  It's very slight, though, and a partially transparent image should take care of it.
Comment 10 Frank Yan (:fryn) 2011-02-24 16:12:38 PST
(In reply to comment #9)
> a partially transparent image should take care of it.

Actually, we'd have to add a transparent-to-white left-to-right gradient to the left of the arrow image, since with the current technique, the arrow needs to be opaque to cover the identity box.

A more robust solution would be to apply an svg clip (or mask) to the identity box, but that would require more complex code.

(In reply to comment #8)
> Created attachment 514942 [details] [diff] [review]
> wip winstripe patch

> #identity-box {
...
> -  border-radius: 2px;
> +  background-color: #ededed;
> +  border-top-left-radius: 2px;
> +  border-bottom-left-radius: 2px;

>  #notification-popup-box {
...
> +  background: url("chrome://browser/skin/urlbar-arrow.png") no-repeat right;

This will break in RTL.
Comment 11 Frank Yan (:fryn) 2011-02-24 17:26:29 PST
Comment on attachment 514942 [details] [diff] [review]
wip winstripe patch

I just realized that the background of the notification popup region can just be white. The current behavior of that region changing color (on Windows) when the url bar gets focused is odd, since conceptually it's not part of the editable input box. By making the notification popup region opaque, we eliminate this odd behavior and the transparent/white problem.

Overall, the appearance is already spectacular. :)
Comment 12 :Margaret Leibovic 2011-02-25 15:56:52 PST
Comment on attachment 514942 [details] [diff] [review]
wip winstripe patch

I have a new patch in the works that uses an SVG mask instead of an image. So far, it seems like a more elegant approach.
Comment 13 :Margaret Leibovic 2011-02-28 18:04:31 PST
Created attachment 515801 [details] [diff] [review]
patch

I decided to use SVG to make a background image for #notification-popup-box. I have a different version of the patch that applies an SVG mask to #identity-box, but that isn't very helpful because it doesn't give us a border along the edge of the arrow. 

I added a 4px gradient to the edge of the arrow on Windows, and it makes the background color for the urlbar pretty much impossible to notice, event at 8x zoom.

I like using SVG for the triangle because it will scale well for different font sizes, and it looks sharper than the image I used in my first patch. However, I'll run this through try, and if the SVG causes performance regressions, it will be easy to swap in an image (a better quality one than the one I was using before).

I also still need the correct color values from Stephen.
Comment 14 Frank Yan (:fryn) 2011-03-01 16:20:17 PST
Comment on attachment 515801 [details] [diff] [review]
patch

I tested it on Windows and OS X, in both LTR and RTL mode, and it looks good.
The only exception is that the popup-notification-icons need opaque backgrounds so that the non-white urlbar doesn't show through.
A skim of the code yielded no obvious issues. :)
I wonder if we could pull a |-moz-transform: scaleX(-1)| trick on the #notification-popup-box for the arrow instead of using two separate images. I think that would be a lot easier.
Comment 15 Frank Yan (:fryn) 2011-03-01 16:51:27 PST
Using |-moz-transform: scaleX(-1)| also means that we'd use -left and -right instead of -start- and -end.

And btw, it looks awesome!
Comment 16 John Ford [:jhford] 2011-03-01 17:55:53 PST
Created attachment 516104 [details]
margarets patch on gnome

This looks awesome!
Comment 17 Frank Yan (:fryn) 2011-03-01 18:01:21 PST
Margaret, it looks like the vertical alignment of the text in the identity-box doesn't match that of the location bar and search bar's text. On Windows and OS X, it's higher, and on Gnome, it's lower, but we already have this problem, so it's not a huge problem, if it remains that way.
Comment 18 Alex Limi (:limi) — Firefox UX Team 2011-03-01 23:23:04 PST
Renaming the bug, since it needs a more descriptive title (especially during these times of rapid triage and approvals).
Comment 19 Markus Stange [:mstange] 2011-03-03 04:46:18 PST
Comment on attachment 515801 [details] [diff] [review]
patch

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css
>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css
>@@ -774,17 +774,16 @@ toolbar[mode="icons"] #zoom-in-button {
> /* ::::: nav-bar-inner ::::: */
> 
> #urlbar,
> .searchbar-textbox {
>   font: icon;
>   width: 7em;
>   min-width: 7em;
>   -moz-appearance: none;
>-  box-shadow: 0 1px rgba(255, 255, 255, 0.2), inset 0 1px #d6d6d6;

Why this change? I think at least the outer shadow should stay.
Comment 20 Peter Henkel [:Terepin] 2011-03-03 06:42:22 PST
This is intended for Firefox 5?

I must say it looks awesome!
Comment 21 Peter Henkel [:Terepin] 2011-03-03 06:48:29 PST
One question: What's the point of keeping favicon in both places? I thought we are trying to get rid of duplication.
Comment 22 :Margaret Leibovic 2011-03-03 09:52:09 PST
(In reply to comment #19)
> Comment on attachment 515801 [details] [diff] [review]
> patch
> 
> >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css
> >--- a/browser/themes/pinstripe/browser/browser.css
> >+++ b/browser/themes/pinstripe/browser/browser.css
> >@@ -774,17 +774,16 @@ toolbar[mode="icons"] #zoom-in-button {
> > /* ::::: nav-bar-inner ::::: */
> > 
> > #urlbar,
> > .searchbar-textbox {
> >   font: icon;
> >   width: 7em;
> >   min-width: 7em;
> >   -moz-appearance: none;
> >-  box-shadow: 0 1px rgba(255, 255, 255, 0.2), inset 0 1px #d6d6d6;
> 
> Why this change? I think at least the outer shadow should stay.

I was trying to match Stephen's mock-up, but I guess I didn't think about the differences we want between OSX and Windows. The inner shadow interacts badly with the button, but I agree that the outer shadow could stay.
Comment 23 bb10 2011-03-12 21:21:35 PST
I find the SSL and EV states hard to see in that mockup. The SSL block background almost matches the nav toolbar background and the EV background is almost white.

The font color in those states also doesn't seem enough to help notice them.
Comment 24 Frank Yan (:fryn) 2011-03-12 23:31:18 PST
Created attachment 518998 [details] [diff] [review]
patch addendum to use favicon as drag image

Margaret, by adding this to your patch, the favicon will always be the drag image (when dragging the identity block), instead of the entire identity block or various parts thereof.

Feel free to obsolete this upon incorporation. :)
Comment 25 :Margaret Leibovic 2011-03-16 16:17:13 PDT
Created attachment 519803 [details] [diff] [review]
updated patch

I updated my patch to address comments. The one problem I'm seeing is that the popup notification panel arrow points to the wrong place in RTL mode because of the -moz-transform style on #notification-popup-box. It seems that the panel positioning is being computed before the transformation is applied, and because there is unsymmetrical padding, the panel becomes misaligned.

I feel like this is something that can be fixed in a separate bug and should not keep us from using this approach, but I could be wrong about that.
	
Dão, I don't want to ask for review until we sort out the RTL issue, but I would appreciate any comments you might have for me so far!
Comment 26 :Margaret Leibovic 2011-03-16 16:19:39 PDT
Neil, I think applying -moz-transform: scaleX(-1); to the arrow panel's parent node is causing the arrow to point to the wrong place (see above comment). Do you know if there is a way to work around this issue, or if is this a bug you could fix?
Comment 27 Dão Gottwald [:dao] 2011-03-16 16:28:20 PDT
Comment on attachment 519803 [details] [diff] [review]
updated patch

CSS transforms don't affect layout, that's expected at this stage. You shouldn't use them here anyway, as we don't want to mirror all icons for RTL.
Comment 28 :Margaret Leibovic 2011-03-16 16:41:45 PDT
(In reply to comment #27)
> CSS transforms don't affect layout, that's expected at this stage. You
> shouldn't use them here anyway, as we don't want to mirror all icons for RTL.

For the icons, Frank suggested that we could flip the icons back with another scaleX(-1). However, I don't see a solution to the layout issue if that's expected behavior. I was trying to avoid using two images, but do you think we should just do that? Or is there something else we should do?
Comment 29 Dão Gottwald [:dao] 2011-03-16 16:45:18 PDT
> I was trying to avoid using two images, but do you think we
> should just do that?

yes
Comment 30 :Margaret Leibovic 2011-03-17 16:45:25 PDT
Created attachment 520071 [details] [diff] [review]
patch using -moz-image-rect for different arrow images

I still need to get better image files from Stepehen to replace my placeholder images.

Also, we need to think of how to best fix the background color problem on Windows that was brought up in comment 7 and comment 11. Stephen said he wanted to think about if there's a better way to do it than adding a transparent gradient like I talked about in comment 13.
Comment 31 Dão Gottwald [:dao] 2011-03-17 16:50:10 PDT
(In reply to comment #30)
> Created attachment 520071 [details] [diff] [review]
> patch using -moz-image-rect for different arrow images

I don't see the point of -moz-image-rect here, these can be just two images.
Comment 32 :Margaret Leibovic 2011-03-18 14:32:18 PDT
Created attachment 520314 [details] [diff] [review]
patch with two images
Comment 33 Dão Gottwald [:dao] 2011-03-27 04:00:17 PDT
Comment on attachment 520314 [details] [diff] [review]
patch with two images

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -7800,16 +7800,17 @@ var gIdentityHandler = {
>     var htmlString = "<a href=\"" + value + "\">" + value + "</a>";
> 
>     var dt = event.dataTransfer;
>     dt.setData("text/x-moz-url", urlString);
>     dt.setData("text/uri-list", value);
>     dt.setData("text/plain", value);
>     dt.setData("text/html", htmlString);
>     dt.addElement(event.currentTarget);
>+    dt.setDragImage(gProxyFavIcon, 0, 0);

It makes no sense to call both addElement and setDragImage, and with different nodes.

I tested this patch only on Windows.

* It looks like the identity block doesn't cleanly connect with the nav bar's corners, making them look slightly washed-out.

* I agree with comment 23, the color differences are too faint to adequately communicate what the identity button was originally supposed to communicate. If we're okay with that, we might as well stop changing the colors altogether. I suspect we're not okay with it, though.

* The non-ssl styling feels a bit heaver than without this patch due to the extra border. I'm not sure that's a good tradeoff.

* The ssl styling feels in a way heaver than without this patch due to the bold text. The mockup tricked by making the text not only bold but also smaller (but too small to meet a11y requirements, imho).
Comment 34 Dão Gottwald [:dao] 2011-03-27 04:22:05 PDT
> I tested this patch only on Windows.

XP without cleartype, btw, which might be relevant for the perception of the bold text.
Comment 35 Frank Yan (:fryn) 2011-03-30 14:52:34 PDT
(In reply to comment #33)

> >     dt.addElement(event.currentTarget);
> >+    dt.setDragImage(gProxyFavIcon, 0, 0);
> 
> It makes no sense to call both addElement and setDragImage, and with different
> nodes.

Sometimes, it does make sense, since addElement establishes the node on which events will be fired, and you might want a different drag image. I wasn't sure if we were listening for any other events that might depend on event.currentTarget, but it looks like we're not, so in this case, we can just use:

> >-    dt.addElement(event.currentTarget);
> >+    dt.addElement(gProxyFavIcon);

I only took a quick glance on possible dependencies, so please test this change before committing, Margaret. :)
Comment 36 :Margaret Leibovic 2011-03-30 16:28:14 PDT
(In reply to comment #35)

> > >-    dt.addElement(event.currentTarget);
> > >+    dt.addElement(gProxyFavIcon);
> 
> I only took a quick glance on possible dependencies, so please test this change
> before committing, Margaret. :)

This looks pretty strange if you initiate the drag by clicking on the text part of the identity box, since it keeps the favicon the same distance from the cursor. I think we need to go with the first approach if we want to do this. However, if this is a contentious change, perhaps we should move it to a different bug so as not to hold up the style changes in this bug.
Comment 37 Frank Yan (:fryn) 2011-03-30 16:54:16 PDT
(In reply to comment #36)

Ugh, I see why that happens.

Let's go with:

-    dt.addElement(event.currentTarget);
+    dt.setDragImage(gProxyFavIcon, 0, 0);

I don't think we need to defer this to another bug. It's not contentious from a UI perspective (I've also gotten UX team verbal approval), and we have a solution (the above two lines).
Comment 38 :Margaret Leibovic 2011-03-30 17:50:03 PDT
Created attachment 523183 [details] [diff] [review]
patch

I increased the border-radius on Windows and Linux to improve the appearance of the corners, and Stephen came up with improved color/text styles. He also made much nicer arrow images!
Comment 39 Dão Gottwald [:dao] 2011-03-31 07:51:24 PDT
Comment on attachment 523183 [details] [diff] [review]
patch

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

> #identity-box {
>+  margin: -2px;
>+  -moz-margin-end: 0;
> }

Instead of this, can you remove the textbox's padding?

> #identity-box.verifiedDomain,
> #identity-box.verifiedIdentity {
>+  -moz-padding-end: 2px;
> }

This is wrong, try setting browser.identity.ssl_domain_display to 0. The spacing needs to be adjusted here:

> #identity-icon-labels {
>   -moz-margin-start: 1px;
>   -moz-margin-end: 3px;
>-  -moz-transform: translate(0, -1px);
> }

I still haven't tested Linux/Mac.
Comment 40 :Margaret Leibovic 2011-03-31 11:25:02 PDT
(In reply to comment #39)
> Comment on attachment 523183 [details] [diff] [review]
> patch
> 
> >--- a/browser/themes/winstripe/browser/browser.css
> >+++ b/browser/themes/winstripe/browser/browser.css
> 
> > #identity-box {
> >+  margin: -2px;
> >+  -moz-margin-end: 0;
> > }
> 
> Instead of this, can you remove the textbox's padding?

I based this off the way the combined stop/go/reload button was styled. To get rid of the padding on the textbox and keep the location bar looking right, I would need to also need to modify that button's styles.
Comment 41 :Margaret Leibovic 2011-03-31 12:59:00 PDT
Created attachment 523404 [details] [diff] [review]
patch v2

On Windows, I removed the #urlbar padding, so I could also get rid of the negative margin on #urlbar > toolbarbutton, since it's not needed anymore. Setting padding on #identity-box to maintain the height of the identity box also took care of the spacing issue from the second review comment. I also had to tweak the #notification-popup-box spacing to cope with getting rid of the #urlbar padding.

On Linux, because we don't style the textbox ourselves (bug 595973), I couldn't get rid of the padding, so I kept the negative margins there. Talking with Stephen, we decided that there should be a consistent negative margin style between the identity box and the stop/go/reload button, so we decided change the negative margin on the button to also be -1px, since that seems to create the right spacing for most themes (right now the stop/go/reload button overlaps the urlbar border on a bunch of themes).
Comment 42 Dão Gottwald [:dao] 2011-04-07 12:22:12 PDT
Comment on attachment 523404 [details] [diff] [review]
patch v2

The :-moz-focusring styling needs an update, at least on Windows and Linux. The color could probably be just black all the time...
Comment 43 :Margaret Leibovic 2011-04-07 14:55:33 PDT
Created attachment 524500 [details] [diff] [review]
patch v3

I changed the -moz-focusring style for Windows so that it is always black. Also, after talking with Stephen I decided to remove the outline-offset. It looks like I didn't need to make any changes to the Linux styles.
Comment 44 Dão Gottwald [:dao] 2011-04-07 15:27:51 PDT
Comment on attachment 524500 [details] [diff] [review]
patch v3

Removing the offset is a problem, as this paints the outline on the textbox border, where it likely won't be visible (this applies to Linux as well, since the button now touches the border).
Comment 45 :Margaret Leibovic 2011-04-08 09:35:54 PDT
Created attachment 524638 [details] [diff] [review]
patch v4

Added outline-offset to -moz-focusring style for Windows and Linux.
Comment 46 Dão Gottwald [:dao] 2011-04-08 10:36:19 PDT
Comment on attachment 524638 [details] [diff] [review]
patch v4

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -7787,17 +7787,17 @@ var gIdentityHandler = {
>     var urlString = value + "\n" + content.document.title;
>     var htmlString = "<a href=\"" + value + "\">" + value + "</a>";
> 
>     var dt = event.dataTransfer;
>     dt.setData("text/x-moz-url", urlString);
>     dt.setData("text/uri-list", value);
>     dt.setData("text/plain", value);
>     dt.setData("text/html", htmlString);
>-    dt.addElement(event.currentTarget);
>+    dt.setDragImage(gProxyFavIcon, 0, 0);

This is kind of broken, the favicon is mostly covered by the cursor over here.

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

> #identity-box:-moz-focusring {
>   outline: 1px dotted -moz-DialogText;
>+  outline-offset: -3px;
> }

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

> #identity-box:-moz-focusring {
>   outline: 1px dotted -moz-DialogText;
>   outline-offset: -3px;
> }

The background colors are hardcoded, so the outline color should be 'black'.
Comment 47 :Margaret Leibovic 2011-04-08 11:59:22 PDT
(In reply to comment #46)
> Comment on attachment 524638 [details] [diff] [review]
> patch v4
> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> >@@ -7787,17 +7787,17 @@ var gIdentityHandler = {
> >     var urlString = value + "\n" + content.document.title;
> >     var htmlString = "<a href=\"" + value + "\">" + value + "</a>";
> > 
> >     var dt = event.dataTransfer;
> >     dt.setData("text/x-moz-url", urlString);
> >     dt.setData("text/uri-list", value);
> >     dt.setData("text/plain", value);
> >     dt.setData("text/html", htmlString);
> >-    dt.addElement(event.currentTarget);
> >+    dt.setDragImage(gProxyFavIcon, 0, 0);
> 
> This is kind of broken, the favicon is mostly covered by the cursor over here.

Using dt.setDragImage(gProxyFavIcon, 10, 10); seems to do a good job placing the tip on the cursor in the middle of the icon. Does that sound good? (I only tested on Mac)
Comment 48 Dão Gottwald [:dao] 2011-04-08 16:12:58 PDT
(In reply to comment #47)
> Using dt.setDragImage(gProxyFavIcon, 10, 10); seems to do a good job placing
> the tip on the cursor in the middle of the icon. Does that sound good? (I only
> tested on Mac)

Better, but whether that thing under the mouse can be recognized as the favicon still seems to depend on the icon and cursor being used. How does 16 instead of 10 work for you?
Comment 49 Frank Yan (:fryn) 2011-04-08 20:19:01 PDT
(In reply to comment #47)

FWIW, I concur with 16, since it's the width/length of the favicon and for the same reason that Dao gave.
Comment 50 :Margaret Leibovic 2011-04-09 09:39:36 PDT
I tried using 16, but it made the icon float away from the cursor, which looked a little strange to me. However, testing with more favicons, I think 16 is better because you can definitely see the favicon more clearly.
Comment 51 Dão Gottwald [:dao] 2011-04-11 09:39:55 PDT
Comment on attachment 524638 [details] [diff] [review]
patch v4

> /* Notification icon box */
> #notification-popup-box {
>-  margin: 0 3px;
>+  -moz-margin-end: -8px;
>+  -moz-margin-start: 5px;
>+  -moz-padding-end: 10px;
>+  background-repeat: no-repeat;
>+  background-size: auto 100%;
>+  position: relative;
>+}
>+
>+#notification-popup-box:-moz-locale-dir(ltr) {
>+  background-image: url("chrome://browser/skin/urlbar-arrow.png");
>+  background-position: right;
>+}
>+
>+#notification-popup-box:-moz-locale-dir(rtl) {
>+  background-image: url("chrome://browser/skin/urlbar-arrow-rtl.png");
>+  background-position: left;
>+}

This is broken when using a lightweight theme or dark OS theme.
Comment 52 :Margaret Leibovic 2011-04-11 16:22:27 PDT
Created attachment 525208 [details] [diff] [review]
patch v5

To fix the theme issue, I decided to make the #notification-popup-box background always be white, since we're already using hard-coded colors for the identity block background. Now that the identity box touches the top and bottom borders of the urlbar, the notification popup box no longer looks like part of the same input box, so it does not necessarily need the same background color. Frank also points out that we don't invert the colors of the notification icons in different themes, so this change actually makes those more consistently visible.

To get this to work I had to change my implementation of the arrow, but it works out nicely because using ::after lets us use -moz-transform: scaleX(-1) on the pseudo-element so we only need one image.

I tested this in RTL mode on Windows/OSX/Linux and with different OS themes on Windows/Linux, so hopefully it's almost ready to go!
Comment 53 Dão Gottwald [:dao] 2011-04-13 05:12:03 PDT
Comment on attachment 525208 [details] [diff] [review]
patch v5

>+#notification-popup-box::after {
>+  content: "";
>+  display: -moz-box;
>+  -moz-margin-end: -8px;
>+  width: 10px;
>+  height: 22px;
>+  background-image: url("chrome://browser/skin/urlbar-arrow.png");
>+  background-position: right;
>+  background-repeat: no-repeat;
>+}

This doesn't stretch anymore when increasing the font size. The previous patch used background-size: auto 100% for this.
Comment 54 Frank Yan (:fryn) 2011-04-13 14:40:20 PDT
Created attachment 525823 [details] [diff] [review]
patch v6

Margaret asked me for ideas on how to fix the scaling issue, since I was the one who suggested using background-size: auto 100% and later ::after.

Since ::after doesn't seem to behave nicely with height and -moz-box-align: stretch, I just went with -moz-border-image instead.

Leaving Margaret as the assignee. :)
Comment 55 Siddhartha Dugar [:sdrocking] 2011-04-15 06:29:32 PDT
The tryserver build for Bug 455694 probably contains a patch from this bug. I'd like the identity block to be darkly colored on secure sites.
Comment 56 Dão Gottwald [:dao] 2011-04-16 07:34:37 PDT
Comment on attachment 525823 [details] [diff] [review]
patch v6

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

>+.searchbar-textbox {
>+  padding: 2px;
>+}
>+
> #urlbar,
> .searchbar-textbox {
>   -moz-appearance: none;
>   margin: 1px 3px;
>-  padding: 2px;
>   background-clip: padding-box;
>   border: 1px solid ThreeDDarkShadow;
>   border-radius: 3.5px;
> }

This lets the url and search bars consume different heights with increased font sizes. You need to either remove the padding from the search bar or undo it for the url bar.
Comment 57 :Margaret Leibovic 2011-04-19 10:37:42 PDT
Created attachment 527036 [details] [diff] [review]
patch v7

I decided to leave the padding as-is and use negative margins for the identity-box and notification-popup-box like we do on the stop/go/reload button.
Comment 58 Dão Gottwald [:dao] 2011-04-19 10:52:03 PDT
Comment on attachment 527036 [details] [diff] [review]
patch v7

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

>+#notification-popup-box:not([hidden]) + #identity-box {
>+  -moz-padding-start: 10px;

The previous patch had 6px here, please explain this change.
Comment 59 :Margaret Leibovic 2011-04-19 10:59:41 PDT
(In reply to comment #58)
> Comment on attachment 527036 [details] [diff] [review]
> patch v7
> 
> >--- a/browser/themes/gnomestripe/browser/browser.css
> >+++ b/browser/themes/gnomestripe/browser/browser.css
> 
> >+#notification-popup-box:not([hidden]) + #identity-box {
> >+  -moz-padding-start: 10px;
> 
> The previous patch had 6px here, please explain this change.

When testing, the arrow jutted right against the favicon, so I increased the padding to make it look better. I'm not sure why winstripe/gnomestripe had 6px, because pinstripe had 10px. It was likely left over from previous iterations of different approaches.
Comment 60 :Margaret Leibovic 2011-04-19 15:19:23 PDT
Thanks for all the review work, Dão!

http://hg.mozilla.org/mozilla-central/rev/aebc3d41381a
Comment 61 Chris B. 2011-04-20 11:05:22 PDT
(In reply to comment #60)
> Thanks for all the review work, Dão!
> 
> http://hg.mozilla.org/mozilla-central/rev/aebc3d41381a

Is there something not set right with this patch?

This bug landed in today's nightly (4/20/11).  HOWEVER, attachment 513594 [details] "Extended Site Identity Button" shows the text in the Identity Info box for "SSL State and Notification" to be in BOLD, but the current nightly has just regular unformatted text.
Comment 62 :Margaret Leibovic 2011-04-20 13:13:49 PDT
(In reply to comment #61)
> Is there something not set right with this patch?
> 
> This bug landed in today's nightly (4/20/11).  HOWEVER, attachment 513594 [details]
> "Extended Site Identity Button" shows the text in the Identity Info box for
> "SSL State and Notification" to be in BOLD, but the current nightly has just
> regular unformatted text.

That is expected behavior. We iterated on the design after a first pass at implementing those mock-ups.
Comment 63 David Hsu 2011-04-20 17:18:04 PDT
And the "pressed" state for indicators? Were they for another bug? Or discarded too?
Comment 64 :Margaret Leibovic 2011-04-20 17:42:27 PDT
(In reply to comment #63)
> And the "pressed" state for indicators? Were they for another bug? Or discarded
> too?

There is a pressed state. If you're not seeing that you should file a new bug :)
Comment 65 Frank Yan (:fryn) 2011-04-20 18:37:45 PDT
(In reply to comment #64)
> (In reply to comment #63)
> > And the "pressed" state for indicators? Were they for another bug? Or discarded
> > too?
> 
> There is a pressed state. If you're not seeing that you should file a new bug
> :)

I don't see it for _indicators_ either, and I don't recall there being code or any intention for it.

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