crash reporter sucks in rtl

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Blocks: 1 bug, {rtl})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9.0.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs rolls up patch])

Attachments

(3 attachments, 6 obsolete attachments)

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

Comment 1

11 years ago
Created attachment 290787 [details]
UI not RTL aligned

Comment 2

11 years ago
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

11 years ago
Created attachment 290923 [details]
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

11 years ago
http://benjamin.smedbergs.us/tests/memory-corrupter-0.2.xpi will crash for you, on Windows or Mac.
Tomer: fwiw, I think he meant the update.exe, which only has a very small UI, but it is a native UI.

Comment 6

11 years ago
(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.

Updated

11 years ago
Blocks: 415606

Updated

11 years ago
Blocks: 412273
Created attachment 305041 [details] [diff] [review]
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.
Created attachment 305042 [details]
screenshot of crash reporter with the patch applied
Keywords: helpwanted

Comment 9

11 years ago
(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 ;-)
Ehsan: that would be great! If you need me you can find me on irc.mozilla.org as ted.

Comment 11

11 years ago
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?
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.
Created attachment 310477 [details]
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.
Assignee: nobody → ted.mielczarek
Attachment #305042 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Created attachment 310484 [details] [diff] [review]
honor isRTL field in crashreporter.ini on win32 [checked in]

This works pretty well on Win32.
Attachment #305041 - Attachment is obsolete: true
Attachment #310484 - Flags: review?(dcamp)
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.
Attachment #310484 - Flags: review?(l10n)
Keywords: helpwanted
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.
Attachment #310484 - Flags: review?(l10n) → review+
(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) 

Updated

11 years ago
Attachment #310477 - Attachment is patch: false
Attachment #310477 - Attachment mime type: text/plain → image/png
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).
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.
Flags: blocking1.9?
Priority: -- → P2
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.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-

Updated

11 years ago
Blocks: 415597

Updated

11 years ago
No longer blocks: 412273

Comment 21

11 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Blocks: 412273
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...
Attachment #310484 - Flags: review?(dcamp) → review+
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.
Attachment #310484 - Attachment description: honor isRTL field in crashreporter.ini on win32 → honor isRTL field in crashreporter.ini on win32 [checked in]
Attachment #290787 - Attachment is obsolete: true
Attachment #290923 - Attachment is obsolete: true
Attachment #310477 - Attachment is obsolete: true
We'll need separate fixes for Mac/Linux.
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.
Created attachment 325684 [details] [diff] [review]
simple patch for linux [checked in]

The first hunk is just to silence a warning.
I tested the patched Linux build and everything looks fine to me (when I finally got it to crash). Thanks, Ted!

Comment 30

11 years ago
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 on attachment 325684 [details] [diff] [review]
simple patch for linux [checked in]

Simple fix for RTL on Linux.
Attachment #325684 - Flags: review?(dcamp)

Comment 32

11 years ago
fwiw, it looked ok to me (i did try reading it). I don't like the font, but that's me :)
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.
Attachment #325684 - Flags: review?(dcamp) → review+
Ah, I had no idea that's what it used. Would our locale names map properly to whatever GTK is expecting?
Comment on attachment 325684 [details] [diff] [review]
simple patch for linux [checked in]

Pushed to mozilla-central.
Attachment #325684 - Attachment description: simple patch → simple patch for linux [checked in]
Created attachment 325818 [details] [diff] [review]
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.
Smokey: wondering if you could take a minute to try that build out?
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).
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.
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.
Yeah, enabling the crashreporter in application.ini fixed the launching; however, after all that, it looks like the build only has the English crashreporter :(
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.
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.
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.

Created attachment 326966 [details] [diff] [review]
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.
Attachment #325818 - Attachment is obsolete: true
Attachment #326966 - Flags: review?(dcamp)
(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
foo@bar.com
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.

Updated

11 years ago
Attachment #326966 - Flags: review?(dcamp) → review+
Comment on attachment 326966 [details] [diff] [review]
mac patch, fix rtl entry in comment box [checked in]

Pushed to mozilla-central in 76d3bd6c0505.
Attachment #326966 - Attachment description: mac patch, fix rtl entry in comment box → mac patch, fix rtl entry in comment box [checked in]
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.

Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Attachment #310484 - Flags: approval1.9.0.2?
Attachment #325684 - Flags: approval1.9.0.2?
Attachment #326966 - Flags: approval1.9.0.2?
Ted, can you please do a roll-up patch of all three patches and request approval on it?
Whiteboard: [needs rolls up patch]
Attachment #310484 - Flags: approval1.9.0.2? → approval1.9.0.3?
Attachment #325684 - Flags: approval1.9.0.2? → approval1.9.0.3?
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.
Attachment #326966 - Flags: approval1.9.0.2? → approval1.9.0.3?
We're opting not to take this in a maintenance release. It will be fixed in Firefox 3.1.
Flags: wanted1.9.0.x+ → wanted1.9.0.x-
Attachment #310484 - Flags: approval1.9.0.4? → approval1.9.0.4-
Attachment #325684 - Flags: approval1.9.0.4? → approval1.9.0.4-
Attachment #326966 - Flags: approval1.9.0.4? → approval1.9.0.4-
Blocks: 459412

Updated

10 years ago
No longer blocks: 415597

Updated

10 years ago
Blocks: 285718
You need to log in before you can comment on or make changes to this bug.