<Swing Dev> Swing dev>[12] Review request for JDK-8189253: [macos] JPopupMenu is inadvertently shown when using setComponentPopupMenu

Tue Nov 6 12:56:33 UTC 2018

Hi Manajit,

Please find my answer inline.

Thanks,
Dmitry
> On 5 Nov 2018, at 17:29, Manajit Halder <manajit.halder at oracle.com> wrote:
> 
> Hi Dmitry,
> 
> Thanks for you reply. Please find my reply inline.
> 
> Regards,
> Manajit
> 
> On 01/11/18 4:59 PM, Dmitry Markov wrote:
>> Hi Manajit,
>> 
>> The usage of addChildWindow will introduce some undesirable behaviour. For example, parent and child windows are created on the same screen; on the build with your fix if the parent window is moved the child will be moved too.
> As per my understanding that is the expected behaviour. Moving the parent window will move the child window as the child window is attached to the parent window. But moving the child window won't move the parent window.
That behaviour is specified for addChildWindow in Cocoa spec but it is incorrect for AWT/Swing. If the current fix is integrated we will get different behaviour on OS X and other platforms. I think we shouldn’t use addChildWindow due to high risk of introducing some unwanted behaviour like this.

>> It is better to not use addChildWindow without a good reasons.
>> 
> addChildWindow need to used in this case because if we use orderWindow an extra event is generated and the reported issue is reproduced, i.e. the JPopupMenu is shown on the modeless Dialog (explained below in my first proposed fix).  
That’s right. The problem is caused by incorrect handling of mouse events for some cases. In my opinion it is necessary to revise event handling logic for OS X and sort out what goes wrong.

> 
>> Will your fix work if the parent with popup menu and the child are created on different screens?
> I have tried my fix with a Frame with PopupMenu and a Dialog on different screens, and it works.
> 
>> Thanks,
>> Dmitry
>> 
>>> On 31 Oct 2018, at 16:41, Manajit Halder <manajit.halder at oracle.com <mailto:manajit.halder at oracle.com>> wrote:
>>> 
>>> Changed the targeted review version to 12.
>>> 
>>> Regards,
>>> Manajit
>>> On 31/10/18 10:09 PM, Manajit Halder wrote:
>>>> Hi Dmitry/Sergey,
>>>> 
>>>> I have a different fix for this issue in Mac OS specific code. In this proposed fix I am trying to fix the window addition/ordering with respect to the screen on which the window is created. As per my debugging this issue was introduced while fixing JDK-8080729. The problem was introduced due to the use of orderWindow by replacing it with addChildWindow. The difference between these two Mac OS methods:
>>>> 
>>>> addChildWindow: After the childWin is added as a child of the window, it is maintained in relative position indicated by place for subsequent ordering operations involving either window. While this attachment is active, moving childWin will not cause the window to move (as in sliding a drawer in or out), but moving the window will cause childWin to move.
>>>> https://developer.apple.com/documentation/appkit/nswindow/1419152-addchildwindow?language=objc <https://developer.apple.com/documentation/appkit/nswindow/1419152-addchildwindow?language=objc>
>>>> orderWindow: Repositions the window’s window device in the window server’s screen list.
>>>> https://developer.apple.com/documentation/appkit/nswindow/1419672-orderwindow?language=occ <https://developer.apple.com/documentation/appkit/nswindow/1419672-orderwindow?language=occ>
>>>> The main difference which was causing this issue is: addChildWindow attaches the child window to its parent, but orderWindow re positions it. 
>>>> 
>>>> Proposed fix:
>>>>     In my proposed fix I am trying to add/order the window based on which screen it is created. If the windows (parent and child) are created on same screen then addChildWindow is used to order child window on top of parent window. And orderWindow is used for the other case when windows are created on different screens.
>>>> 
>>>> Please review the fix:
>>>> http://cr.openjdk.java.net/~mhalder/8189253/webrev.01/ <http://cr.openjdk.java.net/%7Emhalder/8189253/webrev.01/>
>>>> Regards,
>>>> Manajit
>>>> On 03/05/18 12:06 AM, Dmitry Markov wrote:
>>>>> Hi Manajit,
>>>>> 
>>>>> If I got it right, the problem is OSX specific and you suggest fixing it in common code. I am sorry but that does not look like as a good approach unless it has reasonable explanation.
>>>>> I recommend that you should move the fix to more suitable place, (i.e. OSX specific code). For example, LWWindowPeer.notifyMouseEvent() where we receive mouse events from the platform and generate ours mouse enter/exit events if necessary. I guess it is possible to implement something similar to your current fix in LWWindowPeer.
>>>>> 
>>>>> Thanks,
>>>>> Dmitry 
>>>>> 
>>>>>> On 28 Apr 2018, at 00:54, Sergey Bylokhov <sergey.bylokhov at oracle.com> <mailto:sergey.bylokhov at oracle.com> wrote:
>>>>>> 
>>>>>> Hi, Manajit.
>>>>>> Please clarify why this bug is occurred after JDK-8080729, how=20
>>>>>> orderWindow/addChildWindow are related to the mouse events which we get?=20
>>>>>> Did we start to get more event or less events or we get events in the=20
>>>>>> different order, what is the difference?
>>>>>> 
>>>>>> On 04/01/2018 10:22, Manajit Halder wrote:
>>>>>>> Hi Semyon,
>>>>>>> =20
>>>>>>> Fix for issue JDK-8080729=20
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8080729> <https://bugs.openjdk.java.net/browse/JDK-8080729> has caused this=20
>>>>>>> regression due to changes in method setVisible(boolean visible) in file=
>>>>>> =20
>>>>>>> CPlatformWindow.java
>>>>>>> orderWindow is causing the issue here, if we revert to addChildWindow=20
>>>>>>> then the issue is not observed but then the fix for issue JDK-8080729 f=
>>>>>> ails.
>>>>>>> Before this change the child window used to be added on to the parent a=
>>>>>> s=20
>>>>>>> shown below in the commented code. But after the change child window is=
>>>>>> =20
>>>>>>> ordered above the parent.
>>>>>>> =20
>>>>>>> Below code causes the regression:
>>>>>>> =20
>>>>>>> *CWrapper.NSWindow.orderWindow(ptr, CWrapper.NSWindow.NSWindowAbove,=20
>>>>>>> ownerPtr);*
>>>>>>> *//CWrapper.NSWindow.addChildWindow(ownerPtr, ptr,=20
>>>>>>> CWrapper.NSWindow.NSWindowAbove);*
>>>>>>> =20
>>>>>>> Debugging further I found that if we use orderWindow then the new windo=
>>>>>> w=20
>>>>>>> is considered as new graphics device in the method notifyReshape in=20
>>>>>>> LWWindowPeer.java (the method updateGraphicsDevice() returns true) and=20
>>>>>>> is the difference between using orderWindow and addChildWindow.
>>>>>>> =20
>>>>>>> Since the option to add child window is between choosing oderWindow and=
>>>>>> =20
>>>>>>> addChildWindow we don=E2=80=99t have any option to do the fix in the Ma=
>>>>>> c OS=20
>>>>>>> native code.
>>>>>>> =20
>>>>>>> Regards,
>>>>>>> Manajit
>>>>>>> =20
>>>>>>> =20
>>>>>>>> On 02-Jan-2018, at 11:30 PM, Semyon Sadetsky=20
>>>>>>>> <semyon.sadetsky at oracle.com <mailto:semyon.sadetsky at oracle.com> <mailto:semyon.sadetsky at oracle.com> <mailto:semyon.sadetsky at oracle.com>> wrote=
>>>>>> :
>>>>>>>> Hi Manajit,
>>>>>>>> 
>>>>>>>> JDK-8080729 bug was Mac OS specific issue and its fix changed the Mac=20
>>>>>>>> OS code only. Nevertheless you are suggesting to fix the regression in=
>>>>>> =20
>>>>>>>> generic code. This need to be explained somehow.
>>>>>>>> 
>>>>>>>> --Semyon
>>>>>>>> 
>>>>>>>> On 12/25/2017 02:42 AM, Manajit Halder wrote:
>>>>>>>>> Hi Semyon,
>>>>>>>>> 
>>>>>>>>> Regression is cause by JDK-8080729=20
>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8080729> <https://bugs.openjdk.java.net/browse/JDK-8080729>. The fix can=E2=80=
>>>>>> =99t be=20
>>>>>>>>> reversed since it is the=C2=A0choice between addChildWindow or=20
>>>>>>>>> orderWindow. Went through code flow related to the issue but=C2=A0did=
>>>>>> n=E2=80=99t=20
>>>>>>>>> find any other better place in code to handle this issue. The best=20
>>>>>>>>> way to fix the issue would be to avoid retargeting of events=20
>>>>>>>>> (MOUSE_ENTER and MOUSE_EXIT) between MOUSE_PRESS and MOUSE_RELEASE on=
>>>>>> =20
>>>>>>>>> the parent window (when the mouse is actually on the child window).=20
>>>>>>>>> Therefore request you to review the webrev.00.
>>>>>>>>> 
>>>>>>>>> Regards,
>>>>>>>>> Manajit
>>>>>>>>> 
>>>>>>>>>> On 08-Dec-2017, at 9:55 PM, semyon.sadetsky at oracle.com=20 <mailto:semyon.sadetsky at oracle.com=20>
>>>>>>>>>> <mailto:semyon.sadetsky at oracle.com> <mailto:semyon.sadetsky at oracle.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> Hi Manajit,
>>>>>>>>>> 
>>>>>>>>>> Can you provide information which fix caused the regression?
>>>>>>>>>> 
>>>>>>>>>> --Semyon
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On 12/8/17 5:53 AM, Manajit Halder wrote:
>>>>>>>>>>> Hi All,
>>>>>>>>>>> 
>>>>>>>>>>> Kindly review the following Swing fix.
>>>>>>>>>>> 
>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189253 <https://bugs.openjdk.java.net/browse/JDK-8189253>
>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~mhalder/8189253/webrev.00/=20 <http://cr.openjdk.java.net/%7Emhalder/8189253/webrev.00/=20>
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8189253/webrev.00/> <http://cr.openjdk.java.net/%7Emhalder/8189253/webrev.00/>
>>>>>>>>>>> 
>>>>>>>>>>> Cause:
>>>>>>>>>>> Issue was due to retargeting of mouse enter exit events.
>>>>>>>>>>> MOUSE_ENTER and MOUSE_EXIT events were sent on the parent window=20
>>>>>>>>>>> (JFrame) in between MOUSE_PRESS and MOUSE_RELEASE events on the=20
>>>>>>>>>>> modeless JDialog.
>>>>>>>>>>> 
>>>>>>>>>>> Fix:
>>>>>>>>>>> Retargeting of events is not done in-between MOUSE_PRESS and=20
>>>>>>>>>>> MOUSE_RELEASE.
>>>>>>>>>>> 
>>>>>>>>>>> Regards,
>>>>>>>>>>> Manajit
>>>>>>>>>>> 
>>>>>>> =20
>>>>>> --=20
>>>>>> Best regards, Sergey.
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20181106/7cade931/attachment-0001.html>