Crash reporter dialog not wide enough for l10n

RESOLVED FIXED in mozilla1.9

Status

()

Toolkit
Crash Reporting
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: atopal, Assigned: jimm)

Tracking

({l12y})

Trunk
mozilla1.9
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 301113 [details]
german text in crash reporter dialog

The crash reporter dialog is not wide enough for localized text and it seems there is no way for localizers to change the size of that dialog. Texts on Buttons are also cut off and the buttons are apparently only properly sized for the english text.
(Reporter)

Updated

10 years ago
Keywords: l12y
Changing OS per screenshot.  This should work fine on Mac, I definitely tested it there.  I thought I tested it on Win32 as well, but obviously not.
Component: General → Breakpad Integration
OS: Mac OS X → Windows XP
Product: Firefox → Toolkit
QA Contact: general → breakpad.integration

Comment 2

10 years ago
BTW, the stray \n the dialog seems to be caused by a \n\n\n instead of \n\n in crashreporter-override.ini - I guess it's the middle one of those that pops up as plain text in the dialog...
(Reporter)

Comment 3

10 years ago
No, it's because of a \n\\n\n, but that's fixed already
Flags: blocking1.9?
+'ing with a P3.  Not the highest priority, but we should fix this.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
I have a patch.
Assignee: nobody → ted.mielczarek
Flags: blocking1.9+
Priority: P3 → P2
Flags: tracking1.9+

Comment 6

10 years ago
Ted where's that patch...
I was having an issue with it so I dropped it for a bit. Figured that out, just need to polish it and test a bit. Should be done in a day or so.
Created attachment 310356 [details] [diff] [review]
resize things to fit

Took me a bit to polish it. This should resize any of the controls to fit their text, and the dialog to fit the controls if necessary.
Attachment #310356 - Flags: review?(dcamp)
Created attachment 310360 [details]
german l10n with this patch

Better?

Comment 10

10 years ago
Yep, there's not cut-off text. The margin on the right is pretty small, compared to the left, but that doesn't impact readability.
(Reporter)

Comment 11

10 years ago
Yes, definitely better :)

Updated

10 years ago
Attachment #310356 - Flags: review?(dcamp) → review+
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Well, the crash reporter dialog is certainly wide enough now: http://martijn.martijn.googlepages.com/Clipboard01.gif
Perhaps caused by this patch?

Updated

10 years ago
Depends on: 424369
Ok, I filed bug 424369 for it.

Updated

10 years ago
Duplicate of this bug: 424433
Created attachment 311827 [details]
(Vista) Restart Firefox button should be a little bit wider

Comment 17

10 years ago
Created attachment 312580 [details]
Polish version

Still too narrow in polish version (tested on: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.9pre) Gecko/2008032904 Minefield/3.0pre).
Created attachment 312630 [details]
Turkish instance of the problem

I could use just one word for "Quit" and two words for "Restart" in Turkish without mentioning application name and it would fit.
(In reply to comment #17)
> Still too narrow in polish version (tested on: Mozilla/5.0 (Windows; U; Windows
> NT 5.1; pl; rv:1.9pre) Gecko/2008032904 Minefield/3.0pre).

We could shorten it to "Zakończ program" (Close the program) and "Uruchom ponownie" (Restart), but then we're losing the brand name. 

Unfortunately, we can't get rid of the helper word "program", as the brand name without the helper word would need to be declensed, "Zakończ Firefoksa".

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 425937
I'm stumped here. This Win32 theming code has me beat. If anyone else wants to take a crack at it, be my guest.

Comment 22

10 years ago
Jim can you help?
(Assignee)

Comment 23

10 years ago
Sure, I'll take a look.
Assignee: ted.mielczarek → jmathies
Status: REOPENED → NEW
(Assignee)

Comment 24

10 years ago
So the remaining issues seem to be the button widths, and according to that German screenshot, the overall margin padding of the dialog. Is that basically it?
(Reporter)

Comment 25

10 years ago
Created attachment 313956 [details]
Screenshot of crash reporter on German Windows XP

The German dialog is okayish now, still the padding of the button is a little narrow and the same goes for the right side of the dialog, but I can live with that.
Created attachment 314044 [details]
Still not OK (XP, pl)

Comment 27

10 years ago
Created attachment 314301 [details]
Screenshot of crash reporter on Belarusian Windows XP

Crash Reporter is too narrow for Belarusian. Tested with nightly build Firefox 3.0pre (build id 2008040606).
So it seems (comparing the Polish and Belarusian screenshots) that the buttons are adjusting their size to the content, but failing with margins/paddings...
Yes. The code goes through gyrations to get the theme dimensions and tries to size things accordingly, but appears to still be unable to handle it properly. See:

http://lxr.mozilla.org/mozilla/source/toolkit/crashreporter/client/crashreporter_win.cpp#713
http://lxr.mozilla.org/mozilla/source/toolkit/crashreporter/client/crashreporter_win.cpp#276

I wish this was XUL. :-(

Comment 30

10 years ago
Ted/Jim do you need any help here - we have ability to converge towards a fix?
(Assignee)

Comment 31

10 years ago
This is my #2 at the moment, currently working on bug 424675. I'm sure I'll get to this though before the next freeze. If anybody else wants to look at this in the meantime just take the assignment.

I can switch over to this right now if you want.. (bump to P1?) 

Comment 32

10 years ago
(In reply to comment #31)
> This is my #2 at the moment, currently working on bug 424675. I'm sure I'll get
> to this though before the next freeze. If anybody else wants to look at this in
> the meantime just take the assignment.
> 
> I can switch over to this right now if you want.. (bump to P1?) 
> 

Yes - please swap to this given the progress on 424675...
(Assignee)

Comment 33

10 years ago
Created attachment 316256 [details]
new button text example
(Assignee)

Comment 34

10 years ago
Created attachment 316259 [details] [diff] [review]
control padding patch v.1

Ok with this patch, the padding defined in the original rc is honored, except on controls where we define a userDefinedPadding value (like checkboxes). Checkboxes stretch the legnth of the dialog so we want to ignore whitespace, but on other controls like buttons, we want to use the layout created in the resource editor.

I need to check this on a 2K pl image I have, if anyone else cares to give it a run on their systems if they can that would be great.
Jim: if it works on Vista, XP, and XP with the classic theme, you're probably set. You can toss it up on the try server if you don't have all of those platforms readily available, and people can grab the resulting builds for testing. Looks good at a first glance, just r? me when you're ready.
(Assignee)

Comment 36

10 years ago
Created attachment 316278 [details]
pl crash report dialog w/new patch

I think things are in good shape!
(Assignee)

Updated

10 years ago
Attachment #316259 - Flags: review?(ted.mielczarek)
Comment on attachment 316259 [details] [diff] [review]
control padding patch v.1

+    if (!userDefinedPadding) 

nit: I'd prefer != 0 here. Thanks for fixing this, it had me beat!
Attachment #316259 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 38

10 years ago
> nit: I'd prefer != 0 here. 

I'll add that, I want to touch up the big comment block anyway.
(Assignee)

Comment 39

10 years ago
Created attachment 316288 [details] [diff] [review]
control padding patch v.2
Attachment #316259 - Attachment is obsolete: true
(Assignee)

Comment 40

10 years ago
"control padding patch v.2" is the only patch that needs checkin. "resize things to fit" has already landed.
Keywords: checkin-needed
(In reply to comment #36)
> pl crash report dialog w/new patch
> I think things are in good shape!

This looks nice, thanks. ;-)
Needs all patches need approval to land, even blockers.
Keywords: checkin-needed
(Assignee)

Comment 43

10 years ago
argh, sorry. I thought blockers were equal to an sr?
(Assignee)

Comment 44

10 years ago
> I thought blockers were equal to an sr?

correction - I meant "equal to an approval1.9?" Somewhere I missed something in all this.
Comment on attachment 316288 [details] [diff] [review]
control padding patch v.2

a1.9=beltzner
Attachment #316288 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed

Updated

10 years ago
Whiteboard: [needs landing
mozilla/toolkit/crashreporter/client/crashreporter_win.cpp 	1.28
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing
Target Milestone: --- → mozilla1.9
You need to log in before you can comment on or make changes to this bug.