Closed Bug 1321306 Opened 8 years ago Closed 8 years ago

about:tabcrashed unsubmitted crash notification offer label doesn't wrap nicely

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- verified
firefox53 --- fixed
firefox54 --- unaffected
firefox55 --- unaffected

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(1 file)

Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed.
58 bytes, text/x-review-board-request
Gijs
: review+
jcristau
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
Before bug 418833 was available, in-content HTML checkboxes couldn't be styled the way we wanted them. XUL checkboxes, however, can be styled just fine. What we were doing for HTML checkboxes before was to assume that a <label> came right after the <input type="checkbox"/> element, and then putting a pseudoelement before the <label> to act as the checkbox (and then hiding the checkbox). This worked mostly fine, but for long strings that cause the <label> to wrap around, the label text wraps underneath the checkbox, which doesn't look great. Because bug 418833 landed on 53, and the checkbox in question only landed in 52, this bug only affects 52, in English at least.
Comment on attachment 8815768 [details] Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed. https://reviewboard.mozilla.org/r/96578/#review96816 ::: browser/themes/shared/aboutTabCrashed.css:100 (Diff revision 1) > + margin-inline-start: -35px; > +} > + > +#autoSubmit + label { > + margin-inline-start: 35px; > + line-height: 1em; Are we sure this is enough also for other languages that might need more vertical space?
Attachment #8815768 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #2) > Comment on attachment 8815768 [details] > Bug 1321306 - Use some margin hackery to make sure text wrapping works > correctly in about:tabcrashed. > > https://reviewboard.mozilla.org/r/96578/#review96816 > > ::: browser/themes/shared/aboutTabCrashed.css:100 > (Diff revision 1) > > + margin-inline-start: -35px; > > +} > > + > > +#autoSubmit + label { > > + margin-inline-start: 35px; > > + line-height: 1em; > > Are we sure this is enough also for other languages that might need more > vertical space? Good question. I wonder how I'd test this. Hey davidb - do you know if we have any locales that have particularly _tall_ characters?
Flags: needinfo?(dbolter)
(In reply to Mike Conley (:mconley) - Working through post-Hawaii reviews/needinfos from comment #3) > (In reply to :Gijs Kruitbosch from comment #2) > > Comment on attachment 8815768 [details] > > Bug 1321306 - Use some margin hackery to make sure text wrapping works > > correctly in about:tabcrashed. > > > > https://reviewboard.mozilla.org/r/96578/#review96816 > > > > ::: browser/themes/shared/aboutTabCrashed.css:100 > > (Diff revision 1) > > > + margin-inline-start: -35px; > > > +} > > > + > > > +#autoSubmit + label { > > > + margin-inline-start: 35px; > > > + line-height: 1em; > > > > Are we sure this is enough also for other languages that might need more > > vertical space? > > Good question. I wonder how I'd test this. > > Hey davidb - do you know if we have any locales that have particularly > _tall_ characters? IME 1.5em or 1.75em are more conservative line-height values. Do we require the line-height to be smaller here in order to fix this bug? Poking at this on about:tabcrashed after manually unhiding these items (on OS X), it looks like the default line-height here would be... fine? So perhaps we can just land this without the line-height changes?
As per chat: Bengali is tall. (via Pike)
Flags: needinfo?(dbolter)
(In reply to :Gijs Kruitbosch from comment #4) > > IME 1.5em or 1.75em are more conservative line-height values. Do we require > the line-height to be smaller here in order to fix this bug? Poking at this > on about:tabcrashed after manually unhiding these items (on OS X), it looks > like the default line-height here would be... fine? So perhaps we can just > land this without the line-height changes? Manually removing the rule after displaying the about:tabcrashed page, and yeah - things look okay, but I think that's just us not flushing the styles correctly. If I remove the rule, re-build, and shrink the window, I get this: http://imgur.com/a/GiCqg ^-- this is what I'm trying to avoid with the line-height.
Comment on attachment 8815768 [details] Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed. https://reviewboard.mozilla.org/r/96578/#review96816 > Are we sure this is enough also for other languages that might need more vertical space? I tested this with Bengali (recommended by Pike as a "tall" script), and have adjusted this to 1.75em to be more conservative.
Comment on attachment 8815768 [details] Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed. Approval Request Comment [Feature/Bug causing the regression]: Bug 1309316 [User impact if declined]: The label for the checkbox for submitted backlogged crash reports in about:tabcrashed might not wrap correctly for smaller window sizes. Here's an example of what this looks like: https://reviewboard.mozilla.org/r/96578/file/402/ [Is this code covered by automated tests?]: No - this is styling only, and at this point, can only be verified manually. [Has the fix been verified in Nightly?]: No. This patch is only for Aurora. The reason we don't need this on Nightly is because Nightly has bug 418833 fixed, which allows us to create custom-styled checkboxes properly in HTML content. Because Aurora doesn't have this, we have to hack around it. [Needs manual test from QE? If yes, steps to reproduce]: It can't hurt. Steps to reproduce: 1) In a brand new profile, install an add-on that will crash the content process. Example: https://addons.mozilla.org/en-US/firefox/addon/crash-me-now-simple/ 2) Open a tab, and send it to some webpage, like https://www.mozilla.org 3) Crash the tab by clicking on the add-on toolbar button "Crash content process!" (You might need to enter customize mode to bring this button to the toolbar or panel) 4) When the tab shows the Tab Crashed page, try shrinking the window down. Look at the checkbox label for submitting backlogged crash reports. Shrinking the window should not cause the text in the label to overlap the checkbox. Bad: https://reviewboard.mozilla.org/r/96578/file/402/ Good: https://reviewboard.mozilla.org/r/96578/file/400/ [List of other uplifts needed for the feature/fix]: None, thankfully. [Is the change risky?]: No. [Why is the change risky/not risky?]: Styling only. [String changes made/needed]: None.
Attachment #8815768 - Flags: approval-mozilla-aurora?
Comment on attachment 8815768 [details] Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed. fix text wrapping in about:tabcrashed for aurora52
Attachment #8815768 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Let's make sure this is tested once it lands in aurora. Comment 9 has STR.
Flags: qe-verify+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I was unable to reproduce the issue by following the steps in comment 9 using old Aurora 52.0a2 (Build Id: 20161129013042), Aurora 52.0a2 (Build Id:20161115004018). I got the same behavior as in Firefox 52 beta 8, where this issues is marked as fixed. Am I missing something here? Is there a special build that I need to use in order to reproduce this issue?
Flags: needinfo?(mconley)
(In reply to Emil Ghitta, QA [:emilghitta] from comment #13) > I was unable to reproduce the issue by following the steps in comment 9 > using old Aurora 52.0a2 (Build Id: 20161129013042), Aurora 52.0a2 (Build > Id:20161115004018). I got the same behavior as in Firefox 52 beta 8, where > this issues is marked as fixed. > > Am I missing something here? Is there a special build that I need to use in > order to reproduce this issue? Hey Emil. The concern that this patch tries to address is probably not immediately apparent if the window is sufficiently wide, as you need to cause the "unsubmitted crash" text to wrap in order to see the bug. Try making the window smaller in the build without this patch, and you'll notice that the text wraps _underneath_ the checkbox. With this patch, the text wraps, but is still to the right of the checkbox.
Flags: needinfo?(mconley)
Thanks Mike for the clarification! I've reproduced the issue described in comment 9 using 52.0a2 old Aurora (Build Id:20161214004021)and verified that the issues is not reproducible anymore using Firefox 52 beta 8 (Build ID:20170220070057) across platforms (macOS 10.12 ,Windows 10 64bit and Ubuntu 16.04 64bit).
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
Comment on attachment 8815768 [details] Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed. Approval Request Comment [Feature/Bug causing the regression]: See bug 1352406 [User impact if declined]: The label for the checkbox for submitted backlogged crash reports in about:tabcrashed might not wrap correctly for smaller window sizes. Here's an example of what this looks like: https://reviewboard.mozilla.org/r/96578/file/402/ [Is this code covered by automated tests?]: No - this is styling only, and at this point, can only be verified manually. [Has the fix been verified in Nightly?]: No. This is skipping Nightly and going straight to Beta. lizzard knows the specifics - see bug 1352406 for details. [Needs manual test from QE? If yes, steps to reproduce]: 1) In a brand new profile, install an add-on that will crash the content process. Example: https://addons.mozilla.org/en-US/firefox/addon/crash-me-now-simple/ 2) Open a tab, and send it to some webpage, like https://www.mozilla.org 3) Crash the tab by clicking on the add-on toolbar button "Crash content process!" (You might need to enter customize mode to bring this button to the toolbar or panel) 4) When the tab shows the Tab Crashed page, try shrinking the window down. Look at the checkbox label for submitting backlogged crash reports. Shrinking the window should not cause the text in the label to overlap the checkbox. Bad: https://reviewboard.mozilla.org/r/96578/file/402/ Good: https://reviewboard.mozilla.org/r/96578/file/400/ Also see comment 13 to comment 16. [List of other uplifts needed for the feature/fix]: See bug 1352406. It is required that a number of backouts land before this lands. Please coordinate with mconley before attempting a landing. [Is the change risky?]: Not this patch, no. [Why is the change risky/not risky?]: Simple, well understood styling change that we shipped in 52 already. [String changes made/needed]: None.
Attachment #8815768 - Flags: approval-mozilla-beta?
Comment on attachment 8815768 [details] Bug 1321306 - Use some margin hackery to make sure text wrapping works correctly in about:tabcrashed. As discussed in bug 1352406, let's uplift this for 53 beta 10.
Attachment #8815768 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: