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)
Tracking
()
RESOLVED
FIXED
Firefox 40
People
(Reporter: aryx, Assigned: mgoodwin)
References
Details
Attachments
(4 files, 5 obsolete files)
16.45 KB,
image/png
|
Details | |
6.12 KB,
patch
|
dao
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
94.92 KB,
image/png
|
Details | |
95.06 KB,
image/png
|
Details |
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
Comment 1•9 years ago
|
||
[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...
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
tracking-firefox37:
--- → ?
tracking-firefox38:
--- → ?
Flags: needinfo?(mmc)
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
I'll take a look at this
Assignee: nobody → mgoodwin
Flags: needinfo?(mgoodwin)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
Hey Mark, could you upload a screenshot of the patch showing how the text breaks now? Thanks!
Flags: needinfo?(philipp)
Updated•9 years ago
|
Flags: needinfo?(mgoodwin)
Assignee | ||
Comment 7•9 years ago
|
||
Flags: needinfo?(mgoodwin)
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Comment 10•9 years ago
|
||
Comment on attachment 8591751 [details] [diff] [review] Bug1146832.patch Thanks for the screen shots! I think that works.
Attachment #8591751 -
Flags: ui-review+
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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-
Assignee | ||
Comment 13•9 years ago
|
||
(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).
Comment 14•9 years ago
|
||
(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.
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
position: relative; and display: block; don't seem to be needed either... Am I missing something?
Comment 19•9 years ago
|
||
Comment on attachment 8595548 [details] [diff] [review] Bug1146832.patch Looks good!
Attachment #8595548 -
Flags: ui-review?(philipp) → ui-review+
Assignee | ||
Comment 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
(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 24•9 years ago
|
||
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?
Assignee | ||
Comment 25•9 years ago
|
||
(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.
Comment 26•9 years ago
|
||
(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.
Assignee | ||
Comment 27•9 years ago
|
||
(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.
Comment 28•9 years ago
|
||
(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.
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Comment 30•9 years ago
|
||
Ok, please file a followup bug then? Or just upload a second patch taking care of it only for mozilla-central here.
Assignee | ||
Comment 31•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #30) > Ok, please file a followup bug then? Done.
Comment 32•9 years ago
|
||
Comment on attachment 8597933 [details] [diff] [review] Bug1146832.patch thanks
Attachment #8597933 -
Flags: review?(dao) → review+
Assignee | ||
Comment 33•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=404c440eb708
Comment 35•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/56da9fc21bb5
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 36•9 years ago
|
||
This is too late for 38 but we will take a patch for 39.
tracking-firefox39:
--- → +
Flags: needinfo?(mgoodwin)
Comment 37•9 years ago
|
||
Mark, can you request uplift to aurora? Thanks!
Assignee | ||
Comment 38•9 years ago
|
||
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 39•9 years ago
|
||
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.
Description
•