As Bug 272096 (Disable bidi caret on platforms without nsBidiKeyboard::IsLangRTL impl') haven't fixed on 1.8 branch, I want to add GTK2 backend for nsBidiKeyboard to fix for 1.8 branch, to get bidi caret works well with keyboard layouts.

Comment on attachment 233761 [details] [diff] [review] Patch for 1.8 branch >+ if (gdk_keymap_get_direction(NULL) == PANGO_DIRECTION_RTL) >+ *aIsRTL = PR_TRUE; >+ else >+ *aIsRTL = PR_FALSE; Make this *aIsRTL = (gdk_keymap_get_direction(NULL) == PANGO_DIRECTION_RTL); With that, r=smontagu

transferring smontagu's r+

Comment on attachment 233766 [details] [diff] [review] Patch for 1.8 branch, addressed smontagu's review comment Testing bug 348724, I discovered that gdk_keymap_get_direction() isn't very accurate: it returns PANGO_DIRECTION_LTR for the Hebrew keyboard in my distro, which says it is $XKeyboardConfig: xkbdesc/symbols/il,v 1.9 2005/08/01 22:11:07 svu Exp $

Simon, does that mean we shouldn't take this?

Comment on attachment 233766 [details] [diff] [review] Patch for 1.8 branch, addressed smontagu's review comment Well, I guess since it's only intended as a stopgap for the branch, it's still better than what we have now, especially since the branch didn't get bug 288839. We have to work out how to Do It Right in bug 348724.

Comment on attachment 233766 [details] [diff] [review] Patch for 1.8 branch, addressed smontagu's review comment Thanks Simon and Robert for reviews. In fact, the current caret is incorrect for RTL languages. As the bidi caret haven't turned off for GTK back-end, this patch is necessary for branch to correct the caret's appearance.

Comment on attachment 233766 [details] [diff] [review] Patch for 1.8 branch, addressed smontagu's review comment a=beltzner on behalf of drivers, please land on MOZILLA_1_8_BRANCH and mark fixed1.8.1

Did you mean to change the TM? :)

Hey, I was just trying to follow the procedure specified on tinderbox and get the bug on the b2 blocker list. If bugs don't need to be on that list to be checked in the tinderbox page needs updating.

As per schrep's TM change on Aug 21st, we're not going to respin for this bug alone, as much as it sucks. I'll relnote it if it doesn't make it into beta2.

(In reply to comment #10) > Hey, I was just trying to follow the procedure specified on tinderbox and get > the bug on the b2 blocker list. If bugs don't need to be on that list to be > checked in the tinderbox page needs updating. Oops. Updated. Thanks.

Checked in to MOZILLA_1_8_BRANCH

Unfortunately, current patch doesn't check for GTK version, and the function had been added on version 2.6. So the patch prevents Mozilla to be compiled with GTK < 2.6. I'm going to attach a new patch and add the GTK version checking. But as GTK's implementation of layout direction has problems with Hebrew, we want to implement it ourselves. If I didn't attach the patch with new implementation, please check the version-checking patch.

Seems the function have been on GTK+ since more than 4 years ago, but before 2.6 it used to strcmp the script name, and from 2.6, it's checking the codes of the layout. Here are the patch to check the GTK+ version, but looks like nothing breaks without this patch. I'm going to implement the better solution (checking the first level only, or checking based on the current level) soon.

That looks like a build-time check. Don't we really want a runtime check?

Every version of GTK+ has problems in the implementation. The old one doesn't support Persian, and the new one doesn't support Hebrew. I'm going to attached the fix for GKT+ on gnome-bug 116626. So I suggest to don't use GDK's impl. at all, for now. The XKB-based code for trunk (bug 348724) which works well with both Arabic/Persian and Hebrew is ready. May I make the patch for trunk to get it for 1.8.1? If there's no time to fix this issue, please close this bug as FIXED, as I was FIXED and I REOPENED it to add Hebrew support.

I'll attach the patch for 2.0.0.1 then.

What's the status of this one? Am I right in saying there's a patch checked in already (fixed1.8.1), but you want another one for FF2.0.0.1? Is attachment 236797 [details] [diff] [review] that patch? Realistically this isn't going to block a release, but a fix would be great. If you round up reviews then request approval for 1.8.1.x.

Comment on attachment 236797 [details] [diff] [review] Add GTK+ version cheking It's not necessary.

Mark as FIXED (re-close), as the complete solution is not available, and won't be for 2.0.0.x releases. Please remove "wanted1.8.1.x" flag.

If you come up with a better approach for the remaining issues you should file a new bug to track it. mention the new bug here for reference, but trying to track a new patch in a closed bug is confusing.

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