http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsBidiKeyboard.mm I'll fix it after bug 513952

Would be nice to fix this but the file already has 64-bit ifdefs and a comment so this bug does not block 64-bit builds.

Simon, would you review this too? Arabic ("ar"), Persian ("fa"), Hebrew ("he") and Yiddish ("yi") are RTL. Is there other RTL languages?

> ------- Comment #2 From Jonathan Kew 2009-04-27 22:33:04 PDT (-) [reply] ------- > > This will not be a perfect solution, as (a) it will be difficult to come up > with a definitive list of RTL languages, and (b) keyboard layouts need not > necessarily provide this property anyway. But it could probably work for the > most common situations, at least. On Windows and On Linux, the class only checks whether the selected keyboard layout's language is for RTL or not. So, the solution isn't perfect, but it's enough. Note that we're going to be able to use CFLocaleGetLanguageCharacterDirection or CFLocaleGetLanguageLineDirection after we drop 10.5 support.

Some other RTL languages are: Pashto (ps) Urdu (ur) Uzbek (uz) Uighur (ug) Except for Urdu, I have keyboard layouts available for all of those on my Mac 10.5 system, also for Jawi, which returns the language code ms-Arab. That is a good example of why using language codes to determine text direction is not 100% reliable. There are several languages that can be written in different scripts -- others are Kurdish, Punjabi, and several languages from the former Soviet republics in Central Asia. I also have a custom Hebrew layout written for earlier OS X versions which doesn't return any language code (see bug 434261 comment 2 for why I use this instead of Apple's Hebrew keyboard), so the patch causes a regression with this layout.

Comment on attachment 403953 [details] [diff] [review] Patch v1.0 I will be OK with this if you add the additional 2-letter language codes from the previous comment and change the #ifdef back to __LP64__, so as not to regress use cases that are working already. I think it would be overkill to add extra parsing for the "ms-Arab" case, which doesn't work in the current code either.

Um... O.K... Simon, can we check whether the layout is for RTL or not by checking the actual input characters? We can get uchr resource, so, we can get the actual characters by UCKeyTranslate. Or, Are the some characters also used in LTR language?

Do you mean check for RTL layout on key press? That's not the way it's supposed to work: the idea of the directional hook on the caret is to give the user a hint to the active keyboard *before* s/he starts typing.

(In reply to comment #7) > Do you mean check for RTL layout on key press? No, we can always check the translated character of any keys. E.g., we can check 'A' key's character of the current keyboard layout when nsBidiKeyboard::IsLangRTL is called. Can we decide the result by the charcter(s)? If it inputs Arabic or Hebrew character, we can decide the layout is for RTL language.

Yes, I guess that will work. You can use the UCS2_CHAR_IS_BIDI macro from nsBidiUtils.h (Do we support supplementary characters in keyboard input on OS X? If so, it will be enough to add a test if the first UTF-16 character is 0xD802 or 0xD803 to cover the range U+10800-U+10FFF in UTF-16)

Thank you, Simon. This should work fine. I need to clean-up the patch. I'll request the review after that.

O.K. This should work fine. nsTISInputSource::IsForRTLLanguage() gets the character that is inputted by the 'A' key with ANSI keyboard (physical keyboard type, not the keyboard layout on the OS). And checks the first character range by the Simon's advice.

Do you really need the nsAString argument to TranslateToString and UCKeyTranslateToString? We only care about the first character, so why not just return chars[0] in a PRUnichar? Are you expecting other callers in the future?

(In reply to comment #12) > Are you expecting other callers in the future? Yes. nsKeyEvent and nsAlternativeCharCode use PRUint32 for the charCode, I think that we should support non-BMP characters on Mac key events. # So, PRUint32 might be enough for the result, but it's not useful for other things.

Comment on attachment 405876 [details] [diff] [review] Patch v2.2 Josh should probably review too.

Comment on attachment 405876 [details] [diff] [review] Patch v2.2 >diff -r 11b21449d195 widget/src/cocoa/nsBidiKeyboard.mm ... >+#include "nsCocoaUtils.h" Do you really need to include nsCocoaUtils.h here? Why? >+ enum { >+ eKbdType_ANSI = 40 >+ }; Please add a comment explaining where this constant comes from.

(In reply to comment #15) > (From update of attachment 405876 [details] [diff] [review]) > >diff -r 11b21449d195 widget/src/cocoa/nsBidiKeyboard.mm > ... > >+#include "nsCocoaUtils.h" > > Do you really need to include nsCocoaUtils.h here? Why? NS_LEOPARD_AND_LATER is defined in nsCocoaUtils.h.