<AWT Dev> [8] Review Request for 8010009 : [macosx] Unable type into online word games on MacOSX

[Dmitry Cherepanov]

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