406088 - crash reporter sucks in rtl

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P2)

Description

[(not currently active) Ted Mielczarek]

Pike has a screenshot of it sucking in Hebrew.  We should make this suck less.

Comment 1

[Axel Hecht [:Pike]]

Attachment: UI not RTL aligned

Comment 2

[Axel Hecht [:Pike]]

One way to do this is to just add another entry to crashreporter.ini,

# LOCALIZATION NOTE (isRTL):
# Leave this entry empty unless your language requires right-to-left layout,
# for example like Arabic, Hebrew, Persian. If your language needs RTL, please
# use the untranslated English word "yes" as value
isRTL=

and check that for non-empty values to switch on RTL layout.

I think that software update has the same problem, though I didn't find a bug on that. Tomer? Behnam?

Comment 3

[Tomer Cohen :tomer]

Attachment: Minefield Software Updater (Hebrew)

I didn't noticed that. Should we file a bug requesting for a automatic-crash feature? 

As for software updater, I think it don't have any RTL issues, since it is a plain XUL window, while the Crash Reporter (AFAIK) isn't.

Comment 4

[Benjamin Smedberg]

http://benjamin.smedbergs.us/tests/memory-corrupter-0.2.xpi will crash for you, on Windows or Mac.

Comment 5

[(not currently active) Ted Mielczarek]

Tomer: fwiw, I think he meant the update.exe, which only has a very small UI, but it is a native UI.

Comment 6

[Tomer Cohen :tomer]

(In reply to comment #5)
> Tomer: fwiw, I think he meant the update.exe, which only has a very small UI,
> but it is a native UI.
> 

I Can't see any GUI for ./updater on linux.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: wip win32 patch

This is a WIP that covers Win32 only.  smontagu pointed out that there's a window style you can set on Win32 that mirrors your window.  This patch sets it on the dialog, so it looks a little better.  Unfortunately all of the code that does resizing breaks badly with this, probably due to stuff explained here:
http://msdn2.microsoft.com/en-us/library/ms903932.aspx

I don't have the time to finish this patch, but I wanted to try it out and see if I could get a quick fix for Win32.  If someone else wanted to step up to the plate and finish this off, I would be happy to give a speedy review to squeak this in for beta 4 OS X and Linux are probably a little harder, so I didn't even try on those platforms.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: screenshot of crash reporter with the patch applied

Comment 9

[(no longer active)]

(In reply to comment #7)
> I don't have the time to finish this patch, but I wanted to try it out and see
> if I could get a quick fix for Win32.  If someone else wanted to step up to the
> plate and finish this off, I would be happy to give a speedy review to squeak
> this in for beta 4

I'd like to help on the Windows side of the bug.  I appreciate your guidance as promised above ;-)

Comment 10

[(not currently active) Ted Mielczarek]

Ehsan: that would be great! If you need me you can find me on irc.mozilla.org as ted.

Comment 11

[(no longer active)]

Just one quick question.  If I'm reading the patch correctly, it changes the dialog to RTL without checking for locale settings, etc.  Should we use some trick as described in comment 2 to enable RTL mode only for locales which actually need it?

Comment 12

[(not currently active) Ted Mielczarek]

Yeah, that was my plan, I just hacked this together to test.  The |if (true)| should be |if (Str("isRTL") == L"yes")| or something like that.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: screenshot of crash reporter with the (updated) patch applied

So I applied my patch and found out that it works a lot better now that I fixed bug 415428. smontagu pointed out a few things, so I cleaned it up a little bit, and this is the result. The only issue that he pointed out that I didn't fix is that the email text field will be in RTL, when it really ought to be LTR. Turns out that's hard to fix without a lot of hassle, so I left it. I'll post the updated patch in a minute.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: honor isRTL field in crashreporter.ini on win32 [checked in]

This works pretty well on Win32.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 310484 [details] [diff] [review]
honor isRTL field in crashreporter.ini on win32 [checked in]

Axel said this would be ok and we could just mass-patch the localizations, so no real l10n impact.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 310484 [details] [diff] [review]
honor isRTL field in crashreporter.ini on win32 [checked in]

From an l10n point of view, we should be able to create a mass-patch for all localizations and just fix ar and he by hand, so the actual impact on localizers would be small, and IMHO, bearable.

We should come up with that patch in time for landing, I can help there.

Comment 17

[Tomer Cohen :tomer]

(In reply to comment #13)
> The only issue that he pointed out that I didn't fix is
> that the email text field will be in RTL, when it really ought to be LTR. T

Sorry for jumping, but I think we should change that string to request the user to supply English information only. I'm not sure how helpful will be crash information in Hebrew, for example. 

(I should also change the Hebrew translation seen in attachment 305042 [details] to make sure the text isn't oversized) 

Comment 18

[(not currently active) Ted Mielczarek]

I'm not going to be around, so if this gets review in time for the b5 freeze, someone else will have to shepherd this in. (request approval, then check in or set checkin-needed).

Comment 19

[Axel Hecht [:Pike]]

We should get this evaluated for taking for RC1.

This is a RTL bug, there seems to be a fix in hand, with reasonable impact on l10n at large. I'd be giving this a P2+epsilon, epsilon depending on the load on ted and dcamp.

Comment 20

[Damon Sicore (:damons)]

While we should fix this, at this point in the release process, I feel we could ship this change in a dot release.  We shouldn't hold back the release for this issue.  Marking wanted1.9.0.x+.

Additionally, will take this patch once reviews are completed, but dcamp has other blockers he should be working on at the moment.

Comment 21

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

Comment 22

[Dave Camp (:dcamp)]

Comment on attachment 310484 [details] [diff] [review]
honor isRTL field in crashreporter.ini on win32 [checked in]

It's kinda sad that this is how it's supposed to be done, but MSDN seems to concur...

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 310484 [details] [diff] [review]
honor isRTL field in crashreporter.ini on win32 [checked in]

Pushed to mozilla-central. Since we don't have l10n builds there, I just checked in the changes to the en-US crashreporter.ini. If we want to backport this to 1.9.0 we'll need to sort that out, obviously.

Comment 24

[(not currently active) Ted Mielczarek]

We'll need separate fixes for Mac/Linux.

Comment 25

[(not currently active) Ted Mielczarek]

This should fix us for Linux:
http://library.gnome.org/devel/gtk/unstable/GtkWidget.html#gtk-widget-set-default-direction

Comment 26

[(not currently active) Ted Mielczarek]

http://mavra.perilith.com/~luser/crashreporter-gtk-he.png

How does that look?

Comment 27

[(not currently active) Ted Mielczarek]

http://mavra.perilith.com/~luser/firefox-3.1a1pre.en-US.linux-i686.tar.bz2

Here's a patched Linux build, with the crashreporter localized in hebrew. If someone could test this, I would appreciate it.

Comment 28

[(not currently active) Ted Mielczarek]

Attachment: simple patch for linux [checked in]

The first hunk is just to silence a warning.

Comment 29

[Simon Montagu :smontagu]

I tested the patched Linux build and everything looks fine to me (when I finally got it to crash). Thanks, Ted!

Comment 30

[(no longer active)]

Looks good to me as well.  I can't read Hebrew, but the direction has been correctly changed, as far as I can tell.

Comment 31

[(not currently active) Ted Mielczarek]

Comment on attachment 325684 [details] [diff] [review]
simple patch for linux [checked in]

Simple fix for RTL on Linux.

Comment 32

[timeless]

fwiw, it looked ok to me (i did try reading it). I don't like the font, but that's me :)

Comment 33

[Dave Camp (:dcamp)]

Comment on attachment 325684 [details] [diff] [review]
simple patch for linux [checked in]

This is fine, but it might be nice to include the locale name in the ini file and use setlocale() where appropriate to get stuff like this right by default.

Comment 34

[(not currently active) Ted Mielczarek]

Ah, I had no idea that's what it used. Would our locale names map properly to whatever GTK is expecting?

Comment 35

[(not currently active) Ted Mielczarek]

Comment on attachment 325684 [details] [diff] [review]
simple patch for linux [checked in]

Pushed to mozilla-central.

Comment 36

[(not currently active) Ted Mielczarek]

Attachment: mac patch

Screenshot: http://mavra.perilith.com/~luser/crashreporter-he-osx.png
Test build: http://people.mozilla.com/~tmielczarek/firefox-3.1a1pre.en-US.mac.dmg

Here's a patch for OS X. I created a new nib that has an RTL layout, and added branches to a few of the layout code paths, and it's looking pretty good. The only thing bothering me is the scrollbar on the text area being on the wrong side, but I don't know of any way to fix that. I'd appreciate any feedback on this build.

Comment 37

[(not currently active) Ted Mielczarek]

Smokey: wondering if you could take a minute to try that build out?

Comment 38

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ted, I can't get crashreporter to trigger in that build (I'm still using the testcase on bug 378521 to crash; bsmedberg's extension still won't install).

Comment 39

[(not currently active) Ted Mielczarek]

http://ted.mielczarek.org/code/mozilla/crashme.xpi
Tools -> Crash me now!

Comment 40

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Still not getting the crashreporter :(  

I'm getting the Mac OS X one and no Moz one (even when I completely remove the defaults key that allows us to show the Mac OS X crash reporter).  Not sure what's happening here; crashreporter works fine in 3.0 and /latest-mozilla-central/ :(

/me puzzled; I'll try to catch you on irc later.

Comment 41

[(not currently active) Ted Mielczarek]

Oh, that build may not have the crashreporter enabled in application.ini. You can edit that file (inside the bundle) or you can set MOZ_CRASHREPORTER=1 in the environment.

Comment 42

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Yeah, enabling the crashreporter in application.ini fixed the launching; however, after all that, it looks like the build only has the English crashreporter :(

Comment 43

[(not currently active) Ted Mielczarek]

Smokey: I uploaded a new build, same URL as comment 36, that has the Hebrew l10n, and the crash reporter enabled by default. Turns out I was trying to package from the x86 half of my universal build, and kept packaging the wrong files. Oops.

Comment 44

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ted: just a few little things.

1) Most of the text in the "Details" sheet isn't translated (and thus is LTR).  Is it supposed to be translated, or are those the literal db fieldnames/keys being displayed?  If the bold items are supposed to be localized, the sheet's TextView should really be RTL, I think.

2) For whatever reason, the placeholder text in the comments field is RTL, but when I try to type something (in an RTL script, even), the cursor starts on the left.  I asked around, and apparently doing something with setBaseWritingDirection:range: http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSTextView_Class/Reference/Reference.html#//apple_ref/doc/uid/20000373-CJBDCJIC on the NSTextView inside of the NSScrollView should be able to fix that (but not flip the scrollbar, alas).  Rather annoying that can't be set in the nib, but oh well.

3) By contrast, the email address field is RTL, and it's designed for LTR-only text, so it feels very strange typing in in.  I'd switch the text direction of the field to LTR in the nib (but leave the placement as-is).  

Oh, hmm.  I see that field has placeholder text if you've never before entered an email address (it doesn't re-appear if you delete the address and tab somewhere else).  I suppose if you can trick this field into behaving like the comments field is currently (mis)behaving, that would be the best solution.  Otherwise, I think it's a toss-up as to what's more peculiar: an right-justified, RTL-ish email address, or an left-justified RTL placeholder string.

ٌOther than those, I think everything looks good and behaves OK.

Comment 45

[(not currently active) Ted Mielczarek]

Thanks for the testing, I appreciate it!

(In reply to comment #44)
> 1) Most of the text in the "Details" sheet isn't translated (and thus is LTR). 
> Is it supposed to be translated, or are those the literal db fieldnames/keys
> being displayed?  If the bold items are supposed to be localized, the sheet's
> TextView should really be RTL, I think.

That text is the literal key/value pairs being sent, so it's always ASCII.

> 
> 2) For whatever reason, the placeholder text in the comments field is RTL, but
> when I try to type something (in an RTL script, even), the cursor starts on the
> left.  I asked around, and apparently doing something with
> setBaseWritingDirection:range:
> http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSTextView_Class/Reference/Reference.html#//apple_ref/doc/uid/20000373-CJBDCJIC
> on the NSTextView inside of the NSScrollView should be able to fix that (but
> not flip the scrollbar, alas).  Rather annoying that can't be set in the nib,
> but oh well.

Thanks, I wasn't sure how the field would actually behave, since I haven't tried entering an RTL script in it. I subclass the TextView to draw the placeholder text, so I was able to make that appear correctly.

> 3) By contrast, the email address field is RTL, and it's designed for LTR-only
> text, so it feels very strange typing in in.  I'd switch the text direction of
> the field to LTR in the nib (but leave the placement as-is).  
> 
> Oh, hmm.  I see that field has placeholder text if you've never before entered
> an email address (it doesn't re-appear if you delete the address and tab
> somewhere else).  I suppose if you can trick this field into behaving like the
> comments field is currently (mis)behaving, that would be the best solution. 
> Otherwise, I think it's a toss-up as to what's more peculiar: an
> right-justified, RTL-ish email address, or an left-justified RTL placeholder
> string.

Yeah, I wasn't sure how this would work. I think unfortunately aside from subclassing the control, I can't get it to switch between RTL/LTR for the placeholder/entry. FWIW, I'm pretty sure the Windows code has the same problem in RTL.

I'll fix #2 and get a patch up for review.

Comment 46

[(not currently active) Ted Mielczarek]

Attachment: mac patch, fix rtl entry in comment box [checked in]

Ok, this fixes the RTL entry in the comments box. I played with the email field, and I just don't see any way to make the hint text RTL but the entry LTR. I'm leaving it RTL in both since it's no worse than the alternative, afaict.

Comment 47

[Simon Montagu :smontagu]

(In reply to comment #46)
> Ok, this fixes the RTL entry in the comments box. I played with the email
> field, and I just don't see any way to make the hint text RTL but the entry
> LTR. I'm leaving it RTL in both since it's no worse than the alternative,
> afaict.

Is that direction or alignment? Entering email addresses in RTL direction is nasty, because the separators jump around, something like:
foo
@foo
foo@bar
.foo@bar
[e-mail]

Comment 48

[(not currently active) Ted Mielczarek]

It is direction, apparently. :-/ Looking at it again, I can set it to be right-aligned, but LTR direction, so the hint text looks ok, and entering the email address is not so bad, although still right-aligned.

Comment 49

[(not currently active) Ted Mielczarek]

Comment on attachment 326966 [details] [diff] [review]
mac patch, fix rtl entry in comment box [checked in]

Pushed to mozilla-central in 76d3bd6c0505.

Comment 50

[(not currently active) Ted Mielczarek]

This is fixed on trunk for all platforms now. I'll let the mac patch bake for a little bit, then see if we can get this in 1.9.0.x.

Comment 51

[(not currently active) Ted Mielczarek]

Comment on attachment 310484 [details] [diff] [review]
honor isRTL field in crashreporter.ini on win32 [checked in]

I'd like to get all three of these onto the 1.9 branch, to fix the appearance of the crashreporter in RTL locales. We'll also need to patch the locale files to include the isRTL flag, but Axel said previously that he could handle that, so it wouldn't affect localizers.

Comment 52

[Samuel Sidler (old account; do not CC)]

Ted, can you please do a roll-up patch of all three patches and request approval on it?

Comment 53

[Samuel Sidler (old account; do not CC)]

Comment on attachment 326966 [details] [diff] [review]
mac patch, fix rtl entry in comment box [checked in]

Moving out to 1.9.0.3 since Ted needs to coordinate this with l10n drivers.

Comment 54

[Samuel Sidler (old account; do not CC)]

We're opting not to take this in a maintenance release. It will be fixed in Firefox 3.1.