415428 - Crash reporter dialog not wide enough for l10n

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P2)

Description

[Kadir Topal [:atopal]]

Attachment: 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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Robert Kaiser]

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...

Comment 3

[Kadir Topal [:atopal]]

No, it's because of a \n\\n\n, but that's fixed already

Comment 4

[Damon Sicore (:damons)]

+'ing with a P3.  Not the highest priority, but we should fix this.

Comment 5

[(not currently active) Ted Mielczarek]

I have a patch.

Comment 6

[Mike Schroepfer]

Ted where's that patch...

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: german l10n with this patch

Better?

Comment 10

[Axel Hecht [:Pike]]

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.

Comment 11

[Kadir Topal [:atopal]]

Yes, definitely better :)

Comment 12

[(not currently active) Ted Mielczarek]

Checked in.

Comment 13

[Martijn Wargers (dead)]

Well, the crash reporter dialog is certainly wide enough now: http://martijn.martijn.googlepages.com/Clipboard01.gif
Perhaps caused by this patch?

Comment 14

[Martijn Wargers (dead)]

Ok, I filed bug 424369 for it.

Comment 16

[Vlado Valastiak [:wladow]]

Attachment: (Vista) Restart Firefox button should be a little bit wider

Comment 17

[Bartosz Piec]

Attachment: 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).

Comment 18

[Ahmet Serkan Tıratacı]

Attachment: 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.

Comment 19

[Marek Stępień [:marcoos, inactive]]

(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.

Comment 21

[(not currently active) Ted Mielczarek]

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

[Mike Schroepfer]

Jim can you help?

Comment 23

[Jim Mathies [:jimm]]

Sure, I'll take a look.

Comment 24

[Jim Mathies [:jimm]]

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?

Comment 25

[Kadir Topal [:atopal]]

Attachment: 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.

Comment 26

[Marek Stępień [:marcoos, inactive]]

Attachment: Still not OK (XP, pl)

Comment 27

[Siarhei]

Attachment: 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).

Comment 28

[Marek Stępień [:marcoos, inactive]]

So it seems (comparing the Polish and Belarusian screenshots) that the buttons are adjusting their size to the content, but failing with margins/paddings...

Comment 29

[(not currently active) Ted Mielczarek]

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

[Mike Schroepfer]

Ted/Jim do you need any help here - we have ability to converge towards a fix?

Comment 31

[Jim Mathies [:jimm]]

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

[Mike Schroepfer]

(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...

Comment 33

[Jim Mathies [:jimm]]

Attachment: new button text example

Comment 34

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 35

[(not currently active) Ted Mielczarek]

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.

Comment 36

[Jim Mathies [:jimm]]

Attachment: pl crash report dialog w/new patch

I think things are in good shape!

Comment 37

[(not currently active) 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!

Comment 38

[Jim Mathies [:jimm]]

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

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

Comment 39

[Jim Mathies [:jimm]]

Attachment: control padding patch v.2

Comment 40

[Jim Mathies [:jimm]]

"control padding patch v.2" is the only patch that needs checkin. "resize things to fit" has already landed.

Comment 41

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #36)
> pl crash report dialog w/new patch
> I think things are in good shape!

This looks nice, thanks. ;-)

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Needs all patches need approval to land, even blockers.

Comment 43

[Jim Mathies [:jimm]]

argh, sorry. I thought blockers were equal to an sr?

Comment 44

[Jim Mathies [:jimm]]

> I thought blockers were equal to an sr?

correction - I meant "equal to an approval1.9?" Somewhere I missed something in all this.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

They used to be, until we entered "lockdown" for RC1:
http://developer.mozilla.org/devnews/index.php/2008/04/08/tree-is-entering-lockdown-for-firefox-3-release-candidate-1/

Comment 46

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316288 [details] [diff] [review]
control padding patch v.2

a1.9=beltzner

Comment 47

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/crashreporter/client/crashreporter_win.cpp 	1.28