Closed Bug 433192 Opened 16 years ago Closed 16 years ago

Ctrl+Shift+X doesn't work in text boxes with Arabic/Hebrew/Persian keyboard to switch between RTL/LTR layouts

Categories

(Core :: Widget, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: eyalroz1, Assigned: masayuki)

References

Details

(Keywords: intl, regression, rtl, Whiteboard: [key hell])

Attachments

(3 files, 9 obsolete files)

3.86 KB, patch
Details | Diff | Splinter Review
2.63 KB, patch
Gavin
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
6.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
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...
Blocks: 429510
No longer blocks: 42950
Any idea when this last worked?

US or Hebrew localized build?
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #320409 - Flags: review?(mozbugz)
We must fix this bug for Hebrew users.
Flags: blocking1.9?
Keywords: intl, regression
Whiteboard: [key hell]
Target Milestone: --- → mozilla1.9
Flags: in-testsuite?
(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
Attachment #320433 - Flags: review?(gavin.sharp)
Attachment #320433 - Flags: review?(neil)
Attached patch Patch v2.0 (obsolete) — Splinter Review
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.
Attachment #320409 - Attachment is obsolete: true
Attachment #320435 - Flags: review?(mozbugz)
Attachment #320409 - Flags: review?(mozbugz)
Attached patch Patch v2.1 (obsolete) — Splinter Review
oops, sorry for the spam.
Attachment #320435 - Attachment is obsolete: true
Attachment #320436 - Flags: review?(mozbugz)
Attachment #320435 - Flags: review?(mozbugz)
Comment on attachment 320436 [details] [diff] [review]
Patch v2.1

er, this may have a bug. sorry.
Attachment #320436 - Flags: review?(mozbugz) → review-
Attached patch Patch v2.2 (obsolete) — Splinter Review
Attachment #320436 - Attachment is obsolete: true
Attachment #320438 - Flags: review?(mozbugz)
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.
Attached patch Patch v2.2.1 (v2.2 + tests) (obsolete) — Splinter Review
Attachment #320438 - Attachment is obsolete: true
Attachment #320441 - Flags: review?(mozbugz)
Attachment #320438 - Flags: review?(mozbugz)
> 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.)
Keywords: rtl
Summary: Ctrl+Shift+X doesn't work in text boxes with Hebrew Layout → Ctrl+Shift+X doesn't work in text boxes with Arabic/Hebrew/Persian keyboard to switch between RTL/LTR layouts
Attached patch Patch v2.2.2 (v2.2 + tests) (obsolete) — Splinter Review
adding some tests.
Attachment #320441 - Attachment is obsolete: true
Attachment #320448 - Flags: review?(mozbugz)
Attachment #320441 - Flags: review?(mozbugz)
(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.
Attachment #320448 - Flags: review?(mozbugz) → review-
Attached patch Patch v2.3 (obsolete) — Splinter Review
fix a mistake.
Attachment #320448 - Attachment is obsolete: true
Attachment #320454 - Flags: review?(mozbugz)
Whiteboard: [key hell] → [key hell][RC2?]
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?
Attachment #320454 - Flags: superreview?
Attachment #320454 - Flags: review?(mozbugz)
Attachment #320454 - Flags: review+
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?
Attachment #320470 - Flags: superreview?(roc)
Attachment #320470 - Flags: review?(masayuki)
Attachment #320470 - Flags: review?(gavin.sharp)
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.
Attachment #320433 - Flags: review?(neil) → 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."
Attachment #320470 - Attachment is obsolete: true
Attachment #320477 - Flags: superreview?
Attachment #320477 - Flags: review?(gavin.sharp)
Attachment #320470 - Flags: superreview?(roc)
Attachment #320470 - Flags: review?(masayuki)
Attachment #320470 - Flags: review?(gavin.sharp)
Attachment #320477 - Flags: superreview? → superreview?(roc)
Attachment #320477 - Flags: superreview?(roc) → superreview+
Attachment #320470 - Attachment description: fix without HasSameCharCaseInsensitive → fix without HasSameCharCaseInsensitive and with further simplification
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].
Attachment #320477 - Flags: approval1.9?
Attachment #320477 - Flags: review?(gavin.sharp) → 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.
Attachment #320477 - Flags: approval1.9? → approval1.9+
Flags: blocking1.9? → blocking1.9+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #320433 - Attachment is obsolete: true
Attachment #320433 - Flags: review?(gavin.sharp)
Attachment #320454 - Attachment is obsolete: true
(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.
Whiteboard: [key hell][RC2?] → [key hell]
(In reply to comment #46)
> Can we have an automated test for comment #44?

ok, I'll post the tests.
Attached patch additional testsSplinter Review
Attachment #320548 - Flags: superreview?(roc)
Attachment #320548 - Flags: review?(roc)
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.
Attachment #320548 - Flags: superreview?(roc)
Attachment #320548 - Flags: superreview+
Attachment #320548 - Flags: review?(roc)
Attachment #320548 - Flags: review+
I don't see why you couldn't just land them in CVS now.
Roc or someone:

looks like the tree is opened now. but I cannot commit it to tree now, would you do it?
Attachment #320477 - Attachment description: patch v3 without HasSameCharCaseInsensitive → patch v3 without HasSameCharCaseInsensitive [checked in]
Attachment #320473 - Attachment description: tests → tests [checked in]
You need to log in before you can comment on or make changes to this bug.