Identity portion of urlbar doesn't turn green with EV Certificates under win32 and Linux

VERIFIED FIXED in Firefox 3 beta3

Status

()

P3
normal
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: wgianopoulos, Assigned: dao)

Tracking

({platform-parity, regression})

Trunk
Firefox 3 beta3
platform-parity, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 10 obsolete attachments)

18.73 KB, image/png
Details
7.52 KB, image/png
Details
7.20 KB, image/png
Details
6.22 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
824 bytes, patch
mconnor
: review+
mconnor
: approval1.9+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
Like it or not, I think we are going to be stuck with the green bar for EV Certificates.

I think the CAs have publicized the green bar enough that it is kind of too late to do something different.

I think something like the way it displays if you add the following to userChrome.css is what is probably needed.

#identity-box.verifiedIdentity {
  -moz-appearance: none !important;
  background-color: #AFA !important;
}
(Reporter)

Comment 2

11 years ago
(In reply to comment #1)
> http://blog.johnath.com/index.php/2007/06/04/will-firefox-have-a-green-bar/
> 
Well, that notwithstanding, what prompted me to file this was Johnathan's comment in bug 404592 on 2007-11-29 which follows:

The extension is indeed out of date at this point - it was a convenient test
bed but the active code is now on trunk.  On the other hand, you absolutely
will get the green background, and the Company Name (Country) treatment for
_EV_ certificates, but neither of those for basic DV SSL.

So Bill, in your initial comments, you describe a solution that would turn the address bar green.  We're not doing that, so if that is the intent of this bug, then I can mark it WONTFIX.

In your reply to reed, you indicate that you meant to refer to the identity box, which already does turn Green, with Company Name (Country) text on EV sites.  This code exists and works (though see below) on current trunk, except that there are currently no EV roots in our store.  The instructions in bug 404592 describe how to enable the Verisign root for testing purposes, at which point sites like paypal, britishairways, ebay, and schwab should show the intended visual treatment.  If this is the intent of the report, then I will mark it WORKSFORME.

In the same bug, Kai described a platform-specific issue where his identity box was being styled grey, instead of green.  This occurs on Vista as well, and is a result of the changes made in bug 395248 which style the button as a toolbox.  We can morph this bug into one which tracks that problem, but I'd like to know which problem you're describing, first.
(Reporter)

Comment 4

11 years ago
(In reply to comment #3)
> So Bill, in your initial comments, you describe a solution that would turn the
> address bar green.  We're not doing that, so if that is the intent of this bug,
> then I can mark it WONTFIX.
> 
> In your reply to reed, you indicate that you meant to refer to the identity
> box, which already does turn Green, with Company Name (Country) text on EV
> sites.  This code exists and works (though see below) on current trunk, except
> that there are currently no EV roots in our store.

In my initial comments I suggested a css fix to turn the identity portion of the address  bar green leaving the URL portion of it yellow.  My feeling is this indicates that identity has been verified but does not turn the URL portion green as the existance of an EV cert says nothing about the safety of the site.

In current trunk builds this does not happen.  The only thing that appears green is the larry information but you have to click on something to see that.

If it is the intention that nothing is green unless you click someplace, then I guess you can wontfix, if you are actually seeing something green with out having to click anywhere then you must have some patch in your tree that is not in CVS, or changes in your userChrome.css file.
It sounds like you are describing my third scenario, where the styling from bug 395248 causes you not to see the styling.  Are you on a windows/linux platform?
(Reporter)

Comment 6

11 years ago
Yes, this is on both windows and linux.
(Reporter)

Comment 7

11 years ago
(In reply to comment #5)
> It sounds like you are describing my third scenario, where the styling from bug
> 395248 causes you not to see the styling.  Are you on a windows/linux platform?
> 

Yes that is exactly the issue.  That is why the css code I added both changed the background color and removes the styling for the case where it is an EV Cert.  I can find no simple way to make it look like a button and be green.

I left the styling for the non EV case so it is still more obvious that the icon is a clickable button.

I can make this into a patch for winstripe/gnomestripe, but I suppose you could do this too.  My personal opinion is that the color is more important than the styling here.
(Reporter)

Comment 8

11 years ago
For windows how about something like this?

#identity-box.verifiedIdentity {
  background-color: #AFA !important;
  -moz-appearance: none !important;  
}

#identity-box.verifiedIdentity:hover {
  -moz-appearance: toolbox !important;
}

This makes it green but becomes button like when you hover.
(Reporter)

Updated

11 years ago
No longer blocks: 383183
Depends on: 406612
Summary: Larry does not show a green bar for EV Certificates → Identity portion of urlbar doesn't turn green with EV Certificates under win32 and Linux
Whiteboard: WONTFIX?
(Reporter)

Comment 9

11 years ago
For Linux, which uses different styling, it appears all that is required is this:

#identity-box.verifiedIdentity {
  background-color: #AFA !important;
}

(Reporter)

Comment 10

11 years ago
Created attachment 291352 [details] [diff] [review]
patch v1

This patch implements the CSS from comment #8 and comment #9.
(Reporter)

Comment 11

11 years ago
Created attachment 291354 [details]
Linux Screenshot

Screenshots to show effect of this patch.
(Reporter)

Comment 12

11 years ago
Created attachment 291355 [details]
Windows screenshot
(Reporter)

Comment 13

11 years ago
Created attachment 291357 [details]
Windows screenshot before patch v2
(Reporter)

Comment 14

11 years ago
(In reply to comment #10)
> This patch implements the CSS from comment #8 and comment #9.
> 
I meant to mention that in looking at the pinstripe theme I noticed that the green background color was changed from #AFA to #BFA, so I echoed that change in the patch.

Sorry for all the bugspam.
(Reporter)

Comment 15

11 years ago
Created attachment 291651 [details] [diff] [review]
patch v2

This patch creates the button appearance using standard CSS elements (inset/outset borders).

This has the following advantages:

 1.  You can make the button be greeen.
 2.  The button background in the text area is a solid color making
     the text easier to read if we go to the 90%+bold font as suggested
     in bug 406612.
 3.  The look can be the same cross platform (Johnathan was keen to have this)

Disadvantage:

 1.  The button does not look as pretty.

This patch only affects Linux and Windows.  I made no atempt to modify pinstripe as I do not have a Mac.
Assignee: nobody → wgianopoulos
Attachment #291352 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #291651 - Flags: ui-review?
(Reporter)

Comment 16

11 years ago
Created attachment 291652 [details]
windows screenshot with patch v2
Attachment #291354 - Attachment is obsolete: true
Attachment #291355 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #291357 - Attachment description: Windows screenshot while hovered → Windows screenshot before patch v2
(Reporter)

Updated

11 years ago
Flags: blocking-firefox3?
(Reporter)

Updated

11 years ago
Blocks: 383183
No longer depends on: 406612
(Reporter)

Updated

11 years ago
Attachment #291651 - Flags: ui-review? → ui-review?(beltzner)
Created attachment 291659 [details]
 windows screenshot with patch v2  

Patch v2 looks bad on windows and horrible on linux as you can see. The former patch looked more natural and overall much better.
(Assignee)

Comment 18

11 years ago
Created attachment 291661 [details]
counterdraft (screenshot WinXP/Luna)

(In reply to comment #7)
> (In reply to comment #5)
> > It sounds like you are describing my third scenario, where the styling from bug
> > 395248 causes you not to see the styling.  Are you on a windows/linux platform?
> > 
> 
> Yes that is exactly the issue.  That is why the css code I added both changed
> the background color and removes the styling for the case where it is an EV
> Cert.  I can find no simple way to make it look like a button and be green.

Here it is, using |background-color: rgba(0, 255, 0, 0.25)|.
Created attachment 291662 [details]
 counterdraft / linux

For reference, the same on linux.
(Reporter)

Comment 20

11 years ago
(In reply to comment #17)
> Created an attachment (id=291659) [details]
>  windows screenshot with patch v2  
> 
> Patch v2 looks bad on windows and horrible on linux as you can see. The former
> patch looked more natural and overall much better.
> 

Well, that was my opinion as well, but the Firefox UI folk seemed kind of sold on the make it look like a button idea.
(In reply to comment #20)
> Well, that was my opinion as well, but the Firefox UI folk seemed kind of sold
> on the make it look like a button idea.

I'm sure they didn't realize it would look like that... Worst thing is that it doesn't even look like a "button" with the inset/outset stuff
If you really need to make it more obvious that you can click on it to get the identity menu, you could add a \/ arrow like search has. Then again, you could also add this to the feeds button...
(Reporter)

Comment 23

11 years ago
Created attachment 291706 [details] [diff] [review]
patch v3

Well I guess I should never assume anything.

I did not think a 1px outset border would appear clickable.  It does, and is far less ugly.
Attachment #291651 - Attachment is obsolete: true
Attachment #291706 - Flags: ui-review?(beltzner)
Attachment #291651 - Flags: ui-review?(beltzner)
(Reporter)

Comment 24

11 years ago
(In reply to comment #22)
> If you really need to make it more obvious that you can click on it to get the
> identity menu, you could add a \/ arrow like search has. Then again, you could
> also add this to the feeds button...
> 
Well, that might make sense if it produced an identity menu.  Since it does not the \/ is not the correct solution.
(Assignee)

Updated

11 years ago
Attachment #291661 - Flags: ui-review?(beltzner)
(Reporter)

Comment 25

11 years ago
Created attachment 291709 [details]
Windows screenshot w patch v3 and EV Cert
Attachment #291652 - Attachment is obsolete: true
Attachment #291659 - Attachment is obsolete: true
(Reporter)

Comment 26

11 years ago
Created attachment 291710 [details]
Windows screenshot w patch v3 and no EV Cert
This won't work for linux and mac, as the url bar is rounded here and putting a square into that looks bad. 

If you compare an actual button on the toolbar with the identity "button", there is no telling that it is supposed to be a button, even on windows, as you screenshots show.
(Assignee)

Comment 28

11 years ago
And your "button" doesn't match the search engine button.
(Assignee)

Comment 29

11 years ago
Created attachment 291722 [details] [diff] [review]
alternative approach

This patch doesn't regress the appearance implemented in bug 395248 and bug 398020. See attachment 291661 [details].
(Assignee)

Updated

11 years ago
Attachment #291661 - Attachment description: counterdraft → counterdraft (screenshot WinXP/Luna)
(Reporter)

Comment 30

11 years ago
(In reply to comment #29)
> Created an attachment (id=291722) [details]
> alternative approach
> 
> This patch doesn't regress the appearance implemented in bug 395248 and bug
> 398020. See attachment 291661 [details].
> 

I'm with you on this.  I am now running windows and Linux using your patch an like the way it looks much better.

I am just not 100% sure it resolves the issues in bug 395248, especially under Linux where, for some reason, it seems to look less like a button.

In any event, I unassigned the bug if you want to take it.
Assignee: wgianopoulos → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 31

11 years ago
(In reply to comment #30)
> I'm with you on this.  I am now running windows and Linux using your patch an
> like the way it looks much better.

> In any event, I unassigned the bug if you want to take it.

OK. In this case, I'll cancel your ui review request as well as mine, which shouldn't be required as I'm only bringing back the green without modifying the current appearance. Some sort of feedback from beltzner would be nice, but I prefer to move on with this and potentially have to change the appearance later on.

> I am just not 100% sure it resolves the issues in bug 395248, especially under
> Linux where, for some reason, it seems to look less like a button.

This could be addressed by adding a custom gradient. The same applies to Classic Windows themes.
Assignee: nobody → dao
Blocks: 395248, 406612
(Assignee)

Updated

11 years ago
Attachment #291706 - Attachment is obsolete: true
Attachment #291706 - Flags: ui-review?(beltzner)
(Assignee)

Updated

11 years ago
Attachment #291709 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #291710 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #291661 - Flags: ui-review?(beltzner)
(Assignee)

Updated

11 years ago
Attachment #291722 - Flags: review?(mconnor)
(Assignee)

Updated

11 years ago
Keywords: pp, regression

Updated

11 years ago
Attachment #291722 - Flags: review?(mconnor) → review+

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.399; previous revision: 1.398
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.144; previous revision: 1.143
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.100; previous revision: 1.99
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.140; previous revision: 1.139
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 33

11 years ago
Created attachment 292576 [details]
screenshot

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121103 Minefield/3.0b2pre ID:2007121103

not green.
see screenshot
(Assignee)

Comment 34

11 years ago
Comment on attachment 292576 [details]
screenshot

EV isn't activated yet.
Attachment #292576 - Attachment is obsolete: true
This has the effect of restoring the identity label on SSL sites, which was not intended, bug 406612 is looking at options there.  I think this is because of Dao's changes to the structure of the identity-box in browser.xul.  He fixed the various theme-based browser.css selectors, but not the one in browser/base/content/ - which is how mconnor originally hid the label.  So that selector no longer matches.

I'll attach a patch in a couple minutes that fixes that selector, which we should land immediately, imo.

Comment 36

11 years ago
Why is there now an extra hbox introduced? I think it is not needed, and just impacts Ts/Txul/Tp needlessly.
So, can the emergency patch (also) undo this hbox addition?
Created attachment 292591 [details] [diff] [review]
Re-hide larry

The main patch added an hbox in browser.xul, and caught all the selectors but this one, I think.
Attachment #292591 - Flags: review?(mconnor)
(In reply to comment #36)
> Why is there now an extra hbox introduced? I think it is not needed, and just
> impacts Ts/Txul/Tp needlessly.
> So, can the emergency patch (also) undo this hbox addition?

The emergency patch is just a question of making the patch as-designed work properly - what you are describing sounds more like changing the approach of the patch.  By all means, if there are perf regressions, re-open or file a follow-up to fix that, I support that, but I don't want to force through a structural change on a patch that's already been reviewed and approved as part of fixing an omission.  
Comment on attachment 292591 [details] [diff] [review]
Re-hide larry

r+a=mconnor
Attachment #292591 - Flags: review?(mconnor)
Attachment #292591 - Flags: review+
Attachment #292591 - Flags: approval1.9+
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.43; previous revision: 1.42
done
(Assignee)

Comment 41

11 years ago
Alfred, the inner box is there to overlaying/colorize the background (background-color:-moz-dialog, -moz-appearance:toolbox, whatever).

Johnathan, thanks for the quick fix!

Comment 42

11 years ago
But this could also be done directly on the <box id="identity-box" ?
I did this for my themes and this works perfectly.
(Assignee)

Comment 43

11 years ago
(In reply to comment #42)
> But this could also be done directly on the <box id="identity-box" ?

#identity-box can't have two background colors or the toolbox appearance and a background color.
Verified FIXED using:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre (Proto theme)
-and-

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre

Litmus testcase:

https://litmus.mozilla.org/show_test.cgi?id=5151 (we'll update it once bug 406612 lands)
Status: RESOLVED → VERIFIED
Flags: in-litmus+
You need to log in before you can comment on or make changes to this bug.