Closed Bug 1146832 Opened 9 years ago Closed 9 years ago

Ugly wrapping of label for submission status of certificate pinning errors in localizations with texts longer than English

Categories

(Firefox :: Security, defect)

x86
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox37 - wontfix
firefox38 + wontfix
firefox39 + fixed
firefox40 --- fixed

People

(Reporter: aryx, Assigned: mgoodwin)

References

Details

Attachments

(4 files, 5 obsolete files)

Attached image screenshot of issue
Firefox 37.0 Beta, Build ID 20150319212106, on Windows 8.1

There is an ugly wrapping of the label with the submission status for a certificate pinning error in localizations with texts longer than English.

Steps to reproduce:
1. Use a localized build, e.g. a German one (language code 'de'). Get it from https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/
2. Open https://pinningtest.appspot.com/
3. Report that certificate error.

Actual result:
After hitting the button for submitting the error, the localized labels for "Sending report" (German: "Bericht wird gesendet…") and "Report sent" (German: "Bericht gesendet") get wrapped with the text starting on the right and ending on the left.

Fort the "Report sent" label, many more languages should be affected according to http://transvision.mozfr.org/string/?entity=browser/chrome/overrides/netError.dtd:errorReporting.sent&repo=beta

L10n list for the 'sending' string: http://transvision.mozfr.org/string/?entity=browser/chrome/overrides/netError.dtd:errorReporting.sending&repo=beta
[Tracking Requested - why for this release]:
This looks dumb (on l10n builds) and we should fix that.

Monica, can you nominate someone to look at this? I'm not sure who else has done work with the pinning/TP UI here...
Flags: needinfo?(mmc)
I think that mgoodwin knows the most about the SSL error reporting page. I also heard from psackl that this page may be restyled?
Flags: needinfo?(mmc) → needinfo?(mgoodwin)
This is ugly but given that we're building the 37 RC today and this is not an issue that I expect the majority of the release population to encounter, I don't think that we should block on it. If there is a trivial fix I'm happy to consider it if we need an RC2.

I'm tracking for 38 as I don't want to release as second version with this issue.
I'll take a look at this
Assignee: nobody → mgoodwin
Flags: needinfo?(mgoodwin)
Attached patch Bug1146832.patch (obsolete) — Splinter Review
Changed so that wrapping is less likely - and looks better when it happens.

I'm not sure who would be a good reviewer for this. Any suggestions?
Flags: needinfo?(philipp)
Hey Mark, could you upload a screenshot of the patch showing how the text breaks now?
Thanks!
Flags: needinfo?(philipp)
Attached image all on same line (obsolete) —
Flags: needinfo?(mgoodwin)
Attached image forced down, no wrap (obsolete) —
Attached image with wrap (obsolete) —
Comment on attachment 8591751 [details] [diff] [review]
Bug1146832.patch

Looking at similar bugs, :dao looks like a suitable reviewer.
Attachment #8591751 - Flags: review?(dao)
Comment on attachment 8591751 [details] [diff] [review]
Bug1146832.patch

>-          reportSendingMsg.style.display = "inline";
>+          reportSendingMsg.style.display = "block";

This doesn't belong in a script. Can you use reportSendingMsg.style.removeProperty("display") here and set display in the style sheet?

> #reportingState {
>-  padding-left: 150px;
>+  line-height:1.8em;

Can you document where does this number comes from?

nit: space after colon

>+  float: right;
> }

Does this work as expected for right-to-left languages? I assume it doesn't...

How does this handle even longer strings? Looks like it might overlap the "More Information..." link?

How about using inline-block or flexbox layout for both the link and the status label instead of display:block and float?
Attachment #8591751 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #12)
> This doesn't belong in a script. Can you use
> reportSendingMsg.style.removeProperty("display") here and set display in the
> style sheet?

We're showing and hiding elements here (since we want the localized strings to some from a single source) - what's the preferred way of doing this?
 
> How does this handle even longer strings? Looks like it might overlap the
> "More Information..." link?

It looks fine (see the three screenshot attachments)

> How about using inline-block or flexbox layout for both the link and the
> status label instead of display:block and float?

I'll look at using flexbox - this will likely improve the right-to-left situation (although this patch is no worse than the current situation).
(In reply to Mark Goodwin [:mgoodwin] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > This doesn't belong in a script. Can you use
> > reportSendingMsg.style.removeProperty("display") here and set display in the
> > style sheet?
> 
> We're showing and hiding elements here (since we want the localized strings
> to some from a single source) - what's the preferred way of doing this?

Like I said, style.removeProperty("display") for showing the element such that it uses its default display as specified in style sheets.
Mark, any news on this? Thanks
Flags: needinfo?(mgoodwin)
Attached patch Bug1146832.patch (obsolete) — Splinter Review
Modified to address dao's feedback (in particular to make use of flexbox layout)

It now works better for right-to-left languages (messages and buttons appear in expected positions).

Philipp, do you need new screenshots? You can test this by hitting any site with a non-overrideable TLS error.
Attachment #8591751 - Attachment is obsolete: true
Attachment #8592768 - Attachment is obsolete: true
Attachment #8592770 - Attachment is obsolete: true
Attachment #8592771 - Attachment is obsolete: true
Flags: needinfo?(mgoodwin)
Attachment #8595548 - Flags: ui-review?(philipp)
Attachment #8595548 - Flags: review?(dao)
Comment on attachment 8595548 [details] [diff] [review]
Bug1146832.patch

>+        <div id="certificateErrorReportingDescription">
>+          <p>&errorReporting.longDesc;</p>
>+          <p>
>+            <input type="checkbox" id="automaticallyReportInFuture" />
>+            <label for="automaticallyReportInFuture" id="automaticallyReportInFuture">&errorReporting.automatic;</label>
>+          </p>
>+          </div>
>+          <div id="errorStatePanel">

The last two lines are indented too far.

>+    // hide parts of the UI we don't need yet
>+    let contentDoc = content.document;
>+
>+    let reportSendingMsg = contentDoc.getElementById("reportSendingMessage");
>+    let reportSentMsg = contentDoc.getElementById("reportSentMessage");
>+    let reportBtn = contentDoc.getElementById("reportCertificateError");
>+    let retryBtn = contentDoc.getElementById("reportCertificateErrorRetry");
>+    reportSendingMsg.style.display = "none";
>+    reportSentMsg.style.display = "none";
>+    retryBtn.style.display = "none";
>   },

reportBtn is unused

>+div#errorStatePanel {
>+  display:flex;

nit: space after colon

You can (should) also remove "div" from the selector, #errorStatePanel works just fine.

>+  /* adjust the line-height to match the link */
>+  line-height: 22px;

This doesn't seem to have any meaningful effect? I removed it and seemingly got the same result.
Attachment #8595548 - Flags: review?(dao)
position: relative; and display: block; don't seem to be needed either... Am I missing something?
Attached patch Bug1146832.patchSplinter Review
Addressed feedback in comment 17 and comment 18, specifically:
* fixed bad indentation in aboutNetError.xhtml
* removed unused variable (reportBtn) in content.js
* added missing space before 'flex' in aboutNetError.css
* removed unnecessary 'div' from selector
* removed "position: relative" and "display:block" from reportingState and reportingStateMessage since they weren't needed
* removed "button#reportCertificateError" and "#button:reportCertificateErrorRetry" sections from aboutNetError.css since they only contained items that had no effect
Attachment #8595548 - Attachment is obsolete: true
Attachment #8597933 - Flags: review?(dao)
Attached image without line-height
Attached image with line-height
(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 8595548 [details] [diff] [review]
> >+  /* adjust the line-height to match the link */
> >+  line-height: 22px;
> 
> This doesn't seem to have any meaningful effect? I removed it and seemingly
> got the same result.

The difference is subtle but I noticed omitting it made the message sit above the line of the link on the left.

See with: https://bugzilla.mozilla.org/attachment.cgi?id=8597937
and without: https://bugzilla.mozilla.org/attachment.cgi?id=8597936
Comment on attachment 8597933 [details] [diff] [review]
Bug1146832.patch

>+  /* adjust the line-height to match the link */
>+  line-height: 22px;

I still don't understand what's going on here. Where did you get that number from? Trial and error? Or is there actually some CSS somewhere setting the link's line-height to 22px?

Does this still work as expected when setting a minimum font size in the content/fonts preferences?
(In reply to Dão Gottwald [:dao] from comment #24)
> Comment on attachment 8597933 [details] [diff] [review]
> I still don't understand what's going on here. Where did you get that number
> from? Trial and error? Or is there actually some CSS somewhere setting the
> link's line-height to 22px?

There's some CSS setting that - as seen through use of the devtools inspector on the link. I'm guessing it's from here https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#394

> Does this still work as expected when setting a minimum font size in the
> content/fonts preferences?

I would imagine so. I'll test and let you know if not.
(In reply to Mark Goodwin [:mgoodwin] from comment #25)
> There's some CSS setting that - as seen through use of the devtools
> inspector on the link. I'm guessing it's from here
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/common.inc.css#394

Ok, can you please remove that? It makes no sense as far as I can tell.
(In reply to Dão Gottwald [:dao] from comment #26)
> Ok, can you please remove that? It makes no sense as far as I can tell.

Shouldn't this be in a follow up? I'm not at all confident doing this won't break something else and, from reading the comments above, it's likely people will want uplift.
(In reply to Mark Goodwin [:mgoodwin] from comment #27)
> (In reply to Dão Gottwald [:dao] from comment #26)
> > Ok, can you please remove that? It makes no sense as far as I can tell.
> 
> Shouldn't this be in a follow up? I'm not at all confident doing this won't
> break something else and, from reading the comments above, it's likely
> people will want uplift.

Do they? This UI is currently disabled, isn't it? I had to flip a hidden pref to see it.
(In reply to Dão Gottwald [:dao] from comment #28)
> Do they? This UI is currently disabled, isn't it? I had to flip a hidden
> pref to see it.

This UI is not currently disabled. All non-overridable TLS errors hit this UI currently.
Ok, please file a followup bug then? Or just upload a second patch taking care of it only for mozilla-central here.
Blocks: 1159789
(In reply to Dão Gottwald [:dao] from comment #30)
> Ok, please file a followup bug then?

Done.
Comment on attachment 8597933 [details] [diff] [review]
Bug1146832.patch

thanks
Attachment #8597933 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/56da9fc21bb5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
This is too late for 38 but we will take a patch for 39.
Mark, can you request uplift to aurora? Thanks!
Comment on attachment 8597933 [details] [diff] [review]
Bug1146832.patch

Approval Request Comment
[Feature/regressing bug #]: 1146832
[User impact if declined]: The "report sent" message looks a little funky for some locales
[Describe test coverage new/current, TreeHerder]: Existing tests (browser chrome) cover this functionality - the changes are essentially cosmetic.
[Risks and why]: Low. The changes are isolated to error reporting functionality and essentially cosmetic.
[String/UUID change made/needed]: None
Flags: needinfo?(mgoodwin)
Attachment #8597933 - Flags: approval-mozilla-aurora?
Comment on attachment 8597933 [details] [diff] [review]
Bug1146832.patch

Approved for uplift to aurora.
Attachment #8597933 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.