Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8254841: [macos] Enter and Exit events shouldn't be sent whilst resizing #5497

Closed
wants to merge 12 commits into from

Conversation

alisenchung
Copy link
Contributor

@alisenchung alisenchung commented Sep 13, 2021

Added a resizing flag when the window is currently being resized to block mouseEntered and mouseExited events from being posted to that window.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8254841: [macos] Enter and Exit events shouldn't be sent whilst resizing

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5497/head:pull/5497
$ git checkout pull/5497

Update a local copy of the PR:
$ git checkout pull/5497
$ git pull https://git.openjdk.java.net/jdk pull/5497/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5497

View PR using the GUI difftool:
$ git pr show -t 5497

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5497.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 13, 2021

👋 Welcome back alisenchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 13, 2021

@alisenchung The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client label Sep 13, 2021
@pankaj-bansal
Copy link

@pankaj-bansal pankaj-bansal commented Sep 14, 2021

I have some suggestion for test

  1. Access the swing components only on EDT thread.
  2. The test java/awt/event/MouseEvent/SpuriousExitEnter/SpuriousExitEnter.java is added against the current bug in ProblemList. Please have a look at this test also. It is a manual test and it should also pass after your fix. If it does pass, remove it from the ProblemList and if does not, please look into it.
  3. Looks like the bug is a confidential bug, it should be made public for github review.

@openjdk openjdk bot added the rfr label Sep 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 14, 2021

@alisenchung alisenchung force-pushed the alisenchung:8254841 branch from dd8d591 to e299142 22 hours ago

Absent a compelling reason, please do not force push to a branch once you have create a PR from that branch.

@alisenchung alisenchung changed the title 8254841: Enter or Exit events are shouldn't be displayed while resizing 8254841: [macos] Enter or Exit events are shouldn't be displayed while resizing Sep 14, 2021
@mrserb
Copy link
Member

@mrserb mrserb commented Sep 15, 2021

I have a general question about this bug, initially the test was added for https://bugs.openjdk.java.net/browse/JDK-6479820 and that bug was fixed because of this:

When resizing the window by dragging a corner with the mouse,
the panel receives entry/exit mouse events randomly.
These events should not occur because the mouse is not entering
or exiting the window, it is dragging a corner of its border.

And in the current bug description:

The window resizing is a bit slower than mouse movement. So before the window resize can catch up to the mouse, the mouse exits the window, then re-enters when the window catches up to it. This can be verified if you resize the window at the edge without entering the window: there will be no mouseEntered or mouseExit events.

So situation is different, if the mouse is moved faster than window is moving, then I do not see why we should not post an events. Especially if the native system did that.

But the question is it really a problem of slow window, or may be we have some bug in the nativeSynthesizeMouseEnteredExitedEvents where we synthesize such events when we should not.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 15, 2021

But the question is it really a problem of slow window, or may be we have some bug in the nativeSynthesizeMouseEnteredExitedEvents where we synthesize such events when we should not.

I think we don't synthesize these events, they come from native.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 15, 2021

I have a general question about this bug, initially the test was added for https://bugs.openjdk.java.net/browse/JDK-6479820 and that bug was fixed because of this:

When resizing the window by dragging a corner with the mouse,
the panel receives entry/exit mouse events randomly.
These events should not occur because the mouse is not entering
or exiting the window, it is dragging a corner of its border.

And in the current bug description:

The window resizing is a bit slower than mouse movement. So before the window resize can catch up to the mouse, the mouse exits the window, then re-enters when the window catches up to it. This can be verified if you resize the window at the edge without entering the window: there will be no mouseEntered or mouseExit events.

So situation is different, if the mouse is moved faster than window is moving, then I do not see why we should not post an events. Especially if the native system did that.

In the original bug report, I believe the root cause of the bug is the same based on the comment in JBS:
So far this happens when system is not fast enough to resize native component in a short period of time while message is on its way.

And I tried to keep the same idea for the fix in windows:
The idea of the fix would be to pick up AwtWindow.m_resizing value and not consider any messages in PreProcessMouseMsg until m_resizing is true.

So I guess my question is whether or not we should be posting these events in the first place during resizing? I still see the original bug fix in the windows files so I believe there currently is a mismatch in behavior across platforms.

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 16, 2021

And I tried to keep the same idea for the fix in windows:
The idea of the fix would be to pick up AwtWindow.m_resizing value and not consider any messages in PreProcessMouseMsg until m_resizing is true.

If the idea just align behavior to the Windows then you can just update the AWTWindow.m#_deliverMoveResizeEvent, same as on windows set your flag to true when the [self.nsWindow inLiveResize] is true, and then at the end of the method _deliverMoveResizeEvent sets the flag to false. Or just check that [self.nsWindow inLiveResize] flag before sending the mouse event.

But note that [AWTWindow synthesizeMouseEnteredExitedEventsForAllWindows] is called in the _deliverMoveResizeEvent, so it is quite possible that these "additional" events are posted by our code and not by the macos.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 16, 2021

And I tried to keep the same idea for the fix in windows:
The idea of the fix would be to pick up AwtWindow.m_resizing value and not consider any messages in PreProcessMouseMsg until m_resizing is true.

If the idea just align behavior to the Windows then you can just update the AWTWindow.m#_deliverMoveResizeEvent, same as on windows set your flag to true when the [self.nsWindow inLiveResize] is true, and then at the end of the method _deliverMoveResizeEvent sets the flag to false. Or just check that [self.nsWindow inLiveResize] flag before sending the mouse event.

But note that [AWTWindow synthesizeMouseEnteredExitedEventsForAllWindows] is called in the _deliverMoveResizeEvent, so it is quite possible that these "additional" events are posted by our code and not by the macos.

Oh you're right, removing the line also removes the extra mouseEntered and mouseExit events. I thought it wasn't being called because a print statement I put in that function never showed up, but I must've messed something up. Was there a reason for the synthesizeMouseEnteredExitedEventsForAllWindows call?

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 17, 2021

Oh you're right, removing the line also removes the extra mouseEntered and mouseExit events. I thought it wasn't being called because a print statement I put in that function never showed up, but I must've messed something up. Was there a reason for the synthesizeMouseEnteredExitedEventsForAllWindows call?

That was done as a fix for JDK-8028485. We have to generate the mouse enter/exit events if the window was moved/resized under the mouse(while the mouse pointer was not moved)

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 23, 2021

Before the changes the mouseEntered and mouseExited events would constantly fire, while after the fix only a mouseEntered event would fire

Does it mean that after the fix we will post a few enter events w/o exit events? Then it is worse than posting enter/exit events that are paired, even if we will post many such pairs.

When the user maximizes the window a mouseEntered event does fire, so it preserves the fix for JDK-8028485.

Just for the record did you check what is the inLiveResize value when the user maximizes the window, or moves it to the full screen/tile the window to the left/right?

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 23, 2021

Before the changes the mouseEntered and mouseExited events would constantly fire, while after the fix only a mouseEntered event would fire

Does it mean that after the fix we will post a few enter events w/o exit events? Then it is worse than posting enter/exit events that are paired, even if we will post many such pairs.

It always posts a single enter event without an exit event.

When the user maximizes the window a mouseEntered event does fire, so it preserves the fix for JDK-8028485.

Just for the record did you check what is the inLiveResize value when the user maximizes the window, or moves it to the full screen/tile the window to the left/right?

I checked for when the user maximizes the window and tiles to the left or right of the screen and both seem to work.

I just ran all the tests and my test fails on both windows and linux platforms so I think this bug isn't platform specific.

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 23, 2021

I checked for when the user maximizes the window and tiles to the left or right of the screen and both seem to work.

But what is the flag value? Is it true or false? If it's true and everything works maybe we do not need this line at all?

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 23, 2021

I checked for when the user maximizes the window and tiles to the left or right of the screen and both seem to work.

But what is the flag value? Is it true or false? If it's true and everything works maybe we do not need this line at all?

Oh the flag is always true. And removing the line completely fixes the bug and doesn't have any problems with fullscreen or tiling.

@prrace
Copy link
Contributor

@prrace prrace commented Sep 23, 2021

I checked for when the user maximizes the window and tiles to the left or right of the screen and both seem to work.

But what is the flag value? Is it true or false? If it's true and everything works maybe we do not need this line at all?

Oh the flag is always true. And removing the line completely fixes the bug and doesn't have any problems with fullscreen or tiling.

Surely that was there for a reason ? Looks like it might be important for the synthesized full screen mode ?
It was added in that location by this fix : https://mail.openjdk.java.net/pipermail/awt-dev/2013-November/006372.html
and that just moved it a few lines so it was already in use before then.

Does the test from there still pass ? Any others fail ?

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 24, 2021

I checked for when the user maximizes the window and tiles to the left or right of the screen and both seem to work.

But what is the flag value? Is it true or false? If it's true and everything works maybe we do not need this line at all?

Oh the flag is always true. And removing the line completely fixes the bug and doesn't have any problems with fullscreen or tiling.

Surely that was there for a reason ? Looks like it might be important for the synthesized full screen mode ?
It was added in that location by this fix : https://mail.openjdk.java.net/pipermail/awt-dev/2013-November/006372.html
and that just moved it a few lines so it was already in use before then.

Sergey did mention it was added as a fix for JDK-8028485 earlier too. Reverting the changes made by JDK-8028485 doesn't seem to reintroduce the fullscreen not firing mouseEntered bug, so I'm not exactly sure how that fix worked. Behavior seems to be the same regardless of the position of the [AWTWindow synthesizeMouseEnteredExitedEventsForAllWindows] line.

Does the test from there still pass ? Any others fail ?

All manual and automatic tests are passing on macOS.

@prrace
Copy link
Contributor

@prrace prrace commented Sep 24, 2021

OK. Unless someone else can point out what we are missing it seems like it should resolve this issue.

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 24, 2021

Does the test from there still pass ? Any others fail ?

All manual and automatic tests are passing on macOS.

I guess the only test which makes sense here is the test for JDK-8013468 and it is "problem listed" due to a reflection issue. I suggest checking how it works.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Sep 29, 2021

I guess the only test which makes sense here is the test for JDK-8013468 and it is "problem listed" due to a reflection issue. I suggest checking how it works.

This test fails both before and after the change. However, before the change if I'm constantly moving the mouse the test will pass whereas after the change it won't pass even if I'm moving my mouse. I re-added the synthesizeMouseEnteredExitedEventsForAllWindows with a check for in fullscreen mode which should only be true if its in true fullscreen mode (without title bar). This keeps the behavior from before the change, but the test won't pass if the mouse isn't moving during the fullscreen.

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 29, 2021

It sound strange, isn't the "_deliverMoveResizeEvent" should be called on the resize only, why it affect the mouse move events which posted later? the new flag NSWindowStyleMaskFullScreen will work when the window was moved to the full screen, what about the case when the window will moved from the full screen -> the mouse will appier outside of the window?

BTW we also have a call to synthesizeMouseEnteredExitedEventsForAllWindows in the windowDidEnterFullScreen which should do what you tried to achieve in the latest patch, why it does not work?

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 1, 2021

It sound strange, isn't the "_deliverMoveResizeEvent" should be called on the resize only, why it affect the mouse move events which posted later? the new flag NSWindowStyleMaskFullScreen will work when the window was moved to the full screen, what about the case when the window will moved from the full screen -> the mouse will appier outside of the window?

It looks like the line was added for the previous bug only for when fullscreen mode is turned off and the mouse is outside the window. Entering fullscreen already posts a mouseEntered event.

BTW we also have a call to synthesizeMouseEnteredExitedEventsForAllWindows in the windowDidEnterFullScreen which should do what you tried to achieve in the latest patch, why it does not work?

I'm don't see windowDidEnterFullScreen calls when the window is being fullscreened.

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 1, 2021

I'm don't see windowDidEnterFullScreen calls when the window is being fullscreened.

As far as I understand the windowDidEnterFullScreen should be called when the window moved to the fullscreen mode
https://developer.apple.com/documentation/appkit/nswindowdelegate/1419116-windowdidenterfullscreen?language=objc

There are two fullscreen modes on macOS, one of them can be set via GraphicsDevice.setFullScreenWindow() and another one can be set by pressing the maximized button(https://support.apple.com/guide/mac-help/use-apps-in-full-screen-mchl9c21d2be/mac), which one did you test?

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 1, 2021

I'm don't see windowDidEnterFullScreen calls when the window is being fullscreened.

As far as I understand the windowDidEnterFullScreen should be called when the window moved to the fullscreen mode https://developer.apple.com/documentation/appkit/nswindowdelegate/1419116-windowdidenterfullscreen?language=objc

There are two fullscreen modes on macOS, one of them can be set via GraphicsDevice.setFullScreenWindow() and another one can be set by pressing the maximized button(https://support.apple.com/guide/mac-help/use-apps-in-full-screen-mchl9c21d2be/mac), which one did you test?

I tested both. The maximized button works properly, but fullscreen mode set by GraphicsDevice.setFullScreenWindow() isn't working properly.

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 1, 2021

I tested both. The maximized button works properly, but fullscreen mode set by GraphicsDevice.setFullScreenWindow() isn't working properly.

And the windowDidEnterFullScreen is not called? Or the fullscreen mode work because this method is called?

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 1, 2021

And the windowDidEnterFullScreen is not called? Or the fullscreen mode work because this method is called?

windowDIdEnterFullScreen isn't being called but entering fullscreen mode still posts a mouseEntered event.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 1, 2021

I think synthesizeMouseEnteredExitedEventsForAllWindows shouldn't be in deliverMoveResizeEvent since there shouldn't be any mouseEnter/Exit events during a resize or move and I can submit another bug for didWindowEnterFullscreen and didWindowExitFullscreen not being called/not posting mouseEvents that I can work on separately..

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 1, 2021

windowDIdEnterFullScreen isn't being called but entering fullscreen mode still posts a mouseEntered event.

I have added a trace to the window WillEnterFullScreen and window DIdEnterFull Screen and see that both are called when the window is moved to the full screen mode via green button.

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 1, 2021

I think synthesizeMouseEnteredExitedEventsForAllWindows shouldn't be in deliverMoveResizeEvent since there shouldn't be any mouseEnter/Exit events during a resize or move

Why do you think that if the window is moved/resized and/or appeared under the mouse then the mouse event should not be posted? I do not think so. We can try to minimize the number of events when the user drags/resizes the window, but we should not change the number of events if the macos/app moved/resized the window.

and I can submit another bug for didWindowEnterFullscreen and didWindowExitFullscreen not being called/not posting mouseEvents that I can work on separately..

It should be called, I have checked that on macOS 11.5, but I do not think it depends on the version.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 4, 2021

I added a keyListener to SpuriousExitEnter test to enter and exit fullscreen with GraphicsDevice on a specific keypress and I wasn't able to get NSLog to show up in AWTWindow. What test were you able to see the trace and did it also post a mouseEntered/mouseExit event properly?

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 4, 2021

What test were you able to see the trace and did it also post a mouseEntered/mouseExit event properly?

I just added the fprintf(stderr, "windowDidEnterExitFullScreen\n");
to the AWTWindow.m#windowDidEnterFullScreen/windowDidExitFullScreen
and see that the logging is printed.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 7, 2021

I think synthesizeMouseEnteredExitedEventsForAllWindows shouldn't be in deliverMoveResizeEvent since there shouldn't be any mouseEnter/Exit events during a resize or move

Why do you think that if the window is moved/resized and/or appeared under the mouse then the mouse event should not be posted? I do not think so. We can try to minimize the number of events when the user drags/resizes the window, but we should not change the number of events if the macos/app moved/resized the window.

I think mouseEvents are already posted by macos when the mouse enters the window thru fullscreen-resizing (green button) or tiling, so I think we don't need to synthesize additional events. I suggest we remove the line because it introduces another bug where dragging to move the window will post a mouseEntered event without the mouse ever entering the actual window. It also introduces the bug from the original bug report.

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 7, 2021

I think mouseEvents are already posted by macos when the mouse enters the window thru fullscreen-resizing (green button) or tiling, so I think we don't need to synthesize additional events.

If we do not need to synthesize additional events then why do you see a difference in behavior before/after the fix here:
#5497 (comment)
?

I suggest we remove the line because it introduces another bug where dragging to move the window will post a mouseEntered event without the mouse ever entering the actual window. It also introduces the bug from the original bug report.

But we should not introduce the new one.

@alisenchung
Copy link
Contributor Author

@alisenchung alisenchung commented Oct 8, 2021

I think mouseEvents are already posted by macos when the mouse enters the window thru fullscreen-resizing (green button) or tiling, so I think we don't need to synthesize additional events.

If we do not need to synthesize additional events then why do you see a difference in behavior before/after the fix here: #5497 (comment) ?

The case I tested is supposed to be covered by windowDidEnterFullscreen. Also it passed when I tested it manually, but only the automatic test has different behavior..

I suggest we remove the line because it introduces another bug where dragging to move the window will post a mouseEntered event without the mouse ever entering the actual window. It also introduces the bug from the original bug report.

But we should not introduce the new one.

I meant that by removing the line 2 different bugs are fixed

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 8, 2021

If we do not need to synthesize additional events then why do you see a difference in behavior before/after the fix here: #5497 (comment) ?

The case I tested is supposed to be covered by windowDidEnterFullscreen. Also it passed when I tested it manually, but only the automatic test has different behavior..

The windowDidEnterFullscreen is called before and after the fix, so If automatic test has a different behaviour means that the current change affects it in some unclear way, right?

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 6, 2021

@alisenchung This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 4, 2021

@alisenchung This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client ready rfr
7 participants