Closed Bug 1207619 Opened 9 years ago Closed 8 years ago

[Control Center] Information about the website certificate are truncated and incomplete by default

Categories

(Firefox :: Security, defect, P1)

41 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- verified

People

(Reporter: LpSolit, Assigned: jkt)

References

()

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files, 2 obsolete files)

Attached image screenshot 1
In Firefox 41.0, when you click on the green bar besides the address bar to get details about the website certificate, you get nothing useful. It repeats exactly what is already displayed in the green bar, i.e. the name of the website. If the name is truncated in the green bar, it's truncated too in the new panel, see screenshot 1. Moreover, the panel for BMO is truncated, and the right arrow is not visible, see screenshot 2.

The previous UI was more helpful, and didn't require an extra click to know the location of the website and who signed the certificate.
Attached image screenshot 2 (obsolete) —
Thanks for already filling this.

I do not see the point for summarising the panel. For an advanced user it will be annoying, as he will *always* want the extra details. And I'm not sure that panel simply saying "Secure connection" is really useful for a novice.
The UI itself is confusing, since there is zero additional information in the first panel (actually less, since the url bar has the country code, which isn't there), and
a) you have a > at the right to see who signed the certificate and the extra location information and
b) a “More information” link that tells you the cipher and allows accessing the certificate details (in addition of other page info)

Also, you can't directly go from a to b or b to a (in case you wanted to view everything.

I guess there must be a bug discussing to perform this change. but was unable to find it (I couldn't even find an entry for this in the changelog). Maybe it was bug 1168421, alhough the provided link is behind a passwall. However, (a) there's a difference between uncluttered and redundant and (b) goes counter to the bug title of the savvy user wanting to get more information about the connection.

I recommend to simply put the subpanel in the main control center space, as it's general content suitable for everybody (non-technical) that is useful in every case.

Since bug 1170759 suggests there will be more subpanels, maybe a more suited design would be to automatically show the first panel if there are no subpanels, or even a way to remember the panels that were expanded in previous accesses and persist them.
Summary: Information about the website certificate are truncated and incomplete by default → [Control Center] Information about the website certificate are truncated and incomplete by default
Frédéric, can you please try with a recent Nightly version, and see if some of these issues have been fixed already? If some of them remain, an updated screenshot with Nightly would be useful.
Whiteboard: [fxprivacy][triage]
The second screenshot looks like bug 1206916, which is fixed in 42+
See Also: → 1214082, 1206916
(In reply to :Paolo Amadini from comment #3)
> Frédéric, can you please try with a recent Nightly version, and see if some
> of these issues have been fixed already? If some of them remain, an updated
> screenshot with Nightly would be useful.
Flags: needinfo?(LpSolit)
I can still reproduce screenshot 1 with Nightly 44.0a1 (both en and fr locales). Screenshot 2 is fixed.
Flags: needinfo?(LpSolit)
Attachment #8664868 - Attachment is obsolete: true
One other thing is the if you hover the URL bar text, it says "Verified By: QuoVadis Limited" and doesn't mention the owner in the tooltiptext.  So maybe the owner could be added there as well.
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Depends on: 1217442
Blocks: 1216897
Assignee: nobody → jkingston
Attached patch bug-1207619 (obsolete) — Splinter Review
Removed cropping code as no longer needed. Removed broadcasting code as textContent does not get observed and is needing to wrap.
Attachment #8737565 - Flags: review?(dtownsend)
Attachment #8737565 - Attachment is patch: true
Attachment #8737565 - Flags: review?(dtownsend) → review?(mak77)
Comment on attachment 8737565 [details] [diff] [review]
bug-1207619

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

Gijs offered to help my reviews load :)
Attachment #8737565 - Flags: review?(mak77) → review?(gijskruitbosch+bugs)
Comment on attachment 8737565 [details] [diff] [review]
bug-1207619

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

This stops cropping data URLs which get rendered verbatim. So they are going to make the panel really really really really really long. E.g. open:

http://software.hixie.ch/utilities/cgi/data/data

and submit the form, then open the identity popup. IMO we should continue to crop data: URIs the way we do now.

The design comments in https://bugzilla.mozilla.org/show_bug.cgi?id=1217442 also said that we should continue to crop in the main view. I disagree with that (the rationale was 'the whole thing is in the location bar', but that is not actually the case here), and so does this patch, but I was wondering if that had been explicitly discussed.

So IMO we should keep the crop style (text-overflow: ellipsis should work, if we're switching to CSS anyway) at least for data URIs, potentially for DV certs, though I wouldn't mind those wrapping as much - they're less likely to be ginormous, and use word-wrap for EV certs.

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +178,5 @@
>  
>  .identity-popup-headline {
>    margin: 3px 0 4px;
>    font-size: 150%;
> +  word-break: break-all;

This wraps mid-word for EV cert names, which seems wrong. "Genossenschaft" is one word in German.

Is there a reason not to use:

word-wrap: break-word;

which seems to behave better?
Attachment #8737565 - Flags: review?(gijskruitbosch+bugs) → review-
Hey thanks for the review, certainly `break-word would be better for EV certs the issue was more with DV wrapping. (http://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/)

I'll change the PR to:
- Truncate data urls (ideally truncating from the opposite side from the data: scheme as seeing '...%OA%3c%2Fhtml%3d%OD%OA' is really not explaining well)
- `word-break: break-all;` when there isn't any identity information
- `word-break: break-word; overflow: hidden; text-overflow: ellipsis;` for all other cases.
(In reply to Jonathan Kingston [:kingstonTime] from comment #12)
> Hey thanks for the review, certainly `break-word would be better for EV
> certs the issue was more with DV wrapping.
> (http://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.
> badssl.com/)

AIUI word-wrap: break-word should force breaks into the words (for some meaning of 'word') when no between-word wrapping point can be found. As in, I would expect it to deal with this case. Doesn't it?
It didn't in my tests but will check again :), thanks.
Yeah: https://bugzilla.mozilla.org/show_bug.cgi?id=520617 is still an issue preventing break-word to do what I want.
(In reply to Jonathan Kingston [:kingstonTime] from comment #15)
> Yeah: https://bugzilla.mozilla.org/show_bug.cgi?id=520617 is still an issue
> preventing break-word to do what I want.

Hrmpf. I bet this would be fixed if we fixed the whole nsTextBoxFrame vs nsTextFrame thing (cf. https://bugzilla.mozilla.org/show_bug.cgi?id=1222096#c5 , bug 837765, ...).

Anyway, yeah - that means you're stuck with either this or adding an html kid to the label and using that. Both pretty yucky.
(and sorry for not realizing that was broken in XUL before when doing the review! I just checked on jsbin for HTML...)
Don't forget about file: URIs too. Ideally we should avoid a scheme check (which tends to become stale or miss schemes) and do like the old code which checked for a lack of `host` to handle differently.
The code I have been working on keeps the lack of host check in there and changes between single line output with truncation for anything without a host to multiline output for anything with.

I had it working using a label with two nested input elements however I wanted to make the code a little less ugly.

Thanks both.
Attachment #8742289 - Flags: review?(gijskruitbosch+bugs)
https://reviewboard.mozilla.org/r/47097/#review43927

::: browser/base/content/browser.js:6954
(Diff revision 1)
> -    // |textContent| would simply wrap the value.
> -    this._identityPopupContentHost.setAttribute("crop", crop);
> -    this._identityPopupContentHost.setAttribute("value", host);
> +      el.textContent = host;
> +      el.classList.toggle("show", !hostless);
> +    });
> +    this._identityPopupContentHostless.forEach((el) => {
> +      //el.setAttribute("crop", crop);
> +      el.setAttribute("value", host);
> +      el.classList.toggle("show", hostless);

It seems like you should use the @hidden attribute instead of CSS for this: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden

Otherwise CSS related to functionality and less about presentation need to go in content, not skin stylesheets

::: browser/base/content/browser.js:6958
(Diff revision 1)
> -    // host as it's a <label> that will be cropped if too long. Using
> -    // |textContent| would simply wrap the value.
> -    this._identityPopupContentHost.setAttribute("crop", crop);
> -    this._identityPopupContentHost.setAttribute("value", host);
> +    this._identityPopupContentHosts.forEach((el) => {
> +      el.textContent = host;
> +      el.classList.toggle("show", !hostless);
> +    });
> +    this._identityPopupContentHostless.forEach((el) => {
> +      //el.setAttribute("crop", crop);

Commented code

::: browser/components/controlcenter/content/panel.inc.xul:25
(Diff revision 1)
>  
>        <!-- Security Section -->
>        <hbox id="identity-popup-security" class="identity-popup-section">
>          <vbox id="identity-popup-security-content" flex="1">
> -          <label observes="identity-popup-content-host"/>
> +          <label class="plain">
> +            <label class="identity-popup-headline host" ></label>

Nit: extra whitespace at the end of the <label>

::: browser/themes/shared/controlcenter/panel.inc.css:193
(Diff revision 1)
> +.identity-popup-headline.show {
> +  display: inherit;
> +}
> +
> +.identity-popup-headline.host {
> +  word-wrap: break-word;

This should ideally also be in a content stylesheet but I guess that would mean browser.css since I don't see one for control center. Perhaps control center is already not using best practices for theming.
Comment on attachment 8742289 [details]
MozReview Request: Bug 1207619 - Make control center hostname wrap on long cert names rather than truncating

https://reviewboard.mozilla.org/r/47097/#review43949

I think Matt got to this one before me, so clearing r? for now. :-)
Attachment #8742289 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8742289 [details]
MozReview Request: Bug 1207619 - Make control center hostname wrap on long cert names rather than truncating

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47097/diff/1-2/
Attachment #8737565 - Attachment is obsolete: true
Comment on attachment 8742289 [details]
MozReview Request: Bug 1207619 - Make control center hostname wrap on long cert names rather than truncating

I'm guessing you meant to flag someone…
Attachment #8742289 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8742289 [details]
MozReview Request: Bug 1207619 - Make control center hostname wrap on long cert names rather than truncating

https://reviewboard.mozilla.org/r/47097/#review44433

r=me with the caveat below - note that I've not tested my suggestion, so it'd be good if you doublechecked on OS X and Windows that it worked correctly. :-)

::: browser/themes/shared/controlcenter/panel.inc.css:186
(Diff revision 2)
>    font-size: 150%;
>  }
>  
> +.identity-popup-headline.host {
> +  word-wrap: break-word;
> +  max-width: 15em;

This doesn't quite work on Windows, and will work even less well on Linux, unfortunately. The expander button gets cut off when present (e.g. on https://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/ )

The panel-mainView has a max-width and min-width of 30em. By default on Windows that is 360px. The expander is 38px. The horizontal padding on `#identity-popup-security-content` is 3em + 24px, which then boils down to 3 * 12 + 24 = 60px. So we're down to 360 - 60 - 38 = 262px of space.

Unfortunately 15em *with a font-size of 150%* boils down to 15 * (1.5 * 12) = 270px, which is 8px too many, so stuff gets cut off.

The math here will vary depending on what the OS's font-size is (I expect it's 11px on OS X and so it might be working there - but it'll be > 12px by default on Linux, so it won't work there...).

It would be nice if we could figure out a way to make this suck less... While we can use variables, those with 'em's get reinterpreted based on the font-size of the element where they're applied ( http://jsbin.com/pajujidaqe/edit?html,css,output ) . Once you then mix ems and px values in a calc() expression I don't know how to make it work cleanly and clearly.

So the best I can suggest is to do the math here directly, which is probably something like:

```
max-width: calc(20em /* 30em premultiplied for font-size changes */ - 2em /* 3em premultiplied for font-size changes */ - 24px - 38px);
```

You can still use a CSS variable for the expander's width (38px), but for the padding on the content container, please just add a comment into the existing selector there that points out that the label width depends on these values and should be updated if they change.
Attachment #8742289 - Flags: review?(gijskruitbosch+bugs) → review+
Hm, though, one way that the variables route could simplify this is if we could substitute 'rem' for 'em'... which could be something to try?
Ah I misunderstood your explanation, yeah rem solves the precalculation etc.

Still lost why I can't contain the element into a fixed width container etc.

Thanks again!
(In reply to Jonathan Kingston [:kingstonTime] from comment #27)
> Ah I misunderstood your explanation, yeah rem solves the precalculation etc.
> 
> Still lost why I can't contain the element into a fixed width container etc.

Yeah, I tried. "XUL", or something. :-\
Comment on attachment 8742289 [details]
MozReview Request: Bug 1207619 - Make control center hostname wrap on long cert names rather than truncating

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47097/diff/2-3/
Latest push should fix your latest review Gijs, still feels wrong that I can't reference the variables or contain it in the size etc.
https://reviewboard.mozilla.org/r/47097/#review44493

::: browser/themes/shared/controlcenter/panel.inc.css:72
(Diff revision 3)
>  #identity-popup > .panel-arrowcontainer > .panel-arrowcontent {
>    padding: 0;
>  }
>  
>  .panel-mainview[panelid=identity-popup] {
> +  --identity-popup-expander: 38px;

You have this in browser.css with "-width" suffixed. You probably want to fix this one to have "-width" and then get rid of the browser.css one?
Comment on attachment 8742289 [details]
MozReview Request: Bug 1207619 - Make control center hostname wrap on long cert names rather than truncating

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47097/diff/3-4/
Sorry I actually meant to remove that instance as there isn't any vars defined at selector level in the file so made more sense to be in browser.css. Amended commit.
(In reply to Jonathan Kingston [:kingstonTime] from comment #33)
> Sorry I actually meant to remove that instance as there isn't any vars
> defined at selector level in the file so made more sense to be in
> browser.css. Amended commit.

This wfm, or the inverse. Because of the theme/content confusion here it's not clear where this belongs. I guess it would be good to have a follow-up bug to address the theme/content distinction for control center.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12a27b31a15f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Iteration: --- → 48.3 - Apr 25
Flags: qe-verify?
Priority: P3 → P1
Hey Henrik,

I wasn't aware of the failure sorry.

The patch removes the element in the test, would it be easier if I rewrote it to check for the two new elements instead?

I pinged you on irc :).
Thanks
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
Verified fixed FX 48b1 Win 7, Ubuntu 14.04, OS X 10.10.5.
Status: RESOLVED → VERIFIED
Depends on: 1332267
Depends on: 1332596
You need to log in before you can comment on or make changes to this bug.