<AWT Dev> [9] Review Request: JDK-8032872: [macosx] Cannot select from JComboBox in a JWindow

[dmitry markov]

Hello,

Could you review the updated fix, please?
webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.01/
- Fixed problem in changeFocusedWindow()
- Added description for getOwnerFrameDialog()

Thanks,
Dmitry

On 03/03/2014 20:19, Sergey Bylokhov wrote:
> On 3/3/14 6:15 PM, Anton V. Tarasov wrote:
>> Hi all,
>>
>> The fix looks fine for me. The usage of getOwnerFrameDialog() in the 
>> mentioned cases indeed seems incorrect. I've looked at the rest of 
>> the code and found yet another incorrect usage, in 
>> LWWindowPeer.changeFocusedWindow line 1265. Please, fix it the same 
>> way. All the other use cases of the method should be fine as they 
>> relate to an activation (a simple window can't be an active window).
> Then it would be good to add appropriate javadoc to 
> getOwnerFrameDialog to mention that it returns owner which can be 
> activated(Frame or D
> ialog and not a WIndow).
>>
>> Also, I'd recommend to add a new testcase to 
>> java/awt/Window/Grab/GrabTest.java
>>
>> Thanks,
>> Anton.
>>
>> On 03.03.2014 16:49, Sergey Bylokhov wrote:
>>> On 3/3/14 2:22 PM, dmitry markov wrote:
>>>> If I get it right, getOwnerFrameDialog() is designed for another 
>>>> purpose. Also it is NOT only used in notifyMouseEvent() and 
>>>> notifyNCMouseDown().
>>> Probably other places don't work also? I see that these usages are 
>>> related to the focus staff. Anton do you know why 
>>> LWWindowPeer.getOwnerFrameDialog() checks Frame and Dialog only?
>>>> So I think we should stay this method as is to avoid any problems 
>>>> in future.
>>>>
>>>> It is really necessary to add new method isOneOfOwnersOf(). BTW 
>>>> this approach is already used on Windows platform, see 
>>>> awt_Window.cpp for details.
>>>>
>>>> Thanks,
>>>> Dmitry
>>>>
>>>> On 03/03/2014 13:54, Petr Pchelko wrote:
>>>>> Hello, Dmitry.
>>>>>
>>>>> I've investigated a similar issue a while ago 
>>>>> (https://bugs.openjdk.java.net/browse/JDK-8029686), could you 
>>>>> please check if this issue is also resolved by your fix?
>>>>>
>>>>>> In other words the current implementation assumes that the 
>>>>>> grabbingWindow must be an instance of Frame or Dialog and does 
>>>>>> not handle the case when the grabbingWindow is JWindow.
>>>>> When I was investigating this I did not understand why that was 
>>>>> done that way. Do you know the reason?
>>>>> May be it's better not in introduce a new function but replace the 
>>>>> getOwnerFrameDialog with your new implementation?
>>>>>
>>>>> Thank you.
>>>>> With best regards. Petr.
>>>>>
>>>>> On 03.03.2014, at 13:45, dmitry markov <dmitry.markov at oracle.com> 
>>>>> wrote:
>>>>>
>>>>>> Hi Sergey,
>>>>>>
>>>>>> The current implementation of LWWindowPeer.getOwnerFrameDialog() 
>>>>>> may return an instance of Frame or Dialog. The returned value 
>>>>>> used to check whether the grabbingWindow is owner of mouse event 
>>>>>> target or not.
>>>>>>
>>>>>> If JComboBox is added to JFrame, the grabbingWindow is JFrame and 
>>>>>> getOwnerFrameDialog() returns the same JFrame object, (i.e. the 
>>>>>> check passes).
>>>>>> If JCombobox is added to JWindow which is owned by JFrame, the 
>>>>>> grabbingWindow is JWindow but getOwnerFrameDialog() returns the 
>>>>>> JFrame, (i.e. the check fails).
>>>>>>
>>>>>> In other words the current implementation assumes that the 
>>>>>> grabbingWindow must be an instance of Frame or Dialog and does 
>>>>>> not handle the case when the grabbingWindow is JWindow.
>>>>>>
>>>>>> Thanks,
>>>>>> Dmitry
>>>>>>
>>>>>> On 03/03/2014 13:01, Sergey Bylokhov wrote:
>>>>>>> Hi, Dmitry.
>>>>>>> Why the problem is reproduced in JWindow? Why it works in JFrame?
>>>>>>>
>>>>>>> On 3/3/14 10:40 AM, dmitry markov wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Could you review the fix for jdk9, please?
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8032872
>>>>>>>> webrev: 
>>>>>>>> http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.00/
>>>>>>>>
>>>>>>>> Problem description: It is impossible to make a selection in 
>>>>>>>> JComboBox added to JWindow via the mouse. The problem is caused 
>>>>>>>> by incorrect mouse events handling in LWWindowPeer class. When 
>>>>>>>> LWWindowPeer receives a mouse event intended for a popup 
>>>>>>>> window, it checks whether the current grabbingWindow is owner 
>>>>>>>> of the popup using getOwnerFrameDialog() method. This approach 
>>>>>>>> always fails for the JComboBox added to JWindow. As a result an 
>>>>>>>> UngrabEvent is send to the popup window.
>>>>>>>>
>>>>>>>> Fix: Introduce new private method 
>>>>>>>> LWWindowPeer.isOneOfOwnersOf(LWWindowPeer peer). The method 
>>>>>>>> will be invoked on grabbingWindow object to test whether it is 
>>>>>>>> owner of current mouse event target or not. The usage of 
>>>>>>>> getOwnerFrameDialog() should be replaced by isOneOfOwnersOf() 
>>>>>>>> in LWWindowPeer.notifyMouseEvent() and 
>>>>>>>> LWWindowPeer.NotifyNCMouseDown().
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Dmitry
>>>>>>>
>>>>
>>>
>>>
>>
>
>