519631 - nsBidiKeyboard.mm is still using KL APIs

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsBidiKeyboard.mm

I'll fix it after bug 513952

Comment 1

[Josh Aas]

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.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Simon, would you review this too?

Arabic ("ar"), Persian ("fa"), Hebrew ("he") and Yiddish ("yi") are RTL. Is there other RTL languages?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 4

[Simon Montagu :smontagu]

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 5

[Simon Montagu :smontagu]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 7

[Simon Montagu :smontagu]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 9

[Simon Montagu :smontagu]

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)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Thank you, Simon. This should work fine.

I need to clean-up the patch. I'll request the review after that.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.2

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.

Comment 12

[Simon Montagu :smontagu]

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?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Simon Montagu :smontagu]

Comment on attachment 405876 [details] [diff] [review]
Patch v2.2

Josh should probably review too.

Comment 15

[Josh Aas]

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.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.3

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/cb5685e57654