Hi Petr,
Looks fine to me too. Just a minor suggestion. If we're going to update
CEmbeddedFrame.handleKeyEvent, could you please also remove another
version of handleKeyEvent (with comment "REMIND: delete ..."). The
plugin doesn't use this version now and it's safe to delete this piece
of code.
Dmitry
On 4/2/2013 8:19 PM, Anthony Petrov wrote:
Thanks for the clarifications, Petr. The fix looks fine to me, then.
--
best regards,
Anthony
On 4/2/2013 20:00, Petr Pchelko wrote:
Thank you for the review, Anthony.
It may happen that a user presses several keys at once, each will
generate a PRESSED event. But the RELEASED event will only be sent
for the lastKeyPressCode. Could you test this case please and see if
we should do anything about it?
There are 2 cases:
1. You click 2 buttons really fast. Then events come as Key1 pressed
-> Text input for key 1 -> key2 pressed -> text input for key 2. 2.
You press and hold key 1 -> press key 2 -> release key 2 -> release
key 1. In this case the browser sends keyPressed events only for the
key pressed last.
Safary handles both cases well. Firefox sends an additional
keyReleased event for one of the keys. I suppose it is a bug in
Firefox, because specification says, that if we return 2 in response
to keyDown event we will not get a keyReleased event.
To conclude: I've tested it quite extensively and multiple keys are
handled well, except an issue in Firefox.
I'm also concerned that synthesizing the RELEASED event in
handleKeyEvent() is guarded with a needsKeyReleased (and the event
might have already been sent for the lastKeyPressCode from there),
however, there's no such check in handleInputEvent() and we send it
unconditionally. So it looks a little unbalanced.
The KEY_RELEASED event is synthesized in handleKeyEvent() only when
both needsKeyTyped and needsKeyReleased flags are true. This is only
true when we hold a key in Firefox. Firefox has a strange
implementation in this case, which depends on a button:
1. For a, i, o, etc. These are the buttons which have some non-latin
symbols like à or ł. We get a cycle of KEY_PRESSED and then nothing.
There is a bug: a special out-of-line box is showing up in the wrong
place and TextInputEvent does not come if you choose a symbol from a
popup. I think it would be better to fix it as a separate issue.
2. For buttons which does not have associated non-latin symbols: We
constantly get KEY_PRESSED and then nothing. 3. For SPACE and ENTER:
these buttons stop the composition, so we get a cycle of
KEY_PRESSED->TEXT_INPUT->KEY_PRESSED->TEXT_INPUT->.... This is worked
around in handleKeyEvent()
So, as you see generation of KEY_RELEASED in handleKeyEvent() and
hadleInputEvent() never goes together. The only exception is an
additional keyReleased event sent by Firefox. However, as I've said,
I think it's a Firefox bug.
With best regards. Petr.
Hi Petr,
The changes in handleKeyEvent() look good to me.
However, I'm not so sure about the handleInputEvent() synthesizing
the RELEASED event. It may happen that a user presses several keys
at once, each will generate a PRESSED event. But the RELEASED event
will only be sent for the lastKeyPressCode. Could you test this case
please and see if we should do anything about it? I'm also concerned
that synthesizing the RELEASED event in handleKeyEvent() is guarded
with a needsKeyReleased (and the event might have already been sent
for the lastKeyPressCode from there), however, there's no such check
in handleInputEvent() and we send it unconditionally. So it looks a
little unbalanced.
--
best regards,
Anthony
On 4/2/2013 18:00, Petr Pchelko wrote:
Hello, thank you for the review.
I have updated the fix, the new version is available at:
http://cr.openjdk.java.net/~pchelko/80010009/
Dmitry wrote:
One comment - perhaps it'd be better to synthesize one UP event.
According to the spec, the Plugin doesn't receive DOWN/UP events
associated with composition - the Plugin receive one DOWN event
and returns kNPEventStartIME to start composition and we should
probably synthesize one UP event for consistency.
Yes, you are certainly right, I have updated the fix for this.
Anthony wrote:
Have you investigated the -isARepeat flag and whether we want to
do the same in JDK?
No, I did not. However, now I have checked this up and updated the
fix. The was an additional issue with a repeat: space and enter
keys stopped the composition instantly, so we've received
duplicated KEY_TYPED events in Firefox. Now this is fixed. The new
version of the fix also affects only applet case as
needsKeyReleased flag is always false for normal windows.
With best regards. Petr.
On Apr 2, 2013, at 1:23 PM, Dmitry Cherepanov wrote:
Hi Petr,
Looks fine to me, thanks for fixing this.
One comment - perhaps it'd be better to synthesize one UP event.
According to the spec, the Plugin doesn't receive DOWN/UP events
associated with composition - the Plugin receive one DOWN event
and returns kNPEventStartIME to start composition and we should
probably synthesize one UP event for consistency.
And it seems like Glass works the same way - synthesize one UP
event associated with composition:
GlassEmbeddedWindow+Npapi.m (_1dispatchCocoaNpapiTextInputEvent
function)
if ([chars length] == 1) {
Java_com_sun_glass_events_mac_NpapiEvent__1dispatchCocoaNpapiKeyEvent(
env, jNpapiClass, jPtr,
com_sun_glass_events_mac_NpapiEvent_NPCocoaEventKeyUp,
0, jText, jText, JNI_FALSE, 0, JNI_FALSE);
}
Dmitry
Petr Pchelko wrote:
Hello, AWT team.
Please review the fix for the following issue:
http://bugs.sun.com/view_bug.do?bug_id=8010009
The fix is available at:
http://cr.openjdk.java.net/~pchelko/8010009/webrev.00/
<http://cr.openjdk.java.net/%7Epchelko/8010009/webrev.00/>
The problem occurred because we use input methods for plugin
keyboard input. So when the text from the input method comes only
KEY_TYPED events were synthesized. Now we also synthesize
KEY_RELEASED events. Additionally, the last keyCode should be
stored because some swing code relies on the keyCode for the
KEY_RELEASED event.
There still are 2 issues around this:
1. Firefox has a very strange behavior when user types extremely
fast. KeyReleased events come from the browser, while they
should not according to the NPAPI specification. I suppose it is
a bug in Firefox. I did not add any workaround for this issue,
because I there is no way to add it safely. The workaround could
possible break the existing code. So I suppose it is better to
file a bug to Mozilla.
2. The game from Pogo does not expect some possible combinations
of events coming and skips squares if you type for example "
'r " . This should probably be filed to Pogo Games.
With best regards. Petr.
