315837 - [Mac] Keys which should produce multiple characters only produce one

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Uri Bernstein]

In some keyboard layouts, certain keys (or key-combinations) are supposed to produce a sequence of several characters. However, in Mozilla, only the first character of the sequence is actually produced.

Examples:

1. Select the "Hebrew" keyboard layout, and press shift-a. This should produce a shin with a right-dot (שׁ, encoded as U+05E9,U+05C1). In Mozilla, only the "shin" itself (without the dot) is produced.

2. Select the "Cherokee-Nation" keyboard layout, activate caps lock, and press "1". This should produce a sequence of three characters (ᏣᎳᎩ - U+13E3,U+13B3,U+13A9). Only the first of these is produced.

Comment 1

[Uri Bernstein]

Attachment: patch

Reset the event flags for each character in the sequence.

The problem was that processing the event for the first character resulted in the NS_EVENT_FLAG_NO_DEFAULT flag being turned on, which meant that subsequent characters weren't handled in nsTextEditorKeyListener::KeyPress().

Comment 2

[Asaf Romano (gone)]

Uri, does this issue exist in cocoa widget (i.e. Camino)?

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> Uri, does this issue exist in cocoa widget (i.e. Camino)?

I couldn't repro the bug in a recent Camino branch nightly (but could in Fx branch nightly).

Comment 4

[Uri Bernstein]

(In reply to comment #2)
> Uri, does this issue exist in cocoa widget (i.e. Camino)?

As Smokey wrote above, Camino does not have the bug. I realise that there is a plan to switch Firefix to cocoa for 1.9, but given that this is such a simple fix, I wonder if it's not worth accepting it for the benefit of Firefox 2.0 (which presumably will be off the 1.8 branch, and will not use cocoa widgets).   

Comment 5

[Asaf Romano (gone)]

Comment on attachment 202548 [details] [diff] [review]
patch

We should initialize the keypress events inside the loop block instead.

Comment 6

[Uri Bernstein]

Notice that InitializeKeyEvent() does not currently reset the flags.

So I have the following options:
1. Create a new brand new keyPressEvent object for each iteration of the loop (and call InitializeKeyEvent() on it).
2. Make InitializeKeyEvent() reset the flags, and just call it inside the loop (reusing the same object).

In any case, I will also have to keep the call to InitializeKeyEvent() before the loop, in order to get isCharacter.

Mano - which of the above do you prefer? Or do you have a better idea?

Comment 7

[Asaf Romano (gone)]

Why do I have the feeling this code works by accident?

Assuming Control+Key never produces multiple characters, I would go with the former.

Comment 8

[Uri Bernstein]

Attachment: patch v.2

Create and initialize a new nsKeyEvent for each iteration of the loop.
In order to avoid having to also call InitializeKeyEvent() before the loop, just for getting isCharacter, I'm now calculating the condition directly in HandleUKeyEvent. Since this was the only place where the aIsChar argument was used, I removed it altogether.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 203798 [details] [diff] [review]
patch v.2

r=mano.

Comment 10

[Asaf Romano (gone)]

Checking in nsMacEventHandler.cpp;
/cvsroot/mozilla/widget/src/mac/nsMacEventHandler.cpp,v  <--  nsMacEventHandler.cpp
new revision: 1.177; previous revision: 1.176
done
Checking in nsMacEventHandler.h;
/cvsroot/mozilla/widget/src/mac/nsMacEventHandler.h,v  <--  nsMacEventHandler.h
new revision: 1.43; previous revision: 1.42
done

Comment 11

[Marcia Knous [:marcia]]

[12-19 triage team commment] We will review this offline to further assess the risk involved and come back to it.

Comment 12

[Mike Schroepfer]

Mano, Uri, Josh, or Simon - can you comment on the risk to breaking Non-Hebrew keyboards - i.e. making the problem worse for the 1.8.0.1 branch?

Comment 13

[Simon Fraser [no longer active]]

(In reply to comment #12)
> Mano, Uri, Josh, or Simon - can you comment on the risk to breaking Non-Hebrew
> keyboards - i.e. making the problem worse for the 1.8.0.1 branch?

This seems low-risk, and has been baking on trunk.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 203798 [details] [diff] [review]
patch v.2

a=dveditz for drivers

Comment 15

[Uri Bernstein]

Checked in to 1.8 and 1.8.0 branches:

Checking in widget/src/mac/nsMacEventHandler.h;
/cvsroot/mozilla/widget/src/mac/nsMacEventHandler.h,v  <--  nsMacEventHandler.h
new revision: 1.41.4.2; previous revision: 1.41.4.1
done
Checking in widget/src/mac/nsMacEventHandler.cpp;
/cvsroot/mozilla/widget/src/mac/nsMacEventHandler.cpp,v  <--  nsMacEventHandler.cpp
new revision: 1.172.4.6; previous revision: 1.172.4.5
done

Checking in widget/src/mac/nsMacEventHandler.h;
/cvsroot/mozilla/widget/src/mac/nsMacEventHandler.h,v  <--  nsMacEventHandler.h
new revision: 1.41.4.1.2.1; previous revision: 1.41.4.1
done
Checking in widget/src/mac/nsMacEventHandler.cpp;
/cvsroot/mozilla/widget/src/mac/nsMacEventHandler.cpp,v  <--  nsMacEventHandler.cpp
new revision: 1.172.4.4.2.2; previous revision: 1.172.4.4.2.1
done