455334 - Improve truncation and RTL display of EV cert owner strings in location bar

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement)

Description

[Kaspar]

Attachment: patch v1

From bug 429722 comment 18:

> > As proposed in bug 414114 already, wouldn't it make more sense to truncate the
> > end of the organization name instead? I'm attaching an illustration which shows
> > middle truncation on the left hand side and the proposed solution on the right
> > hand side.
> > 
> > Will provide a patch (in a separate bug) if anybody is interested.
>
> Yeah, I do think it's a separate bug, but a good idea, so as Dao says, please
> feel invited to do so.

So here's a first try - let me know what you think. For OS X, I've also adjusted the top padding, so that the EV organization name is now baseline aligned with the URL string (currently, it's shifted by 2px).

Comment 1

[Johnathan Nightingale [:johnath]]

Comment on attachment 338667 [details] [diff] [review]
patch v1

>-      if (iData.country)
>-        icon_label = this._stringBundle.getFormattedString("identity.identified.title_with_country",
>-                                                           [iData.subjectOrg, iData.country]);
>-      else
>-        icon_label = iData.subjectOrg;
>+      icon_label = iData.subjectOrg;
>+      icon_country_label = "(" + iData.country + ")";

The approach this patch takes is sort of worrisome to me, because it's generally bad l10n practice to glue multiple strings together in this way.  This one is only for the purposes of organizing discrete pieces of information, which is definitely better than approaches that glue together multiple sentence fragments, but it's still something that I think will confuse our localizers.

We've also historically been surprised by the performance impact that even minor elements can have when added to primary chrome.  Obviously the best thing to do is to understand the source of those problems, but in the meantime, this approach is something we'd want to be testing, in order to be confident it wouldn't regress performance.

Have you considered an approach that just string-truncates to a fixed (pref-controlled) max length in JS, before building the string that is set on the chrome label?  I don't particularly like string truncation in JS, but I think I like breaking-l10n less. :)

I'll CC pike here just to confirm my l10n suspicions.

Comment 2

[Kaspar]

(In reply to comment #1)
> The approach this patch takes is sort of worrisome to me, because it's
> generally bad l10n practice to glue multiple strings together in this way. 
> This one is only for the purposes of organizing discrete pieces of information,
> which is definitely better than approaches that glue together multiple sentence
> fragments, but it's still something that I think will confuse our localizers.

After having looked at

http://mxr.mozilla.org/l10n-central/search?string=title_with_country.*%3D&regexp=on&find=browser.properties&findi=&filter=&hitlimit=&tree=l10n-central

my conclusion was that the string "%S (%S)" doesn't seem to need l10n (or at least no localizer felt a need to change it), so that's why I'm adding the parentheses manually. If considered necessary, though, I can of course introduce something like "identity.identified.country=(%S)".

> Have you considered an approach that just string-truncates to a fixed
> (pref-controlled) max length in JS, before building the string that is set on
> the chrome label?  I don't particularly like string truncation in JS, but I
> think I like breaking-l10n less. :)

This would also imply dropping the max-width CSS property, right? Apart from the fact that I don't really like to reinvent the wheel of nsTextBoxFrame::CalculateTitleForWidth, does this not open up other l10n and/or bidi issues? I'm not that familiar with these things, so further advice is appreciated.

What I would actually like to see is dynamic resizing of the identity-box width, based on whether the URL string can be shown in full or not (in the first case, the identity-box could take up to 50% of the URL bar, in the latter case it would shrink down to, let's say, 25 characters or so). But I don't think that this is doable in a XUL toolbar without going into ugly contortions, however - or am I wrong?

Comment 3

[Axel Hecht [:Pike]]

I'm mostly worried about RTL and languages that might not want the ' '.

CCing a bunch of folks for input.

One thing that'd be good to know is, how internationalized can the data be? Is that real unicode? Could we have an RTL script in the EV cert owner? I expect the region code is just a region code, and as such, ASCII always?

Generally, this sounds less like an l10n problem but like an intl problem, as the internationlized data is coming from a server, and most details should likely display the same for, say, https://meine.deutsche-bank.de/, independent of whether I use a Japanese, German, or Hebrew browser to go there.

Comment 4

[Johnathan Nightingale [:johnath]]

(In reply to comment #2)
> my conclusion was that the string "%S (%S)" doesn't seem to need l10n (or at
> least no localizer felt a need to change it), so that's why I'm adding the
> parentheses manually. If considered necessary, though, I can of course
> introduce something like "identity.identified.country=(%S)".

Yeah, and if the localizers agree that this is not a significant issue (particularly if we extract out the parentheses and add an appropriate localization note to explain the relationship between the two strings) then that's fine by me, definitely.

> This would also imply dropping the max-width CSS property, right? Apart from
> the fact that I don't really like to reinvent the wheel of
> nsTextBoxFrame::CalculateTitleForWidth, does this not open up other l10n and/or
> bidi issues? I'm not that familiar with these things, so further advice is
> appreciated.

Yeah, I'm not sure the JS-truncation is particularly viable (what about scripts like Chinese where the average character width is higher or, as you mention, bidi bustage) but I was curious as to whether you'd considered and rejected it, or just not gone that route.  

> What I would actually like to see is dynamic resizing of the identity-box
> width, based on whether the URL string can be shown in full or not (in the
> first case, the identity-box could take up to 50% of the URL bar, in the latter
> case it would shrink down to, let's say, 25 characters or so). But I don't
> think that this is doable in a XUL toolbar without going into ugly
> contortions, however - or am I wrong?

I don't think it is either - indeed I don't think the seemingly-simpler approach of just making the identity box a percentage width of the overall location bar is doable in XUL right now.  I'll cc Enn to confirm that point, but I'm pretty sure we checked last time to no avail.

Comment 5

[(no longer active)]

(In reply to comment #4)
> Yeah, and if the localizers agree that this is not a significant issue
> (particularly if we extract out the parentheses and add an appropriate
> localization note to explain the relationship between the two strings) then
> that's fine by me, definitely.

I think it's a very bad idea to concatenate strings in code like this patch does, at least as far as RTL locales are concerned.  For example, if we decide to show the country code at the right side of the name, we'll need a LRM at the end of the string, which would not be possible this way.  I think it's best to keep the whole thing ("%S (%S)") as a localizable string, even if no locale decides to modify it.

> Yeah, I'm not sure the JS-truncation is particularly viable (what about scripts
> like Chinese where the average character width is higher or, as you mention,
> bidi bustage) but I was curious as to whether you'd considered and rejected it,
> or just not gone that route.  

Again, JS-truncation can prove bad if the name can contain Arabic characters which may join, for example.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> For example, if we decide
> to show the country code at the right side of the name, we'll need a LRM at the
> end of the string

That could be done by wrapping the labels and using -moz-box-direction:reverse.

Comment 7

[Kaspar]

Attachment: Screenshot of sample cert with RTL script in owner name

(In reply to comment #3)
> One thing that'd be good to know is, how internationalized can the data be? Is
> that real unicode?

It can, though the majority of the CAs probably stick to PrintableStrings right now. UTF8Strings for the organizationName are perfectly fine, however (see RFC 5280 section 4.1.2.6. for further details). 

> Could we have an RTL script in the EV cert owner?

Yes. I've configured a sample certificate at https://shibboleth.sni.velox.ch, and am attaching a screenshot for illustration purposes. Not sure if crop="end" currently does the right thing for an RTL script, however...

> I expect the region code is just a region code, and as such, ASCII always?

Yes, countryName has to be one of the ISO 3166 country codes and must be encoded as a PrintableString.

> Generally, this sounds less like an l10n problem but like an intl problem, as
> the internationlized data is coming from a server, and most details should
> likely display the same for, say, https://meine.deutsche-bank.de/, independent
> of whether I use a Japanese, German, or Hebrew browser to go there.

I tend to agree. So, for an RTL script, would we have to swap the cert owner name and the country code? Dão, how exactly would -moz-box-direction:reverse be used?

(In reply to comment #5)
> I think it's a very bad idea to concatenate strings in code like this patch
> does, at least as far as RTL locales are concerned.  For example, if we decide
> to show the country code at the right side of the name, we'll need a LRM at the
> end of the string, which would not be possible this way.  I think it's best to
> keep the whole thing ("%S (%S)") as a localizable string, even if no locale
> decides to modify it.

Note that the patch does not/no longer concatenate the cert owner and the country code - the only thing it does is add parentheses around the country code. I have no problem with adding

identity.identified.country=(%S)

to browser.properties, but am simply wondering if it's necessary to add this as a localized string (cf. http://mxr.mozilla.org/l10n-central/search?string=signed%3D&regexp=on&case=on&find=xpinstallConfirm.properties&tree=l10n-central for such a string, which is again the same for all locales).

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Dão, how exactly would -moz-box-direction:reverse be used?

You'd add this to browser/base/content/browser.css:

#identity-box[chromedir="rtl"] > hbox {
  -moz-box-direction: reverse;
}

You'd have to add another hbox if you want to exclude the favicon.

Comment 9

[Kaspar]

Thanks for that snippet, Dão. This basically leaves us with two questions, I think:

1) Does the country code (appearing in parentheses - "(US)" e.g.) need to be a localizable string ("(%S)"), or is it ok to simply add the parentheses in JS?

2) Should the cert owner name and country string be swapped for RTL locales (country code to the left of owner name)?

If one of the answers is yes, then I would submit a new patch (otherwise, the current version should be fine, AFAICT).

Who has the "final word" on these questions? And who should be the primary reviewer of the patch (either v1 or v2)?

Comment 10

[Johnathan Nightingale [:johnath]]

(In reply to comment #9)
> Thanks for that snippet, Dão. This basically leaves us with two questions, I
> think:
> 
> 1) Does the country code (appearing in parentheses - "(US)" e.g.) need to be a
> localizable string ("(%S)"), or is it ok to simply add the parentheses in JS?
> 
> 2) Should the cert owner name and country string be swapped for RTL locales
> (country code to the left of owner name)?
> 
> If one of the answers is yes, then I would submit a new patch (otherwise, the
> current version should be fine, AFAICT).
> 
> Who has the "final word" on these questions? And who should be the primary
> reviewer of the patch (either v1 or v2)?

l10n has the final word on the strings question, and for reviewing the patch itself, I would recommend gavin since he is a browser peer and has reviewed EV UI code before.

Comment 11

[Kaspar]

(In reply to comment #10)
> l10n has the final word on the strings question

Pike, can I have your "final word" on question one, then? :-)

> 1) Does the country code (appearing in parentheses - "(US)" e.g.) need to be a
> localizable string ("(%S)")

As to question 2), my suggestion would be to leave it unchanged for the time being - EV certs with RTL scripts in the owner string are probably very rarely seen these days (if at all - even ComSign e.g., "the only Israeli company authorized to issue legally binding electronic signatures approved by the law and by the Justice Ministry", according to https://www.comsign.co.il/eng/default.asp, uses the English organization name in their server certs). If considered necessary at a later time, it could still be done, but probably better in a separate bug.

Comment 12

[Kaspar]

Comment on attachment 338667 [details] [diff] [review]
patch v1

Gavin, could you please review (even if there was no reply to comment 11 so far? I still consider concatenation in JS acceptable in this particular case -

http://mxr.mozilla.org/l10n-central/search?string=ownerUnknown2&find=browser.properties&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=l10n-central

is actually another example showing that parentheses are more or less language independent.

Comment 13

[Kaspar]

(In reply to comment #10)
> l10n has the final word on the strings question, and for reviewing the patch
> itself, I would recommend gavin since he is a browser peer and has reviewed EV
> UI code before.

Is there a chance that this enhancement could be considered for Beta 2? Since no objections have been made to comment 11 so far, I still suggest going with the original patch.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #11)
> (In reply to comment #10)
> > l10n has the final word on the strings question
> 
> Pike, can I have your "final word" on question one, then? :-)

Axel, this was for you but you have probably never received this bug mail due to you are not CC'ed. Doing this now.

Comment 15

[Axel Hecht [:Pike]]

IMHO, the rtl issue is intl, not l10n. Simon, is there a way to find out if the Owner is a RTL string or not? Maybe as low key as a regexp?

Looking at the screenshot that Kaspar did (thanks for that), the cropping itself looks perfectly fine, just that the dots are on the wrong side, and I would expect the region code to be flipped, too.

I'm fine with hardcoding the ().

Regarding comment 8, I think that the ordering of region and cropped owner should depend on the owner's name RTL-ness, whereas the order of favicon and identity should depend on chromedir.

Comment 16

[Kaspar]

Attachment: patch v2, addressing questions from comment 9

(In reply to comment #15)

Thanks for your comments, Pike.

> is there a way to find out if the
> Owner is a RTL string or not? Maybe as low key as a regexp?

Patch v2 is attached. I'm proposing a simple test for RTLness of the owner string - a regex looking at the first character of that string, basically mimicking what UCS2_CHAR_IS_BIDI does (http://mxr.mozilla.org/mozilla/ident?i=UCS2_CHAR_IS_BIDI - it doesn't seem that this is exposed to JS in any way).

> Looking at the screenshot that Kaspar did (thanks for that), the cropping
> itself looks perfectly fine, just that the dots are on the wrong side, and I
> would expect the region code to be flipped, too.

Both points addressed in the new version (note that it's the country code, not the region code... we're not talking about DVDs ;-).

> I'm fine with hardcoding the ().

Ok, thanks for this "approval". Left as is.

> Regarding comment 8, I think that the ordering of region and cropped owner
> should depend on the owner's name RTL-ness, whereas the order of favicon and
> identity should depend on chromedir.

That's what I've implemented in v2. The two labels are contained in an additional hbox (id="identity-icon-labels"), whose direction is flipped when we detect an EV owner name starting with an RTL character.

Gavin, I think patch v2 is ready for your review (both issues from comment 9 have been addressed)... thanks.

Comment 17

[Kaspar]

Attachment: Screenshots of EV owner truncation with RTL script (max-width intentionally reduced to force cropping)

Comment 18

[Axel Hecht [:Pike]]

The screenshot looks great, thanks.

Looking at http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsTextBoxFrame.cpp#643, I'd rather mimic what HasRTLChars does, unless you're trying to make compromises in favour of TP. Like, I assume that mixed BIDI is *really* rare, if it happened at all. I'm not sure if you need to care about the surrogates in http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsBidiUtils.cpp#616.

Comment 19

[Simon Montagu :smontagu]

I think this should be fine as a quick and dirty method, with a follow-up bug to use the proper standards-compliant algorithm as in http://www.unicode.org/reports/tr9/#P2, which we need anyway for bug 218823.

Surrogates we certainly don't need to worry about, since RTL supplementary characters only occur in historical scripts.

Comment 20

[Kaspar]

(In reply to comment #13)
> Is there a chance that this enhancement could be considered for Beta 2?

Since the patch didn't make it into 3.1b2, I'm wondering if there's still hope for getting it into 3.1...? Regarding comment 19, I will "gladly" file a follow-up bug, but that doesn't make sense before patch v2 (attachment 344849 [details] [diff] [review]) has been reviewed, IMO.

Gavin, do you have a few cycles for the review? Or should I ask someone else? Thanks for advising.

Comment 21

[Kaspar]

Gavin: ping? Would appreciate if this bug could be resolved this month...

[Slightly adapting the summary to better reflect what patch v2 comprises.]

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 344849 [details] [diff] [review]
patch v2, addressing questions from comment 9

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>-#identity-box > hbox {
>-  max-width: 22em;
>+#identity-icon-labels {
>+  max-width: 18em;
>   min-width: 1px;
> }

Could you explain this change?

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   _cacheElements : function() {
>     delete this._identityBox;
>     delete this._identityIconLabel;
>     this._identityBox = document.getElementById("identity-box");
>     this._identityIconLabel = document.getElementById("identity-icon-label");
>+    this._identityIconCountryLabel = document.getElementById("identity-icon-country-label");

Need to delete the property as with the others, in case it's still a getter.

>   setIdentityMessages : function(newMode) {
>+    var iData, tooltip, icon_label = "", icon_country_label = "";
>     if (newMode == this.IDENTITY_MODE_DOMAIN_VERIFIED) {
>-      var iData = this.getIdentityData();     
>+      iData = this.getIdentityData();     

These changes are unnecessary and just clutter up blame, could you please avoid them?

>+      icon_label = iData.subjectOrg;
>+      icon_country_label = "(" + iData.country + ")";

This code used to handle iData.country being empty/null, but doesn't with this patch. Isn't that a problem?

>+      if (iData.subjectOrg.match(/^[\u0590-\u08ff\ufb1d-\ufdff\ufe70-\ufefc]/)) {

Should use RegExp.test() rather than String.match() here (since you don't care about the matches).

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

>-#identity-icon-label {
>+#identity-box > hbox > #identity-icon-labels > label {

Why add the extra |#identity-box > hbox > | part?

>-#identity-box.verifiedDomain > hbox > #identity-icon-label {
>+#identity-box.verifiedDomain > hbox > #identity-icon-labels > #identity-icon-label {
>   background: url("chrome://browser/skin/urlbar/startcap-secure-end.png") no-repeat center right;
>+}
>+
>+#identity-box.verifiedDomain > hbox > #identity-icon-labels > #identity-icon-country-label {
>+  display: none;
> }

Why is this extra rule needed? Won't the existing value="" selector take care of this?

>-#identity-box.verifiedIdentity > hbox > #identity-icon-label {
>+#identity-box.verifiedIdentity > hbox > #identity-icon-labels:not([dir="reverse"]) > #identity-icon-label {
>+  -moz-padding-end: 0;
>+}
>+
>+#identity-box.verifiedIdentity > hbox > #identity-icon-labels[dir="reverse"] > #identity-icon-label {
>+  -moz-padding-start: 14px;
>+  -moz-padding-end: 4px;
>   background: url("chrome://browser/skin/urlbar/startcap-verified-end.png") no-repeat center right;
>+}
>+
>+#identity-box.verifiedIdentity > hbox > #identity-icon-labels:not([dir="reverse"]) > #identity-icon-country-label {
>+  -moz-padding-start: 4px;
>+  background: url("chrome://browser/skin/urlbar/startcap-verified-end.png") no-repeat center right;
>+}
>+
>+#identity-box.verifiedIdentity > hbox > #identity-icon-labels[dir="reverse"] > #identity-icon-country-label {
>+  -moz-padding-end: 0;
> }

>-#identity-box.verifiedIdentity:hover:active > hbox > #identity-icon-label {
>+#identity-box.verifiedIdentity:hover:active > hbox > #identity-icon-labels:not([dir="reverse"]) > #identity-icon-country-label {
>+  background: url("chrome://browser/skin/urlbar/startcap-verified-end-active.png") no-repeat center right;
>+}
>+
>+#identity-box.verifiedIdentity:hover:active > hbox > #identity-icon-labels[dir="reverse"] > #identity-icon-label {
>   background: url("chrome://browser/skin/urlbar/startcap-verified-end-active.png") no-repeat center right;
> }

>-#urlbar[focused="true"] > #identity-box.verifiedIdentity > hbox > #identity-icon-label {
>+#urlbar[focused="true"] > #identity-box.verifiedIdentity > hbox > #identity-icon-labels:not([dir="reverse"]) > #identity-icon-country-label {
>+  background-image: url("chrome://browser/skin/urlbar/startcap-verified-end-focused.png");
>+}
>+
>+#urlbar[focused="true"] > #identity-box.verifiedIdentity > hbox > #identity-icon-labels[dir="reverse"] > #identity-icon-label {
>   background-image: url("chrome://browser/skin/urlbar/startcap-verified-end-focused.png");
> }

I'm having a hard time following these changes, could you briefly summarize them? Has this patch been tested on Mac?

I apologize for the delay in reviewing. r- for now, but with these comments addressed you should be good to go.

Comment 23

[Kaspar]

(In reply to comment #22)

Thanks for your review, Gavin.

> >-#identity-box > hbox {
> >-  max-width: 22em;
> >+#identity-icon-labels {
> >+  max-width: 18em;
> >   min-width: 1px;
> > }
> 
> Could you explain this change?

To exclude the favicon from being affected by a direction change, I had to put icon-label and identity-icon-label into another hbox (see also comment 8). Then, to get the truncation working properly, max-width apparently needs to be set on the immediate ancestor of the identity-icon-label (otherwise the text is just cut off at the end - no ellipsis appearing). Finally, the max-width is reduced by 4em as it now excludes the width of the favicon (#page-proxy-stack, whose width is the same for {gnome,pin,win}stripe).

> Need to delete the property as with the others, in case it's still a getter.

Ok, will do.

> These changes are unnecessary and just clutter up blame, could you please
> avoid them?

Will do.

> >+      icon_label = iData.subjectOrg;
> >+      icon_country_label = "(" + iData.country + ")";
> 
> This code used to handle iData.country being empty/null, but doesn't with this
> patch. Isn't that a problem?

For EV certs (IDENTITY_MODE_IDENTIFIED), an empty/null iData.country is actually a "can't happen", at least according to the theory (viz. the EV guidelines). Bug 466488 was recently filed because a CA managed to issue a cert which violates these requirements, nevertheless, but in the meantime the cert has disappeared (also been revoked). Johnathan's fix for bug 466488 would address the issue of an empty iData.country value in any case, so I suggest leaving it as is here.

> >+      if (iData.subjectOrg.match(/^[\u0590-\u08ff\ufb1d-\ufdff\ufe70-\ufefc]/)) {
> 
> Should use RegExp.test() rather than String.match() here (since you don't care
> about the matches).

Will do.

> >-#identity-icon-label {
> >+#identity-box > hbox > #identity-icon-labels > label {
> 
> Why add the extra |#identity-box > hbox > | part?

Right, this is unnecessary. Will remove.

> >+#identity-box.verifiedDomain > hbox > #identity-icon-labels > #identity-icon-country-label {
> >+  display: none;
> > }
> 
> Why is this extra rule needed? Won't the existing value="" selector take care
> of this?

Are you referring to this rule here?

  #identity-icon-labels > label[value=""] {
     display: none;
  }

This is only contained in gnomestripe's and winstripe's browser.css, for pinstripe we have a different rule (which does not apply to #identity-icon-country-label):

  #identity-box.verifiedDomain > hbox > #identity-icon-labels > #identity-icon-label[value=""] {
    -moz-padding-start: 3px !important;
    -moz-padding-end: 8px !important;
  }

For non-EV certs (i.e., the "verifiedDomain" class), the country label is never displayed, that's why this additional rule is needed.

> I'm having a hard time following these changes, could you briefly summarize
> them?

FWIW, I also had a hard time when adapting these rules ;-) (Why does pinstripe need to be that stylish, with these fancy schmancy rounded corners for the URL bar elements?) Well, anyway - the main point is that depending on the script direction, we have to swap the padding and/or the background image, that's why the rules become that complex.

> Has this patch been tested on Mac?

Yes, back and forth - see e.g. the screenshots in attachment 344850 [details]. Wouldn't mind if somebody else offers to test it as well, though...

> I apologize for the delay in reviewing. r- for now, but with these comments
> addressed you should be good to go.

I will produce patch v3 as soon as I have your agreement regarding the changes I suggested in this comment - thanks.

Comment 24

[Eddy Nigg (StartCom)]

Kaspar, by looking at attachment 344850 [details] it seems that you are using a localized (Hebrew) version. If this is the case, why isn't the indicator on the right side instead of left (and as shown in the screen shot)? It confuses me a bit, because Larry shows the content in Hebrew, but the icon is on the left hand side.

Comment 25

[Tomer Cohen :tomer]

(In reply to comment #24)
> It confuses me a bit,
> because Larry shows the content in Hebrew, but the icon is on the left hand
> side.
We have done that because the whole location bar is LTR for most of the time, and writing addresses starting at the right side (like how IE does) will make it messy.

Comment 26

[Eddy Nigg (StartCom)]

Yes, apparently you are right, I just checked with a Hebrew version and the icon is indeed on the left side. I could swear it was on the right...
Naturally it would start at the right side, but since most URLs are indeed Latin letters it makes some sense. However with the Larry icon, it doesn't look so good really (and confusing somehow).

Comment 27

[Johnathan Nightingale [:johnath]]

(In reply to comment #23)
> To exclude the favicon from being affected by a direction change, I had to put
> icon-label and identity-icon-label into another hbox (see also comment 8).

That will break themes then, right?  At this stage in the game, that will be something drivers will have to consider when approving the bug for 3.1 - we've been telling addon authors they are basically good to go. If there are a couple changes like this, they might land as a cluster, but if this is the only one, I fear for its survival.

> > >+      icon_label = iData.subjectOrg;
> > >+      icon_country_label = "(" + iData.country + ")";
> > 
> > This code used to handle iData.country being empty/null, but doesn't with this
> > patch. Isn't that a problem?
> 
> For EV certs (IDENTITY_MODE_IDENTIFIED), an empty/null iData.country is
> actually a "can't happen", at least according to the theory (viz. the EV
> guidelines). Bug 466488 was recently filed because a CA managed to issue a cert
> which violates these requirements, nevertheless, but in the meantime the cert
> has disappeared (also been revoked). Johnathan's fix for bug 466488 would
> address the issue of an empty iData.country value in any case, so I suggest
> leaving it as is here.

By spec, the certs should behave themselves, yeah, and bug 466488-- if it lands in Firefox 3.1-- should do a better job of failing over.  But in the case where someone broke something, the old code would have said

"OrgName"

whereas this patch would have:

"OrgName (undefined)"

With "undefined" being non-localized as well.  That feels like a regression, and I would prefer it if we kept the old failover behaviour here, just in case.

Thanks for your continued work on the patch - it will be great to see this improved.

Comment 28

[Kaspar]

Attachment: patch v3, addressing review from comment 22

(In reply to comment #27)
> (In reply to comment #23)
> > To exclude the favicon from being affected by a direction change, I had to put
> > icon-label and identity-icon-label into another hbox (see also comment 8).
> 
> That will break themes then, right?

Themes are not really my field, so I can't tell for sure. Apart from the additional hbox, we have to split the label into two in any case (identity-icon-label + identity-icon-country-label, that's how the above should really read, actually) - otherwise truncation wouldn't kick in properly.

> whereas this patch would have:
> 
> "OrgName (undefined)"
> 
> With "undefined" being non-localized as well.  That feels like a regression,
> and I would prefer it if we kept the old failover behaviour here, just in case.

I've added the check for an empty iData.country again - but strictly speaking, this should be fixed somewhere else: if an (alleged) EV cert doesn't meet mandatory requirements, then Firefox should automatically downgrade to IDENTITY_MODE_DOMAIN_VERIFIED, i.e. treat it as a "standard" SSL cert.

Gavin, the requests in your review (comment 22) should be addressed by this version of the patch. I slightly improved the code for adjusting the direction of the two labels - previously, the direction wasn't properly reset when switching from a cert with an RTL owner name back to one with an LTR owner name.

Comment 29

[Eddy Nigg (StartCom)]

(In reply to comment #28)
> I've added the check for an empty iData.country again - but strictly speaking,
> this should be fixed somewhere else: if an (alleged) EV cert doesn't meet
> mandatory requirements, then Firefox should automatically downgrade to
> IDENTITY_MODE_DOMAIN_VERIFIED, i.e. treat it as a "standard" SSL cert.

Correct. I think it's not appropriate to correct flaws of EV certs (which should never happen) and such a certificate should be downgraded.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Please file a new bug about that?

Comment 31

[Kaspar]

(In reply to comment #30)
> Please file a new bug about that?

Done - bug 470926.

For the time being, I suggest to leave the check in browser.js as is - i.e. I would appreciate your review of patch v3, Gavin. I hope my answers in comment 23 are satisfactory; if not, please let me know.

A short note for those who want to see a live site with an RTL owner string: apply the following patch (on top of v3) -

----- snip -----
diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6475,6 +6481,9 @@ var gIdentityHandler = {
     
     if (state & Components.interfaces.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL)
       this.setMode(this.IDENTITY_MODE_IDENTIFIED);
+    else if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH &&
+             location.hostname == "shibboleth.sni.velox.ch")
+      this.setMode(this.IDENTITY_MODE_IDENTIFIED);
     else if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH)
       this.setMode(this.IDENTITY_MODE_DOMAIN_VERIFIED);
     else
----- snip -----

and then visit https://shibboleth.sni.velox.ch (either add an exception for this cert or install https://sni.velox.ch/root.crt first).

To test the behavior with a "standard" (LTR) script, use https://www.hypovereinsbank.de/robots.txt e.g.

Comment 32

[Kaspar]

Gavin: re-ping? ;-) Could you have a look at patch v3? Thanks.

Comment 33

[Kaspar]

Sigh. Patch v3 has bitrotten, mostly due to the fixes in bug 414732 and bug 480953.

I'm going to attach an updated version, and would very much appreciate if it can now be landed.

Markus, since you're quite familiar with pinstripe (I assume), would you mind having a look at the changes in browser/themes/pinstripe/browser/browser.css? See also comment 23 for a short explanation why the pinstripe changes are relatively complex.

Comment 34

[Kaspar]

Attachment: patch v4, updated for trunk

Comment 35

[Dão Gottwald [:dao]]

Bug 482086 should make the pinstripe part easier.

Note that this can't land for 3.5 anymore, as it would introduce new incompatibilities with third-party themes.

Comment 36

[Dão Gottwald [:dao]]

Gavin asked me to finish the review here.

Kaspar, do you want to try the patch in bug 482086 and see if you can build this patch on top of it?

Comment 37

[Kaspar]

Attachment: patch v5 (depends on bug 482086 being resolved)

(In reply to comment #36)
> Kaspar, do you want to try the patch in bug 482086 and see if you can build
> this patch on top of it?

I already did that... but forgot to attach that version to this bug so far. So here it is - and your patch from bug 482086 makes life with pinstripe a lot easier, indeed. I also found a better solution for dealing with the margins, in the meantime (for all themes, not only pinstripe).

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 367986 [details] [diff] [review]
patch v5 (depends on bug 482086 being resolved)

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>-#identity-box > hbox {
>-  max-width: 22em;

I believe that hbox is unused now. Can you remove it?

>+#identity-icon-label {
>+  -moz-margin-end: 2px !important;
>+}

Please translate the px to em, and make the selector #identity-icon-label:not([value=""]).

>diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css

>-#identity-icon-label[value=""] {
>+#identity-icon-labels > label[value=""] {
>   display: none;
> }

That rule can be removed.

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

>-#identity-box.unknownIdentity > hbox > #identity-icon-label {
>+#identity-box.verifiedDomain > hbox > #identity-icon-labels > #identity-icon-country-label,
>+#identity-box.unknownIdentity > hbox > #identity-icon-labels {
>   display: none;
> }

Adding verifiedDomain seems wrong. In any case, just remove the whole thing?

>diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css

>-#identity-icon-label[value=""] {
>+#identity-icon-labels > label[value=""] {
>   display: none;
> }

ditto

Looks good overall. Can you explain [\u0590-\u08ff\ufb1d-\ufdff\ufe70-\ufefc]? I suppose the answer is obvious, I just don't know this Unicode stuff by heart and haven't done any research.

Comment 39

[Dão Gottwald [:dao]]

(In reply to comment #38)
> (From update of attachment 367986 [details] [diff] [review])
> >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css
> 
> >-#identity-box > hbox {
> >-  max-width: 22em;
> 
> I believe that hbox is unused now. Can you remove it?

Never mind, gnomestripe uses it at least.

Comment 40

[Kaspar]

Attachment: patch v6, addressing review in comment 38

(In reply to comment #38)

Thanks for the prompt review, Dão.

> >-#identity-box > hbox {
> >-  max-width: 22em;
> 
> I believe that hbox is unused now. Can you remove it?

No, as it's used to hold the favicon on the one hand and the EV labels on the other hand (see comment 8 for why an additional hbox was necessary).

What can be removed is min-width for the #identity-icon-labels - it works fine without.

> >+#identity-icon-label {
> >+  -moz-margin-end: 2px !important;
> >+}
> 
> Please translate the px to em, and make the selector
> #identity-icon-label:not([value=""]).

Done.

> >-#identity-icon-label[value=""] {
> >+#identity-icon-labels > label[value=""] {
> >   display: none;
> > }
> 
> That rule can be removed.

Done (for both gnomestripe and winstripe). At the same time, I'm now making sure that we're only setting margins/padding for verifiedDomain and verifiedIdentity - for the unknownIdentity case, we don't want to insert additional space between the favicon and the URL text box.

> >-#identity-box.unknownIdentity > hbox > #identity-icon-label {
> >+#identity-box.verifiedDomain > hbox > #identity-icon-labels > #identity-icon-country-label,
> >+#identity-box.unknownIdentity > hbox > #identity-icon-labels {
> >   display: none;
> > }
> 
> Adding verifiedDomain seems wrong. In any case, just remove the whole thing?

Yes, whole thing removed.

> Looks good overall. Can you explain [\u0590-\u08ff\ufb1d-\ufdff\ufe70-\ufefc]?
> I suppose the answer is obvious, I just don't know this Unicode stuff by heart
> and haven't done any research.

It's a class looking for any character in the range U+0590 to U+08FF, U+FB1D to U+FDFF and U+FE70 to U+FEFC - i.e. the Unicode code points of RTL scripts.

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #40)
> > >-#identity-box > hbox {
> > >-  max-width: 22em;
> > 
> > I believe that hbox is unused now. Can you remove it?
> 
> No, as it's used to hold the favicon on the one hand and the EV labels on the
> other hand (see comment 8 for why an additional hbox was necessary).

Yes, the additional hbox is correct. I was referring to the one that was already there. The favicon and the labels are held by #identity-box; in that sense, the old hbox becomes useless. Comment 39 remains true, though.

> It's a class looking for any character in the range U+0590 to U+08FF, U+FB1D to
> U+FDFF and U+FE70 to U+FEFC - i.e. the Unicode code points of RTL scripts.

I was more interested in where exactly you got the ranges from, but Simon already gave his OK in comment 19, so that's fine.

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 368027 [details] [diff] [review]
patch v6, addressing review in comment 38

>+#identity-box.verifiedDomain > hbox > #identity-icon-labels,
>+#identity-box.verifiedIdentity > hbox > #identity-icon-labels {
>   padding: 0 2px;

This is wrong for browser.identity.ssl_domain_display=0.

How about letting gIdentityHandler hide identity-icon-labels if neither of the labels has a value?

Comment 43

[Simon Montagu :smontagu]

(In reply to comment #41)
> > It's a class looking for any character in the range U+0590 to U+08FF, U+FB1D to
> > U+FDFF and U+FE70 to U+FEFC - i.e. the Unicode code points of RTL scripts.
> 
> I was more interested in where exactly you got the ranges from, but Simon
> already gave his OK in comment 19, so that's fine.

For the record, it's equivalent to UCS2_CHAR_IS_BIDI at http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsBidiUtils.h#366. See the comment above that for where the ranges come from.

Comment 44

[Kaspar]

Attachment: patch v7, hide/show identity-icon-labels in gIdentityHandler

(In reply to comment #42)
> (From update of attachment 368027 [details] [diff] [review])
> >+#identity-box.verifiedDomain > hbox > #identity-icon-labels,
> >+#identity-box.verifiedIdentity > hbox > #identity-icon-labels {
> >   padding: 0 2px;
> 
> This is wrong for browser.identity.ssl_domain_display=0.

Right, thanks for catching this. There's another case where patch v6 will inject unwanted whitespace - when ssl_domain_display is either 1 or 2, then we don't want the extra 0.25em with -moz-margin-end... I have added #identity-box.verifiedIdentity etc. to the selector to restrict it to EV certs.

> How about letting gIdentityHandler hide identity-icon-labels if neither of the
> labels has a value?

Seems like the easiest solution (instead of going through CSS contortions). Taking #identity-icon-label into account is actually sufficient, I think - if that label is empty, then we shouldn't display the country name either. How about patch v7?

Comment 45

[Dão Gottwald [:dao]]

Comment on attachment 368054 [details] [diff] [review]
patch v7, hide/show identity-icon-labels in gIdentityHandler

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+#identity-box.verifiedIdentity > hbox > #identity-icon-labels > #identity-icon-label:not([value=""]) {
>+  -moz-margin-end: 0.25em !important;
> }

:not([value=""]) isn't needed anymore, right?

Looks great, just want to test it with all themes before marking r+.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #43)
> For the record, it's equivalent to UCS2_CHAR_IS_BIDI at
> http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsBidiUtils.h#366.
> See the comment above that for where the ranges come from.

Wouldn't hurt to put a reference to that code in a comment (reference to a reference is somewhat convoluted, but just duplicating the reference means they could potentially get out of sync).

Comment 47

[Kaspar]

(In reply to comment #45)
> >+#identity-box.verifiedIdentity > hbox > #identity-icon-labels > #identity-icon-label:not([value=""]) {
> >+  -moz-margin-end: 0.25em !important;
> > }
> 
> :not([value=""]) isn't needed anymore, right?

Correct, this can be removed. I can also add a comment referring to nsBidiUtils.h/UCS2_CHAR_IS_BIDI (as suggested by Gavin in comment 46) and make that v8. Is there anything else which should go into the next (and hopefully final) version of the patch?

Comment 48

[Dão Gottwald [:dao]]

I think you should to stick with crop="center" for non-EV.

(In reply to comment #7)
> I've configured a sample certificate at https://shibboleth.sni.velox.ch,

Could you do that again so that I can test RTL?

Comment 49

[Kaspar]

(In reply to comment #48)
> I think you should to stick with crop="center" for non-EV.

No strong feelings about that, but it isn't a big deal to change it either - here's a short incremental patch, do you want to give it a try?

diff -u b/browser/base/content/browser.js b/browser/base/content/browser.js
--- b/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6605,6 +6611,7 @@
     this._identityIconLabel.value = icon_label;
     this._identityIconCountryLabel.value = icon_country_label;
     this._identityIconLabel.parentNode.hidden = icon_label ? false : true;
+    this._identityIconLabel.crop = icon_country_label ? "end" : "center";
     this._identityIconLabel.parentNode.dir = icon_labels_dir == "rtl" ?
                                              "reverse" : "";
     // also adjust the CSS direction property (needed for proper cropping)
diff -u b/browser/base/content/browser.xul b/browser/base/content/browser.xul
--- b/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -387,7 +387,7 @@
                        onerror="this.removeAttribute('src');"/>
               </stack>
               <hbox id="identity-icon-labels">
-                <label id="identity-icon-label" class="plain" crop="end" flex="1"/>
+                <label id="identity-icon-label" class="plain" flex="1"/>
                 <label id="identity-icon-country-label" class="plain"/>
               </hbox>
             </hbox>

> (In reply to comment #7)
> > I've configured a sample certificate at https://shibboleth.sni.velox.ch,
> 
> Could you do that again so that I can test RTL?

It's still there (though somewhat hidden), see also comment 31. To test middle-truncation for non-EV certs (with the incremental patch applied), set ssl_domain_display to 2 and use an arbitrary, but very long host name in the sni.velox.ch subdomain (e.g. https://averylonghostnamewhichgetsmiddletruncated.sni.velox.ch/).

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 368054 [details] [diff] [review]
patch v7, hide/show identity-icon-labels in gIdentityHandler

>-        icon_label = this._stringBundle.getFormattedString("identity.identified.title_with_country",
>-                                                           [iData.subjectOrg, iData.country]);

Also need to remove identity.identified.title_with_country from browser.properties.

>+    this._identityIconLabel.parentNode.dir = icon_labels_dir == "rtl" ?
>+                                             "reverse" : "";

This should be style.direction="ltr"/"rtl" instead of dir=""/"reverse". As far as I can see, this won't affect #identity-icon-country-label, but to be on the safe side, you could add #identity-icon-country-label { direction: ltr; } to browser/base/content/browser.css.

>+    // also adjust the CSS direction property (needed for proper cropping)
>+    this._identityIconLabel.style.direction = icon_labels_dir;

And this can be removed then, as the direction is inherited.

With the changes from comment 45, comment 46 and comment 49, this should be good to go.

Comment 51

[Kaspar]

Attachment: patch v8, with changes from comments 45, 46, 49, and 50

(In reply to comment #50)
> With the changes from comment 45, comment 46 and comment 49, this should be
> good to go.

Thanks for the additional comments - sticking to style.direction is indeed a more elegant solution. I have made the requested changes and hope that v8 can now be declared final (and land after bug 482086 has been resolved).

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 368542 [details] [diff] [review]
patch v8, with changes from comments 45, 46, 49, and 50

Thanks. I'll land this when bug 482086 is reviewed -- not sure how long that will take.

Comment 53

[Kaspar]

(In reply to comment #52)
> Thanks. I'll land this when bug 482086 is reviewed -- not sure how long that
> will take.

Dão, now that attachment 374826 [details] [diff] [review] got r+, would it be possible to land both patches on mozilla-central in the next few days?

Comment 54

[Dão Gottwald [:dao]]

I would like to land bug 482086 together with bug 482105.

Comment 55

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5d2cfb8fa4b5

Comment 56

[Dão Gottwald [:dao]]

backed out because of performance issues with bug 482086

Comment 57

[Kaspar]

Attachment: patch v9 (updated to trunk)

Updated to trunk - hopefully it can definitely land this time ;-)

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 428145 [details] [diff] [review]
patch v9 (updated to trunk)

>+              <hbox id="identity-icon-labels">
>+                <label id="identity-icon-label" class="plain" flex="1"/>
>+                <label id="identity-icon-country-label" class="plain"/>
>+              </hbox>
>               <label id="identity-icon-label" crop="center" flex="1"/>

bad merge

Comment 59

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/83a362a4ffcd

Comment 60

[Kaspar]

(In reply to comment #58)
> bad merge

Indeed, thanks for catching this. There's another tiny change betwen patch v8 and v9 for browser/base/content/browser.css which was left out, however:

>+#identity-box.verifiedIdentity > #identity-box-inner > #identity-icon-labels > #identity-icon-label {

The hbox around identity-icon-labels now also has an id, so the CSS selector can be made more specific if desired (not sure whether you consider it worth an additional commit, though).

Comment 61

[Dão Gottwald [:dao]]

changed the selector:
http://hg.mozilla.org/mozilla-central/rev/303a592b2939

Comment 62

[Pardal Freudenthal (:ShareBird)]

+#identity-icon-labels {
+  max-width: 18em;
+}

+#identity-box.verifiedIdentity > #identity-box-inner > #identity-icon-labels > #identity-icon-label {
+  -moz-margin-end: 0.25em !important;
 }

Please don't put these kind of rules on content. Rules like these should live on the skin package.

Comment 63

[Dão Gottwald [:dao]]

This is intentionally in content.

Comment 64

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #63)
> This is intentionally in content.
Why? 
AFAICS this will apply to every third-party theme, making them to explicitly have to write rules to override rules from content, if they are not desired. The important flags make it even more difficult.

Comment 65

[Dão Gottwald [:dao]]

It's behavior, not styling, and it's the right behavior regardless of the theme. If however you think it's the wrong behavior, please file a bug on that.