414130 - can not type ZWNJ with Ctrl+Shift+2 combination

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Mohammad Alinia]

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]

CCing a Persian localizer.

Comment 2

[:Gijs (out for now; he/him)]

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.

Comment 3

[(no longer active)]

Verified as a dupe.

Comment 4

[(no longer active)]

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]

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

[(no longer active)]

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

[(no longer active)]

Hmmm, I guess this regressed from bug 287179...  Could someone look into this further?

Comment 8

[(no longer active)]

Moving to Core :: Widgets: Win32

Comment 9

[(no longer active)]

Re-requesting blocking as per comment 4

Comment 10

[Dainis Jonitis]

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

[(no longer active)]

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]

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

[(no longer active)]

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]

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

[(no longer active)]

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

Attachment: Allow Ctrl+Shift+num and Alt+Shift+num to return keyboard layout data

Comment 17

[Dainis Jonitis]

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

[(no longer active)]

Asking localizers to test the patch in attachment 300143 [details] [diff] [review] as per comment 17.

Comment 19

[Ere Maijala (slow)]

(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)]

The first message would of course be for shortcut handling only.

Comment 21

[Axel Hecht [:Pike]]

Can we get some try-server builds with that patch, then?
http://wiki.mozilla.org/Build:TryServer

Comment 22

[Dainis Jonitis]

I do not have CVS account. Someone else should commit a patch to TryServer

Comment 23

[(no longer active)]

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 (out for now; he/him)]

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

[(no longer active)]

(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:[e-mail]-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 (out for now; he/him)]

(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:[e-mail]-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

[(no longer active)]

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 28

[:Gijs (out for now; he/him)]

Builds now available at: https://build.mozilla.org/tryserver-builds/2008-01-30_10:[e-mail]-test-bug-414130/

Comment 29

[Mohammad Alinia]

This build is working perfectly for me, I have no problem with inserting ZWNJ anymore.

Comment 30

[Mike Schroepfer]

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)]

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

[(no longer active)]

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

(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)]

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]

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]

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

[(no longer active)]

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)]

It sounds ugly. I would rather add fields to the event object if needed.

Comment 39

[Ere Maijala (slow)]

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]

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

[(no longer active)]

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)]

I'm fine with extended nsKeyEvent too. I don't see how it's so much easier, though..

Comment 43

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

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

(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)]

Who owns this?

Comment 46

[Damon Sicore (:damons)]

Dainis, are you working on another patch that extends nsKeyEvent?  Just want to make sure this blocker is covered.

Comment 47

[Dainis Jonitis]

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] (he/him)(JST, +0900)]

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)]

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] (he/him)(JST, +0900)]

The patch of bug 359638 was landed.

-> FIXED

Comment 51

[(no longer active)]

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

[(no longer active)]

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.