Hello, Dmitry. Thank you for the review. I will delete this piece of code before the push.
With best regards. Petr. On Apr 3, 2013, at 1:13 PM, Dmitry Cherepanov wrote: > 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. >>>>>>> >>> >
