Last Comment Bug 414130 - can not type ZWNJ with Ctrl+Shift+2 combination
: can not type ZWNJ with Ctrl+Shift+2 combination
Status: VERIFIED FIXED
fixed by bug 359638, [key hell]
: regression
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: P2 normal with 1 vote (vote)
: mozilla1.9
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Jim Mathies [:jimm]
Mentors:
Depends on: 359638 416562
Blocks: 287179 Persian-Fx3.5
  Show dependency treegraph
 
Reported: 2008-01-26 08:03 PST by Mohammad Alinia
Modified: 2013-04-18 19:47 PDT (History)
22 users (show)
pavlov: blocking1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow Ctrl+Shift+num and Alt+Shift+num to return keyboard layout data (2.90 KB, patch)
2008-01-29 14:32 PST, Dainis Jonitis
emaijala+moz: review+
Details | Diff | Splinter Review

Description Mohammad Alinia 2008-01-26 08:03:15 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

When trying to type ZWNJ in Persian (Which its keyboard combination in MS Windows XP is Ctrl+Shift+2) in forms and search bar, instead of putting the character Firefox Changes the focus to the second tab. If there is no second tab or if you are already in second tab nothing happens (and the character is not put). 

Reproducible: Always

Steps to Reproduce:
1.Go to search bar.
2.Change keyboard language to Farsi.
3.Try using ZWNJ by using Ctrl+Shift+2.
Actual Results:  
I could not put ZWNJ in the text.

Expected Results:  
It should have inserted the ZWNJ character where I pressed the combination Ctrl+Shift+2.
Comment 1 Kevin Brosnan 2008-01-26 08:12:24 PST
CCing a Persian localizer.
Comment 2 :Gijs Kruitbosch 2008-01-26 08:34:18 PST
Marking as a dupe because, while not exactly the same issue per the summary, the general problem is the same (we don't check for modifiers carefully enough and eat keypresses that are intended to be interpreted by the OS). The patch in bug 403387 should also fix this issue.

*** This bug has been marked as a duplicate of bug 403387 ***
Comment 3 :Ehsan Akhgari 2008-01-26 08:58:28 PST
Verified as a dupe.
Comment 4 :Ehsan Akhgari 2008-01-27 10:42:28 PST
Reopened as per bug 403387 comment 19.  The steps in comment 0 don't lead to ZWNJ being typed, bug with the fix from bug 403387, the second tab is not selected either.

This is a major issue for Persian users at least, therefore, requesting blocking.
Comment 5 Mohammad Alinia 2008-01-27 12:08:47 PST
This is not a clean solution, but for now I inserted

  if (event.ctrlKey && event.shiftKey && event.charCode == 50)
  {
    var e = document.commandDispatcher.focusedElement;
    if (e instanceof HTMLInputElement)
    {
      if (e.type == "text" || e.type == "textarea")
      {
        e.value += String.fromCharCode("\u200C");
      }
    }
  }

just above the fix from bug 403387.
Comment 6 :Ehsan Akhgari 2008-01-27 12:47:31 PST
I doubt that solution is acceptable.  Firstly, it obviously doesn't handle the case when a ZWNJ is added at the middle of the text.  Secondly, it's emulating a feature provided by the keyboard layout driver from the OS in the UI code, which is not acceptable.

Please note that Ctrl+Shift+2 is a workaround implemented by Microsoft for the Persian keyboards coming with Microsoft operating systems, but in the standard keyboard layout issued by ISIRI (Institute of Standards & Industrial Research of Iran) the combination to enter ZWNJ is Shift+Space.  The above code does not consider whether the installed keyboard layout is the standards-based one or the Microsoft provided one.

I suspect that the problem is caused by Firefox somehow "eating" the event at some level, but I'm unclear about how to further debug this.  I'm CCing some people which would hopefully be able to provide more insight.
Comment 7 :Ehsan Akhgari 2008-01-29 00:00:23 PST
Hmmm, I guess this regressed from bug 287179...  Could someone look into this further?
Comment 8 :Ehsan Akhgari 2008-01-29 00:02:21 PST
Moving to Core :: Widgets: Win32
Comment 9 :Ehsan Akhgari 2008-01-29 00:03:16 PST
Re-requesting blocking as per comment 4
Comment 10 Dainis Jonitis 2008-01-29 01:03:05 PST
I have not looked in the details yet, but I guess the code in win32 nsWindow::OnKeyDown is responsible for this _desired_ behaviour:

if (mIsControlDown ^ mIsAltDown)
{
  // XXX
  // For both Alt+key and Ctrl+key combinations we return the latin characters A..Z and
  // numbers 0..9, ignoring the real characters returned by active keyboard layout.
  // This is required to make sure that all shortcut keys (e.g. Ctrl+c, Ctrl+1, Alt+f)
  // work the same way no matter what keyboard layout you are using.
  
  if ((NS_VK_0 <= DOMKeyCode && DOMKeyCode <= NS_VK_9) ||
      (NS_VK_A <= DOMKeyCode && DOMKeyCode <= NS_VK_Z))

The keyboard layout does have information that Ctrl+Shift+2 should return ZWNJ, but Mozilla code is ignoring it to make standard shortcuts (like Ctrl+C) to work with Russian and similar localized versions when Cyrillic keyboard locale is active.
  We are using real keyboard layout returned data only for combinations when _both_ Ctrl and Alt keys are pressed together with some character or number.

You can not have both right. Unfortunately
Comment 11 :Ehsan Akhgari 2008-01-29 02:48:09 PST
The code currently checks for the state of Ctrl and Alt keys.  Can't this be changed to check the Unicode character code received from the OS as well?  For example, make the code check for the real virtual key code entered, and check if it's different to the physical key code?

Also, this seems to affect more Ctrl+Shift+{1-4} in the Farsi keyboard provided by Microsoft: <http://www.microsoft.com/globaldev/keyboards/kbdfa.htm>.  (Ugh!  That page can't be viewed with Firefox :( )
Comment 12 Dainis Jonitis 2008-01-29 12:37:24 PST
We can try to slightly relax the rules when layout independent Latin characters should be returned and when real data from the current keyboard layout.

I may guess that for numbers 0..9 in combination with Ctrl+Shift or Alt+Shift we can return keyboard layout data in hope that no shortcut in any Mozilla application or extension use Ctrl+Shift+Number or Alt+Shift+Number shortcuts.

What user interface guys at Mozilla think about it? Is there any complete list of all used shortcuts in SeaMonkey / Firefox / Thunderbird / Sunbird?
Ere, what is your opinion on this?
Comment 13 :Ehsan Akhgari 2008-01-29 12:48:34 PST
I'm CCing Beltzner on this.  Mconnor is already CCed.  I'm not sure who to CC for SeaMonkey, so I'm throwing this off to Kairo. :-)
Comment 14 Dainis Jonitis 2008-01-29 13:15:57 PST
What about Ctrl+0 combination which produces 0x06C1 (ARABIC LETTER HEH GOAL)?
How important is this character for Arabic language? For Ctrl+'0' we will always return '0' and Ctrl flag set.
Comment 15 :Ehsan Akhgari 2008-01-29 13:19:07 PST
(In reply to comment #14)
> What about Ctrl+0 combination which produces 0x06C1 (ARABIC LETTER HEH GOAL)?
> How important is this character for Arabic language? For Ctrl+'0' we will
> always return '0' and Ctrl flag set.

I'm not really sure.  CCing an Arabic localizer...
Comment 16 Dainis Jonitis 2008-01-29 14:32:07 PST
Created attachment 300143 [details] [diff] [review]
Allow Ctrl+Shift+num and Alt+Shift+num to return keyboard layout data
Comment 17 Dainis Jonitis 2008-01-29 14:35:39 PST
Attached a patch that would allow Ctrl+Shift+num and Alt+Shift+num combinations to read real data from current keyboard layout. Would appreciate if localizers test it with different languages to see whether we do not break anything.
Comment 18 :Ehsan Akhgari 2008-01-29 21:25:12 PST
Asking localizers to test the patch in attachment 300143 [details] [diff] [review] as per comment 17.
Comment 19 Ere Maijala (slow) 2008-01-30 00:57:26 PST
(In reply to comment #12)
> Ere, what is your opinion on this?

Well, looking at it from the widget side I don't really have an opinion -- we can return whatever the higher levels require. 

Perhaps we could implement a new message that would be sent first with the layout-independent character and if it's not consumed we could continue with the layout-dependent character?
Comment 20 Ere Maijala (slow) 2008-01-30 00:58:34 PST
The first message would of course be for shortcut handling only.
Comment 21 Axel Hecht [:Pike] 2008-01-30 01:55:25 PST
Can we get some try-server builds with that patch, then?
http://wiki.mozilla.org/Build:TryServer
Comment 22 Dainis Jonitis 2008-01-30 03:42:00 PST
I do not have CVS account. Someone else should commit a patch to TryServer
Comment 23 :Ehsan Akhgari 2008-01-30 09:45:46 PST
I tested this patch, and it solves the issue in comment 0, as well as the other Ctrl+Shift combinations for Persian.
Comment 24 :Gijs Kruitbosch 2008-01-30 10:05:35 PST
I fired off a tryserver build with the patch. I have no idea how long it takes for  the builds to come available, however. I will try to keep a lookout for them and post a link when they arrive.
Comment 25 :Ehsan Akhgari 2008-01-30 10:53:50 PST
(In reply to comment #24)
> I fired off a tryserver build with the patch. I have no idea how long it takes
> for  the builds to come available, however. I will try to keep a lookout for
> them and post a link when they arrive.

Thanks Gijs for the help.  The builds will be available at:
<https://build.mozilla.org/tryserver-builds/2008-01-30_10:02-gijskruitbosch@gmail.com-allow-modifier-keypresses-nsWindow_cpp/>

Currently, the Linux build is finished.  Other platform builds will appear in the above URL in about 1-2 hours I guess.

It would be great if localizers test these builds and report the results here.  In particular, Behnam and Mohammad: please take some time to test these.

Thanks!
Comment 26 :Gijs Kruitbosch 2008-01-30 11:00:22 PST
(In reply to comment #25)
> (In reply to comment #24)
> > I fired off a tryserver build with the patch. I have no idea how long it takes
> > for  the builds to come available, however. I will try to keep a lookout for
> > them and post a link when they arrive.
> 
> Thanks Gijs for the help.  The builds will be available at:
> <https://build.mozilla.org/tryserver-builds/2008-01-30_10:02-gijskruitbosch@gmail.com-allow-modifier-keypresses-nsWindow_cpp/>
> 
> Currently, the Linux build is finished.  Other platform builds will appear in
> the above URL in about 1-2 hours I guess.

Actually, no. The Linux and Mac builds will be the same as normal trunk builds, given the patch is Windows only. So it's no use testing those. The Windows build failed (http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry ). I'm trying to resolve the issue, and it looks like the current build should work. I'm holding off on posting URLs until I'm sure, though.
Comment 27 :Ehsan Akhgari 2008-01-30 11:08:54 PST
Oops, sorry for the confusion :(

I guess the reason why the build failed was that the patch should be applied in /mozilla/widget/src/windows instead of /mozila...
Comment 29 Mohammad Alinia 2008-01-31 05:08:32 PST
This build is working perfectly for me, I have no problem with inserting ZWNJ anymore.
Comment 30 Mike Schroepfer 2008-02-05 18:45:15 PST
Setting this to P1 since we are messing with keyboard input and I'd like to know sooner rather than later if there are any regressions
Comment 31 Ere Maijala (slow) 2008-02-10 02:40:01 PST
Comment on attachment 300143 [details] [diff] [review]
Allow Ctrl+Shift+num and Alt+Shift+num to return keyboard layout data

Ok, let's try this and revisit the way we do shortcut keys if it's not satisfactory.
Comment 32 :Ehsan Akhgari 2008-02-10 09:32:36 PST
The patch in this build caused bug 416562.  Please see bug 416562 comment 0.  The ui.key.chromeAccess and ui.key.contentAccess prefs may need to be respected here.
Comment 33 Tony Mechelynck [:tonymec] 2008-02-10 09:52:16 PST
(In reply to comment #15)
> (In reply to comment #14)
> > What about Ctrl+0 combination which produces 0x06C1 (ARABIC LETTER HEH GOAL)?
> > How important is this character for Arabic language? For Ctrl+'0' we will
> > always return '0' and Ctrl flag set.
> 
> I'm not really sure.  CCing an Arabic localizer...
> 

According to Unicode chart http://www.unicode.org/charts/PDF/U0600.pdf , that letter is characteristic of the variant of the Arabic alphabet used for writing Urdu (IIUC, the way the "Latin" letter ø is characterisic of Danish, or the "Latin" letter ß of German).

Don't know if this comment is still relevant, or who a Pakistani (Urdu language) localizer would be.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-02-10 15:45:22 PST
Comment on attachment 300143 [details] [diff] [review]
Allow Ctrl+Shift+num and Alt+Shift+num to return keyboard layout data

My understanding is that this patch is not ready to go.
Comment 35 Dainis Jonitis 2008-02-11 13:20:06 PST
The patch in the current form brakes the numeric accelerator keys within web pages   which by default are handled by Alt+Shift+number (ui.key.contentAccess=5). Changing preferences may cause the same bad effect on Ctrl+Shift+number combinations.

My current idea is to tweak cross platform code that is responsible for "accesskey" attribute handling to analyse the key code and if it is range VK_0..VK_9 to actually ignore the returned Unicode character and manually translate it to '0'..'9'. 

Thus for accesskeys where we know for sure that Alt+Shift+number and Ctrl+Shift+number combinations might be used we will still return '0'..'9' (instead of ')', '!', '@' etc.). The accesskey handler for Alt+Shift+1 will be able to intercept keypress even if Win32 keyboard layout will return '!' with Alt and Shift flags set as "accesskey" handler will translate '!' to '1'.

The other keyboard event listeners may still see the real Unicode character returned '!' and use this information for their benefit (e.g. get ZWNJ on Ctrl+Shift+2 in Persian layout). If they do not want keyboard layout to influence their behaviour they also may map key virtual code to symbol. 

The added cross platform code will affect only Win32 as it is the only platform that since bug 287179 is able to return real keyboard layout specific symbols for Ctrl+Shift+num and Alt+Shift+num combinations.

Hopefully it is possible to implement it this way (e.g. get key code for KeyCharCode event).
Any thoughts?
Comment 36 Dainis Jonitis 2008-02-11 14:29:09 PST
nsKeyEvent does not have keyCode for NS_KEY_PRESS events. I think we can't change this historical behaviour. Thus there is no easy way to determine the key virtual code that caused the character to be generated in KEY_PRESS event.

The one solution that I can imagine is to remember the NS_KEY_DOWN key code in nsEventStateManager::PreHandleEvent() and then use remembered key code during NS_KEY_PRESS handling in nsEventStateManager::HandleAccessKey().
Comment 37 :Ehsan Akhgari 2008-02-11 14:32:44 PST
Hmm, roc: do you have any ideas whether this would be feasible and correct?
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-02-11 14:44:26 PST
It sounds ugly. I would rather add fields to the event object if needed.
Comment 39 Ere Maijala (slow) 2008-02-12 04:18:00 PST
I didn't get any comments to my previous suggestion, so I'll just keep firing until I'm shot down: How about a new message for shortcut/accelerator that would be processed first and only then do the normal keypress stuff if it wasn't consumed? 
Comment 40 Dainis Jonitis 2008-02-12 14:55:13 PST
Ere, I understand your idea in general, but I do not think I am able to implement it. Isn't there too much places to change to handle the new accelerator key event? 
Anyone else want to volunteer?

I can try to extend the nsKeyEvent, though.
Comment 41 :Ehsan Akhgari 2008-02-12 20:52:15 PST
I think extending nsKeyEvent is a better idea.  With that, the only piece of code needing change is the access key handling part.  I can volunteer to handle the changes on that side when you're done extending nsKeyEvent.
Comment 42 Ere Maijala (slow) 2008-02-21 09:27:03 PST
I'm fine with extended nsKeyEvent too. I don't see how it's so much easier, though..
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-03-04 12:12:58 PST
(In reply to comment #10)
> I have not looked in the details yet, but I guess the code in win32
> nsWindow::OnKeyDown is responsible for this _desired_ behaviour:
> 
> if (mIsControlDown ^ mIsAltDown)
> {
>   // XXX
>   // For both Alt+key and Ctrl+key combinations we return the latin characters
> A..Z and
>   // numbers 0..9, ignoring the real characters returned by active keyboard
> layout.
>   // This is required to make sure that all shortcut keys (e.g. Ctrl+c, Ctrl+1,
> Alt+f)
>   // work the same way no matter what keyboard layout you are using.
> 
>   if ((NS_VK_0 <= DOMKeyCode && DOMKeyCode <= NS_VK_9) ||
>       (NS_VK_A <= DOMKeyCode && DOMKeyCode <= NS_VK_Z))
> 
> The keyboard layout does have information that Ctrl+Shift+2 should return ZWNJ,
> but Mozilla code is ignoring it to make standard shortcuts (like Ctrl+C) to
> work with Russian and similar localized versions when Cyrillic keyboard locale
> is active.
>   We are using real keyboard layout returned data only for combinations when
> _both_ Ctrl and Alt keys are pressed together with some character or number.
> 
> You can not have both right. Unfortunately

We can remove the code completely in bug 359638. I think charCode should not be overwrote always, right? (I think that other platforms have same problem.)
Comment 44 Ayman Hourieh 2008-03-16 15:11:25 PDT
(In reply to comment #33)
> 
> According to Unicode chart http://www.unicode.org/charts/PDF/U0600.pdf , that
> letter is characteristic of the variant of the Arabic alphabet used for writing
> Urdu (IIUC, the way the "Latin" letter ø is characterisic of Danish, or the
> "Latin" letter ß of German).
> 
> Don't know if this comment is still relevant, or who a Pakistani (Urdu
> language) localizer would be.
> 

No idea about Urdu, but I can confirm that Arabic does not need this character.
Comment 45 Damon Sicore (:damons) 2008-03-17 15:49:22 PDT
Who owns this?
Comment 46 Damon Sicore (:damons) 2008-03-19 14:03:27 PDT
Dainis, are you working on another patch that extends nsKeyEvent?  Just want to make sure this blocker is covered.
Comment 47 Dainis Jonitis 2008-03-20 02:30:14 PDT
No. I am not working on this problem. I think Masayuki Nakano has best solution for this problem and he is working most actively on related keyboard problems that should also cover this case. I think Robert O'Callahan also knows which solution was chosen. See Bug 399939.
Comment 48 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-03-20 03:22:32 PDT
Yeah, the latest patch of bug 359638 fixes this bug probably. 0x200C is inputted by Ctrl+Shift+2 on Windows build with my patch.
Comment 49 Damon Sicore (:damons) 2008-03-20 11:31:37 PDT
Can we please confirm that this problem is fixed?  Is there a test case we could add for this?  Would be great to just get this closed out if it is indeed fixed by bug 359638 or bug 399939.  People don't sound so sure in the two comments above.
Comment 50 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-14 23:18:22 PDT
The patch of bug 359638 was landed.

-> FIXED
Comment 51 :Ehsan Akhgari 2008-04-15 06:55:18 PDT
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041517 Minefield/3.0pre

-> VERIFIED

Masayuki: Thanks for working on bug 359638!  :-)
Comment 52 :Ehsan Akhgari 2008-04-15 07:09:00 PDT
This needs to be tested to make sure it doesn't regress in future builds.  Marking in-litmus?, since I'm not sure how (if) this can be tested automatically.

Note You need to log in before you can comment on or make changes to this bug.