Closed Bug 406612 Opened 17 years ago Closed 17 years ago

Finalize visual presentation of identity information (Larry)

Categories

(Firefox :: Tabbed Browser, defect, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: johnath, Assigned: johnath)

References

Details

(Keywords: late-l10n)

Attachments

(2 files, 7 obsolete files)

Merge cleanly with proto
31.89 KB, patch
Gavin
: review+
johnath
: ui-review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
Updated presentation (Mac trunk)
251.08 KB, image/png
Details 
There have been several tweaks floating around for Larry presentation, some minor and uncontentious, others less so, and yet there is no bug tracking these discussions.  This bug tracks:

 - Simplify some of the popup text.  This means eliminating or rewording the supplemental text in the SSL case, since it is wordy and confusing right now.

 - Come to a decision on what to do about the url bar display on an SSL site.  On non-SSL we just have the button, on EV we have "Company Name (Country)" but on SSL we had the full domain (which was too much repetition) and then bug 402260 hid it completely, but several people (I CC'd those I could remember) are interested to know when it's coming back.  We also need it to not look like another textbox.

 - Address the issue with the winstripe theme (windows/linux) right now introduced by bug 395248.  Dao made Larry much nicer looking in that bug, much more button-ish, instead of just being a textbox, but it had the side effect that instead of turning green as it does on Mac, the Larry box always has the same coloration, and just picks up a blue border highlight on EV.  Whatever we do should be at least marginally consistent, within the constraints of each platform's look and feel.

I've attached a patch that does a first pass at addressing some of these:

 - Identity box text changed to 90%, Bold to set it off from normal text

 - Identity popup on SSL sites drops the supplemental line about "owner information may not have been verified" since it is confusing in contrast to the "Verified" language above and below.

 - On EV sites, popup is more explicit about Location *and* owner being verified, and includes both in the body text to make it clear that this is a superset of DV verification.

 - Identity box shows eTLD+1 on SSL sites by default. I recognize that this is not strictly what PKI promises (since a sub-sub-sub-domain could obtain a cert which does not give them authority over the top level domain.)  On the other hand, the domain name system, and user expectations as well, assumes that subdomains represent trust delegation, and this feels like a good compromise in terms of telling users *something* about the confirmed identity of the site they are visiting, without gobbling up location-bar real-estate. The patch makes this a tri-state pref ("browser.identity.ssl_domain_display"), with 0 hiding the domain entirely, 1 being eTLD+1, and 2 being full domain (the old Larry behaviour).  In all cases, the info popup includes the full domain.

What's still missing is a solution to the cross-platform disparity in how EV is displayed, but I'd like to get Alex/Beltzner to chime in here from the UX side of things.

Nominating blocking because even if we do none of this, we need a final decision here before ship.  I suppose maybe it could pass as just wanted-firefox3, if we think doing nothing is a reasonable approach.

(For people who want to test the EV display in conjunction with this bug, see bug 404592 comment 2)
Flags: blocking-firefox3?
Try server builds with the patch are here until early January:

https://build.mozilla.org/tryserver-builds/2007-12-03_11:05-jnightingale@mozilla.com-1196708691/

I think showing just the eTLD+1 is great.  Here are some comments on the current design:

-I personally think the favicon looks out of place when right aligned, I would keep it on the left.

-The text alignment on the dialog still feels odd, and with different sizes and bolding, the dialog as a whole feels a little chaotic.

-I don't think we should allow larry to hold a question mark, since you can't actually hold a question mark in real life.  Also, if you haven't seen the SSL version of larry, it isn't at all clear what he is supposed to be holding instead of a question mark, or even who he is (it's the passport that makes him a customs official).  I suggest we go with a ghosted passport for normal sites.

-We might want to go with a larger lock icon than 16x16 since we have room in the dialog, also we might want to group the encryption information together with the icon.

-I think we should bring larry back for malware sites, with him holding either a do not enter sign, or a stop sign.  (the stop sign is a little more pervasive, but it means stop and then go, which isn't actually what we want to convey, we want to convey "do not go", so we should probably keep the do not enter sign).

-I think it is important that the site button looks like chrome (button-ish).  To get the right colors and textures for the EV case, I think we should construct the site button out of images, similar to how we built tabs in Firefox 2 (responding to OS theme changes to match the rest of the chrome).  I'll attach a mockup showing what I mean.
Blocks: 406321
(In reply to comment #0)
> Created an attachment (id=291241) [details]
> Identity box 90%+Bold, DV displays eTLD+1 with pref, string tweaks

This patch changes only the pinstripe and winstripe themes.  Therefore, it makes the Windows and Mac behavior different from Linux.

I would think changes to gnomestripe are required.

Although actually this looks much better under Linux than it does under windows.  The 90%+Bold is almost impossible to read.
For me the 90%+Bold is perfectly readable on Windows: see
https://bugzilla.mozilla.org/attachment.cgi?id=286126 (attachment 286126 [details] of bug 395248)
(In reply to comment #4)
> For me the 90%+Bold is perfectly readable on Windows: see
> https://bugzilla.mozilla.org/attachment.cgi?id=286126 (attachment 286126 [details] of bug
> 395248)
> 
It is readable for me when in the office and docked using my 20 inch display.  It is not really readable on the laptop screen at all.  It looks blurred and you kind of have to use a bit of imagination to make it out.  I think it would be more legible if not bolded.

Just for my curiosity, could send me (or attach a screenshot on how it looks on your laptop)? My example is from my Thinkpad T41 (1400x1050) laptop, with small fonts. This will also me to determine what should be a good setting for my themes for this.
(In reply to comment #2)
> I think showing just the eTLD+1 is great.

Yeah, I actually think it's a pretty acceptable trade-off too.

> -I personally think the favicon looks out of place when right aligned, I would
> keep it on the left.

Yeah, this patch does, however the screenshot I showed you before commenting here had it on the right.  Those reading the patch might be confused by your comment here, but to be clear, this patch keeps favicon on the left. 

> -The text alignment on the dialog still feels odd, and with different sizes and
> bolding, the dialog as a whole feels a little chaotic.

That's what bug 406666 is for!  :)
 
> -I don't think we should allow larry to hold a question mark, since you can't
> actually hold a question mark in real life.  Also, if you haven't seen the SSL
> version of larry, it isn't at all clear what he is supposed to be holding
> instead of a question mark, or even who he is (it's the passport that makes him
> a customs official).  I suggest we go with a ghosted passport for normal sites.

WFM - is this part of the icon work that is happening already?

> -We might want to go with a larger lock icon than 16x16 since we have room in
> the dialog, also we might want to group the encryption information together
> with the icon.

FWIW, the lock image is actually 18x18, but I take your point.  I'm not seeing the visual grouping, but am willing to believe!

> -I think we should bring larry back for malware sites, with him holding either
> a do not enter sign, or a stop sign.  (the stop sign is a little more
> pervasive, but it means stop and then go, which isn't actually what we want to
> convey, we want to convey "do not go", so we should probably keep the do not
> enter sign).

I think this is a good idea, but might be its own bug since it will require hooks into the safebrowsing code and other bits that would potentially scope-creep this bug.
 
> -I think it is important that the site button looks like chrome (button-ish). 
> To get the right colors and textures for the EV case, I think we should
> construct the site button out of images, similar to how we built tabs in
> Firefox 2 (responding to OS theme changes to match the rest of the chrome). 
> I'll attach a mockup showing what I mean.

I agree it should look chrome-ish, as Dao has already done on windows.  bug 395248 styled it as a toolbox, but that had the side effect of hiding the colouration.  Dao mitigated this by changing the outline on verified identity sites, but we should definitely find a way to make this consistent.

(In reply to comment #3)
> (In reply to comment #0)
> > Created an attachment (id=291241) [details] [details]
> > Identity box 90%+Bold, DV displays eTLD+1 with pref, string tweaks
> 
> This patch changes only the pinstripe and winstripe themes.  Therefore, it
> makes the Windows and Mac behavior different from Linux.
> I would think changes to gnomestripe are required.

Yes, you're right of course.  I am used to assuming that gnomestripe will pull in the winstripe styles, and missed that change, I'll update it in the next round of patches.

> Although actually this looks much better under Linux than it does under
> windows.  The 90%+Bold is almost impossible to read.

It's certainly possible that we will style things differently on different platforms to maximize legibility, but this is surprising to me since other windows users seem to report it looking fine.  Maybe there's something non-standard about your environment here?  It looks fine for me on Mac and on Vista.
Attached image screenshot with 90%+bold (obsolete) — 

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot 90% no bolding (obsolete)
        — 
      

    


  

        Attachment #291567 -
        Attachment description: 90% no bolding → screenshot 90% no bolding

    
    

    
  
(In reply to comment #6)
> Just for my curiosity, could send me (or attach a screenshot on how it looks on
> your laptop)? My example is from my Thinkpad T41 (1400x1050) laptop, with small
> fonts. This will also me to determine what should be a good setting for my
> themes for this.
> 
I posted 2 screenshots one with 90% bold weight and one with 90% normal weight.

I should tell you that I am old and my eyes are not what they used to be, but to me the bolded one is harder to read.

I am not sure these will look anything like they do onmy screen on yours though.


    
    

    
  
(In reply to comment #10)
> I am not sure these will look anything like they do onmy screen on yours
> though.
> 
Well now i am sure.  I just looked at the 2 screenshots I posted to this bug on my linux laptop and the bolded one is easier to read.

This is another of those screen dependent things much like the domain name highlighting in the urlbar that eventually got backed out ... sigh


    
    

    
  
Bolding does indeed make letters harder to distinguish from one another, and the increased spoofing possibilities which come along with that is the reason we haven't used bold in the URL bar in the past. I haven't had a chance to look at all this yet, but just a quick warning - be wary of using bold for security-critical text.

Gerv

    
    

    
  
Comment on attachment 291241 [details] [diff] [review]
Identity box 90%+Bold, DV displays eTLD+1 with pref, string tweaks

>-identity.identified.title=Identity Verified
>+identity.identified.title=Location & Owner Verified

You'll need to rev the entity here.
No longer blocks: 406321

    
    

    
  
I have added a new patch to bug 406321 which uses standard CSS elements (inset/outset border) to create the button effect on gnomestripe and winstripe.  This also makes the text easier to read as it is no longer on a gradient color background.

    
  
Depends on: 406321

    
    

    
  
>no longer on a gradient color background.

I think the gradient is important in that it makes the button both look like a button, and feel like chrome.  Also, we want the site button to match the search button, so any changes we make to look and feel here we have to consider for over there.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3

    
    

    
  
(In reply to comment #15)
> I think the gradient is important in that it makes the button both look like a
> button, and feel like chrome.

Currently the thing looks like a toolbox and not like a button. A gradient may be OK, a toolbox background is not OK. You cannot be sure that it looks like a button depending on the visual style. Either use your own gradient background (which may also look ugly under some visual styles like windows classic which does not have button-gradients!) or style it using css borders and native colours.


    
    

    
  
>or style it using css borders and native colours.

I was thinking we should probably go with this approach on Windows and Linux, similar to how our tabs have a clickable affordance, are made purely out of css, and adapt to match the color scheme of every platform.

    
    

    
  
Yes this is easy for tabs, but how can you make sure the button looks even remotely nice inside of a native styled text entry like the location bar? I don't think this is possible.

Then again, what does define a "button" on GTK? It's just an icon in the toolbar in "Normal" state. So the button look (the border) would only be drawn on mouse-over? Else it would look totally out of place, if it had a look like dialog buttons (always visible border).

Also, what about "buttons" like Star/Bookmark, Feed and Go? Those also don't have a button look, and they work fine this way.

    
    

    
  
johnath: are those trial builds mentioned in comment 1 still valid, or have you made further tweaks based on the feedback in this bug?

Gerv

    
    

    
  
(In reply to comment #19)
> johnath: are those trial builds mentioned in comment 1 still valid, or have you
> made further tweaks based on the feedback in this bug?

Some things have changed as a result of other bugs being closed, actually.  bug 406666 fixed the text alignment in the popup, and bug 406321 changed the button visualization on windows/linux.  Mac's site button treatment is being tracked, along with the rest of the theme, in bug 397723, but the "Proto" testing theme has treated the favicon as a button since version 0.8.  Proto is broken when the button has a text label though, so testing this patch with current Proto is broken.

As far as the feedback that is not button-appearance-related or alignment related, this like revving the string entity, and including the changes for gnomestripe, I'll attach an updated patch shortly.  Still not sure what to do about the bolding, but maybe in the new patch, I'll style it 90%, non-bold and see how it looks.  I do want to set it off as visually distinct from the editable text in the URL bar, even above and beyond the button treatment.

    
    

    
  
> I do want to set it off as visually distinct from the editable text in
> the URL bar, even above and beyond the button treatment.

Put the text to the left of the favicon? That'd be nicer than saying the same thing (the domain) twice in a row anyway.

I'd love to see the country in there, even for DV certs (maybe especially for those).

For non-ssl sites could we use an "I don't know who this is" icon (or perhaps the greyed out word "unverified")? It seems we haven't solved the "user must notice the _lack_ of an indicator to know they're unsafe" issue which was one of the original goals.

What presentation will manually-added cert exceptions get? In FF2 and current trunk they are indistinguishable from valid certs. The only one asserting their validity is the user, I'm not sure the Mozilla browser should be giving it a presentation that indicates we're vouching for it.

    
  
Blocks: 408865

    
    

    
  
(In reply to comment #21)
> > I do want to set it off as visually distinct from the editable text in
> > the URL bar, even above and beyond the button treatment.
> 
> Put the text to the left of the favicon? That'd be nicer than saying the same
> thing (the domain) twice in a row anyway.

Putting it on the left is an easy change, and I had a try server build of it at one point, but it felt pretty odd.  I agree that repeating is also odd, but my hope is that eTLD on a button-like presentation is a lot less weird looking than the alternatives.  And this version will let people control the experience too, via the pref.

> I'd love to see the country in there, even for DV certs (maybe especially for
> those).

Yeah, I agree, but we can't really put any stock in that information, can we?  DV issuers don't make any guarantees about location verification, (for those that even include it.)

> For non-ssl sites could we use an "I don't know who this is" icon (or perhaps
> the greyed out word "unverified")? It seems we haven't solved the "user must
> notice the _lack_ of an indicator to know they're unsafe" issue which was one
> of the original goals.

The padlock claimed to tell you about safety, but gave you no way to inquire on a non-SSL site, as to whether you were safe or not.  The site icon tells you about identity, because safety isn't something we can do anyhow.  I know this is all old news, but I think it bears repeating - in every case, this tells the user what we know, and in every case, the user can interact with it to get more detail.

Having said all of that, I absolutely agree that personally, I would be as happy, maybe happier, if the site button were somehow more apparent, in all cases, as a thing-which-knows-things-and-wants-to-tell-you, but I think the UI has been around the bend enough times now that we need to take a soft touch up front with how much always-on UI we introduce.  My hope is that, being more visible, and more directly understandable than the padlock, it's presence or absence will be felt more than the padlock's was, but I agree that it's a UI we will need to continue to iterate on.

> What presentation will manually-added cert exceptions get? In FF2 and current
> trunk they are indistinguishable from valid certs. The only one asserting their
> validity is the user, I'm not sure the Mozilla browser should be giving it a
> presentation that indicates we're vouching for it.

I think you want bug 395703, and I agree.  I think it would be as simple as, in that case, saying "Verified by: You"  or, if we think that's too terse, maybe something like "Verified by: You (This site has a Security Override)" but yeah, we should fix that over in that bug.

I have a live version of this patch, but it breaks proto on mac in a pretty ugly way, so I don't want to land it until proto hits trunk, which is supposed to happen today.  Once I have it applying cleanly there, I'll get an updated version into reviews.


    
    

    
  
> I'd love to see the country in there, even for DV certs (maybe especially for
> those).

I know there are some logical reasons why the country name is important, including trademark disambiguations and legal jurisdiction.  However, even given these logical reasons, it just feels like the wrong thing to do.  Basing trust decisions on nationality feels like we are encouraging nationalism and leveraging any latent xenophobic tendencies.  One of the best aspects of the Web is that it breaks down national borders and unites the world with a common means of communication.  Injecting country names into primary UI strikes me as a humanity regression.

    
    

    
  
(In reply to comment #23)
> > I'd love to see the country in there, even for DV certs (maybe especially for
> > those).
> 
> I know there are some logical reasons why the country name is important,
> including trademark disambiguations and legal jurisdiction.  However, even
> given these logical reasons, it just feels like the wrong thing to do.  Basing
> trust decisions on nationality feels like we are encouraging nationalism and
> leveraging any latent xenophobic tendencies.  One of the best aspects of the
> Web is that it breaks down national borders and unites the world with a common
> means of communication.  Injecting country names into primary UI strikes me as
> a humanity regression.

You are almost right about encouraging nationalism, but what if I register Mozilla Foundation in my country, and get a certificate for that, then get people to install extension from my site, mozilla.XY?  As there's no international company/foundation registration rule, there's no way to make trust without mentioning the origin country, unfortunately.

    
    

    
  
>there's no international company/foundation registration rule

Yeah, as I said there are a lot of logical reasons why the country name is important.  But I personally think if you take a wider perspective outside of the specifics of registrars and certificates, it feels wrong.

    
    

    
  


  
Updated to account for several comments.  With this patch 

 - Treatment is now 90%, no bold
 - Extraneous text removed in DV case
 - Domain name added in EV case
 - String change to make EV title line up better with DV title
 - Presentation of domain information in primary chrome is pref-controlled[*]
 - Manually added security exceptions are now presented as "Verified by: You"

[*] The current domain label breaks Proto, the new theme for Mac.  The Proto themers are aware of this, but I don't want this bug to block on that, and at the same time, I don't want to break dogfooding for people by landing and introducing a floating label into their address bar.  My hope is to get this reviewed, and landed with the domain display pref set to 0 (do not display domain) and then separately toggle that pref once proto is fixed.  This also lets me land the string changes (one changed, one added) without blocking on Proto.

        Attachment #291241 -
        Attachment is obsolete: true

        Attachment #298504 -
        Flags: ui-review?(beltzner)

        Attachment #298504 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 298504 [details] [diff] [review]
Updated (neutered) patch for review

>Index: browser/themes/gnomestripe/browser/browser.css

>@@ -907,16 +908,22 @@ toolbar[iconsize="small"] #paste-button[
> }
> 
> #identity-popup-content-box > label {
>   white-space: -moz-pre-wrap;
>   margin-left: 0;
>   padding-left: 10px;
> }
> 
>+#identity-popup-content-box > label {
>+  white-space: -moz-pre-wrap;
>+  margin-left: 0;
>+  padding-left: 10px;
>+}
>+

?

    
    

    
  
(In reply to comment #27)
> (From update of attachment 298504 [details] [diff] [review])
> >Index: browser/themes/gnomestripe/browser/browser.css
> 
> >@@ -907,16 +908,22 @@ toolbar[iconsize="small"] #paste-button[
> > }
> > 
> > #identity-popup-content-box > label {
> >   white-space: -moz-pre-wrap;
> >   margin-left: 0;
> >   padding-left: 10px;
> > }
> > 
> >+#identity-popup-content-box > label {
> >+  white-space: -moz-pre-wrap;
> >+  margin-left: 0;
> >+  padding-left: 10px;
> >+}
> >+
> 
> ?

Umm.  Wow.  How did I miss that, and how did I do that twice?  I'll attach an updated patch.

    
    

    
  


  
Sorry for that paste-fail (and that if statement without a space - I know how gavin hates those)

        Attachment #298504 -
        Attachment is obsolete: true

        Attachment #298514 -
        Flags: ui-review?(beltzner)

        Attachment #298514 -
        Flags: review?(gavin.sharp)

        Attachment #298504 -
        Flags: ui-review?(beltzner)

        Attachment #298504 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 298514 [details] [diff] [review]
Cleaned up version of neutered patch

>Index: browser/base/content/browser.js

> function IdentityHandler() {

>+  this._eTLDService = Cc["@mozilla.org/network/effective-tld-service;1"].
>+                      getService(Ci.nsIEffectiveTLDService);

I'm a little bit concerned that instantiating this in the Ts/Txul path (first onSecurityChange) will have negative perf effects, but I suppose that can't be easily avoided.

>   setIdentityMessages : function(newMode) {
>     if (newMode == this.IDENTITY_MODE_DOMAIN_VERIFIED) {
>       var iData = this.getIdentityData();     
>       
>-      // It would be sort of nice to use the CN= field in the cert, since that's
>-      // typically what we want here, but thanks to x509 certs being extensible,
>-      // it's not the only place you have to check, there can be more than one domain,
>-      // et cetera, ad nauseum.  We know the cert is valid for location.host, so
>-      // let's just use that, it's what the status bar does too.

Leave the comment? Seems like it still applies.

>+      switch(gPrefService.getIntPref("browser.identity.ssl_domain_display")) {
>+        case 2 : // Show full domain
>+          icon_label = this._lastHost;
>+          break;
>+        case 1 : // Show eTLD
>+          icon_label = this._eTLDService.getBaseDomainFromHost(this._lastHost);

You need to watch out for NS_ERROR_HOST_IS_IP_ADDRESS here (e.g. for https://72.21.210.11/ , once you've added an exception and have the pref set to 1).

>+      // We need a port number for all lookups.  If one hasn't been specified, use
>+      // the https default
>+      var lookupHost = this._lastHost;
>+      if (lookupHost.indexOf(':') < 0)
>+        lookupHost += ":443";

Hrm, this string manipulation of the host seems a bit fragile. Will this work for IPv6 IP addresses that contain ":" characters, for example? Granted it's quite a corner case, but I'd feel better if there was a more robust way of ensuring that the port was present. Since _lastHost comes from nsLocation::GetHost(), which returns the URI's hostPort, this is probably sufficient for now, I guess.

>+      var overrideService = Components.classes["@mozilla.org/security/certoverride;1"]
>+                                      .getService(Components.interfaces.nsICertOverrideService);
>+
>+      if (overrideService && overrideService.hasMatchingOverride(lookupHost, iData.cert, {}, {}))

overrideService won't be null here (if it failed to initialize for some reason the getService call would throw). Might also be worth keeping a reference to this service, as you do with the eTLDService, rather than getting it each time.

>@@ -5712,31 +5742,30 @@ IdentityHandler.prototype = {

>     if (newMode == this.IDENTITY_MODE_DOMAIN_VERIFIED) {

>-      supplemental = this._stringBundle.getString("identity.domainverified.supplemental");

This means "identity.domainverified.supplemental" is unused and can be removed now, right?

>Index: browser/locales/en-US/chrome/browser/browser.properties

>+# Localization note.

Doesn't match the standard format at http://developer.mozilla.org/en/docs/Localization_notes , though I'm not sure how much that really matters.

        Attachment #298514 -
        Flags: review?(gavin.sharp)

    
    

    
  
Alex: I understand your worries about country name, but if we don't have it, it's an obvious and enormous loophole. "Paypal, Inc." of Nigeria is going to clean up in the phishing stakes.

Gerv


    
    

    
  
johnath: any chance of a screenshot or two?

Oh, one other thought:

> Injecting country names into primary UI strikes me as a humanity regression.

But they are already there, in TLDs.

Gerv


    
    

    
  
>"Paypal, Inc." of Nigeria is going to clean up in the phishing stakes.

My apologies for taking the bug off topic a little, but aren't there mechanisms in place to revoke the ssl certificate of a phishing site, and eventually the authority of registrars who give out a lot of bogus ssl certs?  Also, aren't ssl certs used phishing sites in the order of magnitude of hundreds of attacks, as opposed to hundreds of thousands of attacks.

I'm not saying the problem doesn't exist, just that these are a couple of weeds on a very large and beautiful lawn, and we should consider the different sides carefully before pulling out the powerful chemicals to fix the problem.


    
    

    
  
Alex: but at this stage, it becomes harder to determine what phishing is. Paypal, Inc. of Nigeria will have obtained their cert legally and without deception. Assuming Nigeria has no laws about appropriating the trade dress of foreign companies, their operations would be legal in their jurisdiction. "Hey, we are running a banking site too. If people turn up and try and log in with the wrong credentials, how is it our fault?".

My point is that prevention is better than cure. Better to show people they are in the wrong place before they enter their credentials than after.

Gerv


    
    

    
  
Johnath: please poke me tomorrow and let me over-the-shoulder-review this one.

    
    

    
  


  

        Attachment #298514 -
        Attachment is obsolete: true

        Attachment #299304 -
        Flags: ui-review?(beltzner)

        Attachment #299304 -
        Flags: review?(gavin.sharp)

        Attachment #298514 -
        Flags: ui-review?(beltzner)

    
    

    
  
(In reply to comment #30)
> (From update of attachment 298514 [details] [diff] [review])
> >Index: browser/base/content/browser.js
> 
> > function IdentityHandler() {
> 
> >+  this._eTLDService = Cc["@mozilla.org/network/effective-tld-service;1"].
> >+                      getService(Ci.nsIEffectiveTLDService);
> 
> I'm a little bit concerned that instantiating this in the Ts/Txul path (first
> onSecurityChange) will have negative perf effects, but I suppose that can't be
> easily avoided.

Changed to a singleton pattern - instantiated and cached on first load (which is in the SSL Tp path for the first page load, less of an issue for me than core Ts/Txul.)

> >-      // It would be sort of nice to use the CN= field in the cert, since that's
> >-      // typically what we want here, but thanks to x509 certs being extensible,
> >-      // it's not the only place you have to check, there can be more than one domain,
> >-      // et cetera, ad nauseum.  We know the cert is valid for location.host, so
> >-      // let's just use that, it's what the status bar does too.
> 
> Leave the comment? Seems like it still applies.

Left most of the comment except the status bar line, since we are diverging from the status bar depending on their pref.

> >+      switch(gPrefService.getIntPref("browser.identity.ssl_domain_display")) {
> >+        case 2 : // Show full domain
> >+          icon_label = this._lastHost;
> >+          break;
> >+        case 1 : // Show eTLD
> >+          icon_label = this._eTLDService.getBaseDomainFromHost(this._lastHost);
> 
> You need to watch out for NS_ERROR_HOST_IS_IP_ADDRESS here (e.g. for
> https://72.21.210.11/ , once you've added an exception and have the pref set to
> 1).

Added a try/catch to fail back to _lastHost anytime eTLD gets cranky.

> 
> >+      // We need a port number for all lookups.  If one hasn't been specified, use
> >+      // the https default
> >+      var lookupHost = this._lastHost;
> >+      if (lookupHost.indexOf(':') < 0)
> >+        lookupHost += ":443";
> 
> Hrm, this string manipulation of the host seems a bit fragile. Will this work
> for IPv6 IP addresses that contain ":" characters, for example? Granted it's
> quite a corner case, but I'd feel better if there was a more robust way of
> ensuring that the port was present. Since _lastHost comes from
> nsLocation::GetHost(), which returns the URI's hostPort, this is probably
> sufficient for now, I guess.

The way the override service handles ports is already broken in cases like this (it also plays with blind substringing and appending).  Filed bug 413909. 

> >+      var overrideService = Components.classes["@mozilla.org/security/certoverride;1"]
> >+                                      .getService(Components.interfaces.nsICertOverrideService);
> >+
> >+      if (overrideService && overrideService.hasMatchingOverride(lookupHost, iData.cert, {}, {}))
> 
> overrideService won't be null here (if it failed to initialize for some reason
> the getService call would throw). Might also be worth keeping a reference to
> this service, as you do with the eTLDService, rather than getting it each time.

Yeah, singleton'd this one off too.  

> This means "identity.domainverified.supplemental" is unused and can be removed
> now, right?

Right!  Removed half a dozen other ones too, with the joint beltzner/madhava over-shoulder last-minute redesign team.  (Screenshots forthcoming)

> >+# Localization note.
> 
> Doesn't match the standard format at
> http://developer.mozilla.org/en/docs/Localization_notes , though I'm not sure
> how much that really matters.

Fixed.  Ditto the new localization comment I added in this rev.

This patch LOOKS a lot different, but a lot of it is just styling.
 


    
    

    
  
Comment on attachment 299304 [details] [diff] [review]
Gavin review comments addressed, ui reorg

nice

        Attachment #299304 -
        Flags: ui-review?(beltzner) → ui-review+

    
    

    
  


  
carrying forward ui-r - one little fix here.

        Attachment #299304 -
        Attachment is obsolete: true

        Attachment #299312 -
        Flags: ui-review+

        Attachment #299312 -
        Flags: review?(gavin.sharp)

        Attachment #299304 -
        Flags: review?(gavin.sharp)

    
    

    
  
I noticed that in the mockups for bug 403155 and bug 403158, which deal with the appearance of contextual dialogs on Vista and OS X respectively (including the add bookmark dialog and Larry), they show the text link 'Tell me more about this web site...' being replaced with a 'More information' button. (The mockup for XP in bug 401357 still shows the 'Tell me more about this web site...' link.)

I think it's a great change; I find the link rather ugly. But I can see no mention of this change in the comments in this bug or in the patch. Is it still planned to happen?

    
    

    
  
Oops, that should be bug 403157 ('Style new bookmark contextual dialog on XP'), not 401357 ('Holiday file for Finland in Swedish').

    
    

    
  
(In reply to comment #40)
> I think it's a great change; I find the link rather ugly. But I can see no
> mention of this change in the comments in this bug or in the patch. Is it still
> planned to happen?

The button is styling, and can be done after string freeze. For tonight, though, I think I agree that "More information ..." reads better than "Tell me more about this website..."

Johnathan, can you make that change on checkin?

    
    

    
  
(In reply to comment #42)
> (In reply to comment #40)
> > I think it's a great change; I find the link rather ugly. But I can see no
> > mention of this change in the comments in this bug or in the patch. Is it still
> > planned to happen?
> 
> The button is styling, and can be done after string freeze. For tonight,
> though, I think I agree that "More information ..." reads better than "Tell me
> more about this website..."
> 
> Johnathan, can you make that change on checkin?

Yeah, I agree that we should change it to a button - prettier looking.  I can make the string change, yes, which reminds me, this bug will be late-l10n.

    
    

    
  
>Yeah, I agree that we should change it to a button

I've created the spin off bug 414183 for styling the site button (and search button) on XP and Vista.

    
    

    
  
Comment on attachment 299312 [details] [diff] [review]
Whoops - cache eTLDService even when we don't use it in the location bar

>Index: browser/base/content/browser.js

>     // Push the appropriate strings out to the UI
>     this._identityBox.tooltipText = tooltip;
>-    this._identityIconLabel.value = icon_label;
>+    if (icon_label.length == 0)

nit: use !icon_label.

>   setPopupMessages : function(newMode) {

>     if (newMode == this.IDENTITY_MODE_DOMAIN_VERIFIED) {
>     else if (newMode == this.IDENTITY_MODE_IDENTIFIED) {

>+      try {
>+        host = this._eTLDService.getBaseDomainFromHost(this._lastHost);
>+      } catch (e) {
>+        // Fail back to the full domain.
>+        host = this._lastHost;
>+      }

I didn't notice this when we were discussing the new design on Friday, but why are we showing the eTLD+1 in the popup now, rather than the full host? Seems like showing the full host would be more useful (and more accurate given the string we're using), especially in the case where we're showing the eTLD+1 in the button text.

>Index: browser/themes/gnomestripe/browser/browser.css
>Index: browser/themes/pinstripe/browser/browser.css
>Index: browser/themes/winstripe/browser/browser.css

>+.verifiedIdentity > #identity-popup-connectedToLabel2,
>+.verifiedDomain > #identity-popup-connectedToLabel2 {
>+    display: none;
> }

I recall you having a reason for this approach, but I can't remember what it was. Why are you using two labels with styling to hide/show them as needed, rather than a single label whose value you can set dynamically like all the others?

There are a few "if(" and "switch("s in this patch :)

        Attachment #299312 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  
>>Yeah, I agree that we should change it to a button

>I've created the spin off bug 414183 

Just realized I misread your comment (talking about the button inside of the contextual dialog as opposed to the site button itself).  I'll file some spin off bugs for styling the identity contextual dialog (similar to 403157, 403155, and 403158). 

    
    

    
  
Do we need such clutter? It uses lots of useful space for unuseful info.
Who cares about certificates? I don't know people who need such info.

    
    

    
  
(In reply to comment #47)
> Do we need such clutter? It uses lots of useful space for unuseful info.
> Who cares about certificates? I don't know people who need such info.
> 

Sadly most people don't care (yet). But I hope Firefox 3 will be able to make people a bit more aware of the risks and what can be done to be save on the web. This is *very* useful information.

    
    

    
  
View yourself some screenshots: in bug 414114.

I find this as useless clutter.

At least I want "not recommended option" to REMOVE it.

    
    

    
  
There should be a visual indication about the verification of location and owner, but
- there is no indication that the green bar is clickable
- end users usually won't know anything about "VA Software Corporation (US)" or "VeriSign, Inc." (example using https://sourceforge.net/account/login.php ), so this doesn't help an end-user at the moment except the green color that it is "somehow" safer.

Please replace with something smaller and more intuitive/descriptive (Larry icon? A castle?)

Jakub: Where is the screenshot with bugzilla and the ev cert from? Bugzilla has no EV cert? Is it a mockup?

    
    

    
  
Just had this thought: What happens when the string used in the certificate is very long? Could this cause the the URL bar to disappear completely? Should there be some limitation, maybe to 50% of the available space, and shortening of the string ending in "..."?


    
    

    
  
@arch...: in linked bug's attachments.
Those are NO MOCKUPS!

    
    

    
  
(In reply to comment #50)

> Jakub: Where is the screenshot with bugzilla and the ev cert from? Bugzilla has
> no EV cert? Is it a mockup?
> 

That is NOT an EV cert indication.  Note that it just has the hostname and not the CA and does not have a green background.  That is the way sites with non EV certs displayed with a past iteration of the code.

    
    

    
  
(In reply to comment #53)
> That is NOT an EV cert indication.  Note that it just has the hostname and not
> the CA and does not have a green background.  That is the way sites with non EV
> certs displayed with a past iteration of the code.
> 

Are you also talking about https://bugzilla.mozilla.org/attachment.cgi?id=299408 ? This shows the company's name like the SourceForge site.

    
    

    
  
Johnath/Gavin: can we get this landed today?

    
    

    
  
(In reply to comment #51)
> Just had this thought: What happens when the string used in the certificate is
> very long? Could this cause the the URL bar to disappear completely? Should
> there be some limitation, maybe to 50% of the available space, and shortening
> of the string ending in "..."?

Well, OUs are limited to 64 characters in the EV spec, fwiw. And by default, we're not showing the eTLD+1 for just SSL, so I think we're safe here.

Component: Location Bar and Autocomplete → Tabbed Browser

    
    

    
  
(In reply to comment #55)
> Johnath/Gavin: can we get this landed today?

That's my hope, but when proto landed on trunk, it only included the stretch-to-fit site button for EV, not DV.  Even if the pref is set to 0 by default, I still don't want to land something that breaks in that case, so I'm doing some png chopping right now for an updated patch.
Status: NEW → ASSIGNED

    
    

    
  


  
Apply cleanly now that proto has landed.  This means duplicating the 3-piece endcap structure used for EV, for DV.  I'll add the three graphics when landing.

Carrying ui-r forward, giving gavin another crack at the changes.

eTLD+1 in the popup was done as a way to keep the information in the popup maximally useful, to mitigate subdomain attacks (paypal.com.paypal.com.evil.com), and to keep the presentation simple and comprehensible, even if it is at the cost of strict accuracy.

        Attachment #299312 -
        Attachment is obsolete: true

        Attachment #299902 -
        Flags: ui-review+

        Attachment #299902 -
        Flags: review?(gavin.sharp)

        Attachment #299902 -
        Flags: review?(gavin.sharp) → review+

    
  
QA Contact: location.bar → tabbed.browser

    
    

    
  
Comment on attachment 299902 [details] [diff] [review]
Merge cleanly with proto

a=beltzner

        Attachment #299902 -
        Flags: approval1.9+

    
    

    
  
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.263; previous revision: 1.262
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.52; previous revision: 1.51
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.947; previous revision: 1.946
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.411; previous revision: 1.410
done
Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.95; previous revision: 1.94
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.61; previous revision: 1.60
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.160; previous revision: 1.159
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.112; previous revision: 1.111
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.68; previous revision: 1.67
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-end.png,v
done
Checking in browser/themes/pinstripe/browser/urlbar/startcap-secure-end.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-end.png,v  <--  startcap-secure-end.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-mid.png,v
done
Checking in browser/themes/pinstripe/browser/urlbar/startcap-secure-mid.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-mid.png,v  <--  startcap-secure-mid.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-start.png,v
done
Checking in browser/themes/pinstripe/browser/urlbar/startcap-secure-start.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-start.png,v  <--  startcap-secure-start.png
initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.155; previous revision: 1.154
done

Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
    

    
  


  
Here are the updated visuals on Mac (with proto-on-trunk).

Still TBD:

 - What should the default setting for the pref be, for the presentation of domain info in SSL?  This patch keeps things as-is, with the pref at 0, but 1 would certainly add discoverability and feedback, and still constrain us from the total duplication of the url bar contents.  I think this is my preference, but rather than bog this bug down, I've opened bug 414627, so if you have an opinion, please take it over there.

 - The "More Information" text link should be a button.  I feel like there's already a bug for this, maybe filed by Faaborg?

        Attachment #291565 -
        Attachment is obsolete: true

        Attachment #291567 -
        Attachment is obsolete: true

    
    

    
  
Backed out while Mossop diagnoses an 8ms linux Ts regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
>- The "More Information" text link should be a button.  I feel like there's
>already a bug for this, maybe filed by Faaborg?

Here are the theming bugs for this contextual dialog:

XP: bug 414698
Vista: bug 414700
OS X: bug 414702

(still discussing with the linux themers what they think is best)


    
    

    
  
re-landed with two fewer Linux CSS rules that were the only plausible candidates.  I'll open a follow up bug to re-evaluate/introduce those.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.263; previous revision: 1.262
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.52; previous revision: 1.51
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.947; previous revision: 1.946
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.411; previous revision: 1.410
done
Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.95; previous revision: 1.94
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.61; previous revision: 1.60
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.160; previous revision: 1.159
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.112; previous revision: 1.111
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.68; previous revision: 1.67
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-end.png,v
done
Checking in browser/themes/pinstripe/browser/urlbar/startcap-secure-end.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-end.png,v  <--  startcap-secure-end.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-mid.png,v
done
Checking in browser/themes/pinstripe/browser/urlbar/startcap-secure-mid.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-mid.png,v  <--  startcap-secure-mid.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-start.png,v
done
Checking in browser/themes/pinstripe/browser/urlbar/startcap-secure-start.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/urlbar/startcap-secure-start.png,v  <--  startcap-secure-start.png
initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.155; previous revision: 1.154
done

Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED

    
  
Blocks: 414919

    
  
Blocks: 414872
Depends on: 526924

    
    

    
  
Hello, I have another ideea...

The blue zone must remain with a decent amount of width to be observed as a Secure Site.
That is for sure...

But both the "1" and "2" values repeat what is in the URL. 1 - domain , 2. - full domain
Practically, is the CN of the certificate.

Why can't we change that to the Organization Field (O) of the Certificate ?...
This way we will have a decent width of the blue zone and not a clone of the URL.... just as in the case of EV Certs where we have the Identity Name... 

I've been discussing this with many people and many of them agree and do not consider this a security problem.
Why should the non-ev certs be denied a decent Organization name on the blue zone of Larry  instead of just copying the URL ?...

    
    

    
  
Because the O field of a non-EV certificate is entirely unverified. There's nothing stopping me from putting, say, "Paypal, Inc." in the O field of a non-EV certificate issued by an authority. Then we're relying on the user to tell the difference between blue and green and know that one means "not verified" and the other means "stringently verified," which we already know (from numerous studies of similar indicators like the padlock) that users will not intuitively understand or heed.

Better to only provide identity information when it's validated; we can validate the URL, which is why we show that.

There are newer bugs for revising the display of the DV case so there's less redundancy - see https://wiki.mozilla.org/Firefox/Features/Locationbar_Domain_Highlight for more information.

    
    

    
  
OK, I understand...

But at least, we could make value '2' default.... because, in many cases, a domain has a number of other subdomains, which do not relate especially to the main domain... 

for example: it.domain.com ... accounting.domain.com ... delegated_subdomain.domain.com ... and so on...

My ideea is to have the FULL CN on the Blue Zone.... not just the main domain... perhaps the main domain has entirely another contents as the "subdomain.domain.sufix" the user is connected to...

So if the EV cert specifies the Identity..... the non-EV cert would have to specify the FULL CN... because "subdomain.domain.sufix" is maybe OWNED by OTHER admin, not neccesarilly by the holder of "domain.sufix"...

Is it possible to set the default value to "2" ?...

    
    

    
  
(In reply to Mike Beltzner [:beltzner] from comment #66)

Mike, the Mozilla CA Certificate Inclusion Policy[1] says...

"6. We require that all CAs whose certificates are distributed with our software products:
...
  -  verify that all of the information that is included in SSL certificates remains current and correct at time intervals of thirty-nine months or less;"

So why do you claim that "the O field of a non-EV certificate is entirely unverified" ?

[1] http://www.mozilla.org/projects/security/certs/policy/InclusionPolicy.html

    
    

    
  
Sorry, I should have said stringently verified, as it is for EV certificates. There is no definition of "current and correct." So, f.e., someone could issue a DV certificate where they ask the user to ensure that the O field is "current and correct," but not then do any additional checking. As I understand things, this is the standard practice for many affiliate and low-price certificate issuers.

For example, GoDaddy's verification is that they call the phone number you provide within business hours.

(I *also* should have stated that there are many certificates that don't require any information for the O field)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: