I use the US English and Hebrew keyboard layouts on Windows. Ctrl+Shift+X, which switches the direction of a textbox between RTL and LTR, now only works in US English, not in Hebrew. Tried a new profile, same thing. To reproduce, just use one of the textboxes in a bugzilla bug page...

Any idea when this last worked? US or Hebrew localized build?

Interesting. We have two event handlers: 1. nsXBLEventHandler 2. nsXBLWindowKeyHandler And the only 2 has Ctrl+Shift+X. But 1 has Ctrl+X for "Cut". On the Hebrew keyboard layout's Latin alphabets are not paired on a key. Therefore, we append shift ignorable shortcut candidate in nsContentUtils::GetAccelKeyCandidates. So, we need to check whether the alternative character is case changeable character and it is same as original charCode.

We must fix this bug for Hebrew users.

(In reply to comment #2) > We have two event handlers: > 1. nsXBLEventHandler > 2. nsXBLWindowKeyHandler > > And the only 2 has Ctrl+Shift+X. But 1 has Ctrl+X for "Cut". On the Hebrew > keyboard layout's Latin alphabets are not paired on a key. Therefore, we append > shift ignorable shortcut candidate in nsContentUtils::GetAccelKeyCandidates. I meant that the event is consumed as Ctrl+X in nsXBLEventHandler. Therefore, text direction switcher is never called from nsXBLWindowKeyHandler.

Maybe we should do the first "exact match" pass in both event handlers and then do the second pass in both event handlers?

(In reply to comment #5) > Maybe we should do the first "exact match" pass in both event handlers and then > do the second pass in both event handlers? It is true. However, such conflict case should not be there.

And also such strictly fix is pretty risky, now. We should do it after 1.9.

If we need to fix this issue ideally, we need to change |nsEventTargetChainItem::HandleEventTargetChain| at keypress event. This loop should run *each* shortcut candidates. But that means keypress event is fired many times. So, I don't have idea for the ideally fix.

The cut command may be defined here: http://mxr.mozilla.org/mozilla/source/content/xbl/builtin/browser-base.inc and this is included by: http://mxr.mozilla.org/mozilla/source/content/xbl/builtin/win/platformHTMLBindings.xml and this binds to input and textarea: http://mxr.mozilla.org/mozilla/source/layout/style/forms.css#101 I think that the non-exact match candidate should be also consumed by the inner element. So, I think that we don't need to change the current our strategy. The key conflict might be occurred, but that is too rare case. And then, the inner element's binding should beat the outer element's handlers.

I don't know enough about bidi editing to say whether or not this should be considered for blocking, so I'm asking in bug 415606 for input. This command is available in the edit menu when the system locale is Hebrew and also when the bidi.browser.ui pref is true. When the command is available in the menu, "Alt+ע כ" and "<Menu> כ" shortcuts are available for Hebrew localizations but unfortunately these only activate the command when the keyboard is in Hebrew layout, so the shortcut would be different depending on whether Hebrew or US layout is currently selected. ("Alt+e w" or "<Menu> w" activates the command from any layout for en builds.)

Please note that this bug is more

Comment on attachment 320409 [details] [diff] [review] Patch v1.0 if (ch == unshiftCh || (IS_IN_BMP(ch) && IS_IN_BMP(unshiftCh) && ToLowerCase(PRUnichar(ch)) == ToLowerCase(PRUnichar(unshiftCh)))) continue; + if (lowerCharCode != upperCharCode && + (lowerCharCode == ch || upperCharCode == ch)) + continue; Is there a reason why you only compare with charCode, rather than checking whether each alternative character is a bicameral letter? If a test for case variability were performed on every character, then the logic in the previous conditional could also be simplified. We could fix this in nsContentUtils::GetAccelKeyCandidates (as you have suggested) but I have another suggestion.

This is my understanding of the problem: There is a conflict between the bindings for cmd_switchTextDirection (Ctrl+Shift+X) and cmd_cut (Ctrl+X) in a keyboard layout (Hebrew) where X is only available with Shift. (We advertise Ctrl+X as the shortcut for Cut in the menu, and the only way to type an X while keeping the keyboard in a Hebrew layout is to hold down shift. It would therefore not be surprising if some users tried to Cut by using Ctrl+Shift+X. However, this particular shortcut is well enough known that most [almost all] users would not hold down shift when trying to Cut.) This conflict would be resolved in favour of cmd_switchTextDirection (as the handler is defined with Shift in "modifiers") if the handlers for these commands were defined on the same element. IMO a big part of the problem is that the cmd_switchTextDirection handler is only defined on the window. cmd_cut is defined on editable elements, and takes priority due to being on the innermost element. It seems that a cmd_switchTextDirection handler should be defined on editable elements (as that is where the shortcut has effect) and doing so would resolve this issue. (This bug does not exist on Linux because on that platform XBL handlers are not defined for cut on editable elements, but instead nsINativeKeyBindings is used. This bug does not exist on Mac because the Hebrew layout on that platform is different.)

This defines handlers for cmd_switchTextDirection on editable elements in the same way as cmd_cut handlers are defined on such elements. I haven't thoroughly tested, sorry, but builds should be available here in about an hour: https://build.mozilla.org/tryserver-builds/2008-05-11_07:37-ktomlinson@mozilla.com-switch-text-direction/ I'm assuming that &bidiSwitchTextDirectionItem.commandkey; can't be used in platformHTMLBindings.xml because no other bindings there are localized. If this binding is ever localized then the localized binding would still exist on the browser window. editorOverlay defines a (empty) key_cut key but I don't know enough about this to know whether it should also define a key_switchTextDirection: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/ui/composer/content/editorOverlay.xul&rev=1.257&mark=71#57

Karl: Even if we use your approach, this bug is really a bug. Because we should not ignore the shift state at that time. Ctrl+Shift+X should not execute cmd_cut on Hebrew layout on Windows.

oops, sorry for the spam.

Comment on attachment 320436 [details] [diff] [review] Patch v2.1 er, this may have a bug. sorry.

Is there any kind of user workaround (e.g. content menu, etc). How often is RTL-LTR switching done for edit boxes?

Also is this something we can add to the test suite?

(In reply to comment #20) > Also is this something we can add to the test suite? now, I'm checking the test, please wait a moment.

(In reply to comment #1) > Any idea when this last worked? > US or Hebrew localized build? Has anyone answered these questions? Drivers will need to make a decision based on impact, and I haven't seen a clear statement about if this is: - only Hebrew keyboard layouts on non-localized builds, - affecting more keyboard layouts on non-localized builds, - something that can be worked around, - something that can be fixed on branch.

(In reply to comment #22) > - only Hebrew keyboard layouts on non-localized builds, This is only on Hebrew kayboard layout, but I'm not sure whether the Hebrew localized build localizes the key. > - affecting more keyboard layouts on non-localized builds, My patch affects to all keyboard layout. If that has a bug. > - something that can be worked around, I think that this is an important usability issue for the keyboard shortcut users. Even if there is another way for accessing to cmd_switchTextDirection, it is uncomfortable for the shortcut key users. > - something that can be fixed on branch. Yes, we can. (In reply to comment #19) > How often is RTL-LTR switching done for edit boxes? Smontagu should know the answer.

> How often is RTL-LTR switching done for edit boxes? Very. Let's say 30% as often as cutting and pasting. > (In reply to comment #1) > Any idea when this last worked? Up until 429510 started manifesting. > US or Hebrew localized build? US build. (In reply to comment #22) This is pretty major bustage for Hebrew, Arabic, Farsi layout users (who use the shortcut rather than the menus or context menus, which I suspect is the majority).

(In reply to comment #24) > Created an attachment (id=320441) [details] > Patch v2.2.1 (v2.2 + tests) > Masayuki/Karlt what is the regression risk of this patch?

(In reply to comment #25) > This is pretty major bustage for Hebrew, Arabic, Farsi layout users (who use > the shortcut rather than the menus or context menus, which I suspect is the > majority). Confirming this for Persian users. Ctrl+Shift+X is a very useful shortcut which Persian users use, and in fact is quite important, because some of the most often used text boxes (such as those of web mail services) need to change from LTR to RTL when for example I want to type in a Persian email. I think 1.9 should have this fixed for the three RTL locales (especially that users expect this to work even in non-ar/fa/he builds as well.)

adding some tests.

(In reply to comment #26) > (In reply to comment #24) > > Created an attachment (id=320441) [details] [details] > > Patch v2.2.1 (v2.2 + tests) > > > > Masayuki/Karlt what is the regression risk of this patch? I think the risk of the patch is low. Because that only appends one limitation for appending a shortcut candidate key combination that ignores the Shift key state. If non-alphabet key is pressed and that cannot be inputted without shift key, we need to ignore the shift key. E.g., Ctrl+'+' is Ctrl+Shift+';' on Japanese keyboard layout. Then, by ignoring the Shift key state, we can execute the Ctrl+'+' command. If there is a regression, this is broken. But I added the test on the latest testcases. I don't have the other ideas for the possibility of the regressions, now.

(In reply to comment #27) > (In reply to comment #25) > > This is pretty major bustage for Hebrew, Arabic, Farsi layout users (who use > > the shortcut rather than the menus or context menus, which I suspect is the > > majority). > > Confirming this for Persian users. Ctrl+Shift+X is a very useful shortcut > which Persian users use, and in fact is quite important, because some of the > most often used text boxes (such as those of web mail services) need to change > from LTR to RTL when for example I want to type in a Persian email. I think > 1.9 should have this fixed for the three RTL locales (especially that users > expect this to work even in non-ar/fa/he builds as well.) Really? Can you reproduce this bug with Persian/Arabic layout?? This bug should be reproduced with Persian and Arabic keyboard layout...

(In reply to comment #30) > This bug should be reproduced with Persian and Arabic keyboard layout... oops, I meant: this bug should *not* be reproduced with Persian and Arabic keyboard layouts...

Comment on attachment 320448 [details] [diff] [review] Patch v2.2.2 (v2.2 + tests) I found a mistake.

fix a mistake.

Comment on attachment 320454 [details] [diff] [review] Patch v2.3 For the sake of Hebrew/Farsi users, I think we should try to jam this into rc1 if we can. It sounds like the current behavior is not expected and inconvenient, and Masayuki's tests cover the area for which we would be most concerned about regressions. (Thank you!) (I haven't reproduced on Arabic layout, but there are several different Arabic layouts and I've only tried Saudi.) With this patch, we don't need attachment 320433 [details] [diff] [review] for rc1 or 1.9. Anyone able to sr soon?

Comment on attachment 320454 [details] [diff] [review] Patch v2.3 + if (IsCaseChangeableChar(ch) && + HasSameCharCaseInsensitive(ch, aCandidates)) The HasSameCharCaseInsensitive looks unnecessary. Isn't this always true because we've already done nsShortcutCandidate key(nativeKeyEvent->charCode, PR_FALSE); and when ch != nativeKeyEvent->charCode nsShortcutCandidate key(ch, PR_FALSE);

(In reply to comment #34) > (From update of attachment 320454 [details] [diff] [review]) > For the sake of Hebrew/Farsi users, I think we should try > to jam this into rc1 if we can. It sounds like the current > behavior is not expected and inconvenient, and Masayuki's tests > cover the area for which we would be most concerned about regressions. (Thank > you!) > > (I haven't reproduced on Arabic layout, but there are several > different Arabic layouts and I've only tried Saudi.) > > With this patch, we don't need attachment 320433 [details] [diff] [review] for rc1 or 1.9. > > Anyone able to sr soon? > You didn't target the SR - who did you want to SR? Can we get an updated version of the patch?

Changes based on comment 35. I think this is all we need but attachment 320454 [details] [diff] [review] is safe (just with some unnecessary code). If we can't find anyone to review this then we could go with attachment 320454 [details] [diff] [review]. I'm assuming Masayuki is asleep. Gavin do you feel up to reviewing this? roc are you around?

Comment on attachment 320433 [details] [diff] [review] Define handlers for cmd_switchTextDirection on editable elements >Index: content/xbl/builtin/browser-base.inc >=================================================================== >RCS file: /cvsroot/mozilla/content/xbl/builtin/browser-base.inc,v >retrieving revision 1.3 >diff -u -p -U 8 -r1.3 browser-base.inc >--- content/xbl/builtin/browser-base.inc 2 Sep 2005 15:54:18 -0000 1.3 >+++ content/xbl/builtin/browser-base.inc 11 May 2008 14:34:11 -0000 >@@ -24,10 +24,12 @@ > > <handler event="keypress" key="x" command="cmd_cut" modifiers="accel"/> > <handler event="keypress" key="c" command="cmd_copy" modifiers="accel"/> > <handler event="keypress" key="v" command="cmd_paste" modifiers="accel"/> > <handler event="keypress" key="z" command="cmd_undo" modifiers="accel"/> > <handler event="keypress" key="z" command="cmd_redo" modifiers="accel,shift" /> > <handler event="keypress" key="a" command="cmd_selectAll" modifiers="accel"/> > >+ <handler event="keypress" key="x" modifiers="accel,shift" command="cmd_switchTextDirection"/> >+ JFTR I don't think this one was necessary. r=me in the unlikely event that this patch is preferred by drivers even though it hardcodes the accel+shift+x key.

The tests (only) from attachment 320454 [details] [diff] [review].

Removing HasSameCharCaseInsensitive without the extra simplification because I couldn't convince people that "If there are _not_ distinct upper and lower case forms of ch, then ToLowerCase(ch) has no effect. Otherwise ToLowerCase and ToUpperCase would need to both change ch and both change ch to the same character."

Comment on attachment 320477 [details] [diff] [review] patch v3 without HasSameCharCaseInsensitive [checked in] Request a1.9 conditional on gavin's review (which sounded likely on #developers) and landing with test cases in attachment 320473 [details] [diff] [review].

Comment on attachment 320477 [details] [diff] [review] patch v3 without HasSameCharCaseInsensitive [checked in] a+ schrep please land immediately + a clobber. If the builds go clean we'll kickoff RC1.

(In reply to comment #29) [...] > I think the risk of the patch is low. Because that only appends one limitation > for appending a shortcut candidate key combination that ignores the Shift key > state. > > If non-alphabet key is pressed and that cannot be inputted without shift key, > we need to ignore the shift key. E.g., Ctrl+'+' is Ctrl+Shift+';' on Japanese > keyboard layout. Then, by ignoring the Shift key state, we can execute the > Ctrl+'+' command. If there is a regression, this is broken. But I added the > test on the latest testcases. > > I don't have the other ideas for the possibility of the regressions, now. > Wouldn't this regress the recently fixed bug for fr-FR keyboards, where 6 is Shift+- so that distinguishing between Ctrl+- (decrease font size) and Ctrl+6 (Firefox: go to tab 6, SeaMonkey: go to Chatzilla window) requires, on that layout, not ignoring the Shift status even when Ctrl is depressed?

(In reply to comment #35) > (From update of attachment 320454 [details] [diff] [review]) > + if (IsCaseChangeableChar(ch) && > + HasSameCharCaseInsensitive(ch, aCandidates)) > > The HasSameCharCaseInsensitive looks unnecessary. Isn't this always true > because we've already done Oh, right! Thank you! (In reply to comment #44) > Wouldn't this regress the recently fixed bug for fr-FR keyboards, where 6 is > Shift+- so that distinguishing between Ctrl+- (decrease font size) and Ctrl+6 > (Firefox: go to tab 6, SeaMonkey: go to Chatzilla window) requires, on that > layout, not ignoring the Shift status even when Ctrl is depressed? The behavior is a special case for AZERTY layout at *unshifted* key pressing. Don't worry the regression for that. I confirmed that is not regressed. Ctrl+Shift+'-': switching the active tab to 6th. Ctrl+'-': zoom-out.

Can we have an automated test for comment #44?

(In reply to comment #46) > Can we have an automated test for comment #44? ok, I'll post the tests.

Comment on attachment 320548 [details] [diff] [review] additional tests excellent. I guess we won't be able to land these until the mozilla-central tree opens.

I don't see why you couldn't just land them in CVS now.

There is a code change.

Roc or someone: looks like the tree is opened now. but I cannot commit it to tree now, would you do it?