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

[Anton V. Tarasov]

Hi Dmitry,

Actually, I meant to add a _simple_ case to the test, or otherwise it becomes overly complicated. 
Sorry, if I didn't make that clear.

So, the simple case is to have a hierarchy: frame -> window-1 -> window-2, where window-1 is 
grabbed. Then you click in window-2 and check if it doesn't cause ungrab event. Right?

Thanks,
Anton.

On 05.03.2014 12:55, dmitry markov wrote:
> Hi Anton,
>
> I updated java/awt/Window/Grab/GrabTest.java as you requested. Please find new webrev here: 
> http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.02/
>
> Thanks,
> Dmitry
>
> On 04/03/2014 19:49, Anton V. Tarasov wrote:
>> Hi Dmitry,
>>
>> The fix looks fine for me, but I'm still voting for adding this case to 
>> java/awt/Window/Grab/GrabTest.java
>>
>> Thanks,
>> Anton.
>>
>> On 04.03.2014 17:26, dmitry markov wrote:
>>> 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
>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>