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

8236971: [macos] Gestures handled incorrectly due to missing events #156

Closed

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 2, 2020

As noted in the JBS issue, this bug is a result of a deliberate change by Apple that affects applications (in this case the JDK) linked against MacOSX 10.11 SDK or later -- they no longer call two methods that we are relying on, beginGestureWithEvent and endGestureWithEvent. There is no deprecation warning or any other indication at compile time or runtime that these methods are ineffective. They just stopped calling them. It is documented in their developer guide:

developer.apple.com/documentation/appkit/nsresponder/1526368-begingesturewithevent

Note that it's the version of the MacOSX SDK that the JDK is linked with that matters. JavaFX gesture notification works when run on a JDK that was linked against the 10.10 SDK or earlier (regardless of what MacOSX SDK was used to link the JavaFX libraries). JavaFX gesture notification fails when run on a JDK that was linked against the 10.11 SDK or later.

The solution, as indicated in the Apple documentation referred to above, is to use the phase information from gesture events to track when to call begin / end gesture.

The fix does the following things:

  1. Removes the beginGestureWithEvent and endGestureWithEvent responder methods in GlassView2D and GlassView3D
  2. Calls new local methods from each gesture to track when to call the associated GlassViewDelegate notification methods, sendJavaGestureBeginEvent and sendJavaGestureEndEvent

I've tested this on macOS 10.13.6 and 10.15.3 (Catalina) and the attached program now runs correctly. I also tested the GestureEvent sample in Ensemble8 and it reproduces the problem before the fix and works correctly after the fix.

I instrumented the code with print statements (which I have since reverted) and verified that the stream of events is as expected, and matches JDK 8u241 with bundled FX.


Progress

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

Issue

  • JDK-8236971: [macos] Gestures handled incorrectly due to missing events

Reviewers

  • Michael Paus (mpaus - Author)
  • Ambarish Rapte (arapte - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/156/head:pull/156
$ git checkout pull/156

@kevinrushforth kevinrushforth self-assigned this Apr 2, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 2, 2020

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

…efore final review)"

This reverts commit 69f0b38.
@kevinrushforth kevinrushforth changed the title WIP: 8236971: [macos] Gestures handled incorrectly due to missing events 8236971: [macos] Gestures handled incorrectly due to missing events Apr 2, 2020
@openjdk openjdk bot added the rfr label Apr 2, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Apr 2, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 2, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Apr 2, 2020

@arapte Can you be one of the reviewers?

@mipastgt I ran it against your test application, but if you have other tests you could run, that would be helpful.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2020

Webrevs

@mipastgt
Copy link
Member

@mipastgt mipastgt commented Apr 3, 2020

I've built a fresh JFX, with your changes applied to master, this morning. I can confirm that the primary bug seems to be fixed with these changes but I have observed some behaviour where I am not sure whether it is correct or whether my expectation is just wrong.

  1. The total delta values are now accumulated correctly for the regualar scroll events but not for the following generated inertia events. They are still equal to the delta values. Is that according to the specification? From a practical point of view a programmer who relies on the total delta values is probably much surprised if the total delta is reset by the inertia events.

  2. Is the touch-count field only valid for touch-events or why is it always zero?

  3. The mouse-wheel behaviour is still wrong because the total-deltas for mouse-wheel generated scroll-events is still not 0 but this has probably another root cause.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Apr 3, 2020

These are all separate issues that could be explored with follow-on bugs. I'm not sure what the right behavior is for these or whether there is enough information from the OS to do anything about them (e.g., I suspect there is no way to get the touch count for these trackpad-generated gestures on macOS, nor do I really think it would be useful).

You might file a follow-on issue for 1 and 3.

Would you be able to review this? If not I'll ask @johanvos or @pankaj-bansal to be the second reviewer.

@mipastgt
Copy link
Member

@mipastgt mipastgt commented Apr 3, 2020

I think 1. is more a point for discussion and I am also not sure what is right or wrong but 3. is IMHO a bug, especially because the documentation is so explicit about this behaviour. I'll file a new issue for that.

What would I have to do to formally review this? I haven't done that before.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Apr 3, 2020

I agree about 1 being a point of discussion. Please do file a bug for 3.

As for formally reviewing it, you would press the Files changed tab near the top of the PR, add any inline comments you might have, and then press the Review changes pull down. You would add any overall review comments that you have, then select one of Comment (to add comments or questions before approving), Approve (this means you have reviewed it and are OK with the changes; you will be listed as a reviewer), or Request changes (to indicate a bug or other serious problem), and then press Submit Review.

if (gesturesBeganMask == 0) {
[self sendJavaGestureBeginEvent:theEvent];
}
gesturesBeganMask |= theMask;

This comment has been minimized.

@arapte

arapte Apr 6, 2020
Member

Rotate and Magnify gestures can be started and performed together.
If Rotate is already in progress then gesturesBeganMask would be set to 4.
and If we start a Magnify gesture while the Rotate is not ended then sendJavaGestureBeginEvent will not be executed for Magnify gesture.
However the application currently receives the synthetic ZOOM_STARTED event, which is generated as a result of call to sendJavaGestureEvent and not an event generated from here,

A change in inner if statement as below can fix the issue,

if ((gesturesBeganMask & theMask) == 0) {
    gesturesBeganMask |= theMask;
    [self sendJavaGestureBeginEvent:theEvent]
}

This comment has been minimized.

@kevinrushforth

kevinrushforth Apr 6, 2020
Author Member

This is a good thought, and one I had initially considered, but didn't do for two related reasons:

  1. The macOS NSEvent code that sends beginGestureWithEvent and endGestureWithEvent when the JDK is compiled with an older SDK doesn't send a begin call when starting a rotate gesture if a zoom gesture is already active, or vice versa. One goal of this change is to match the previous behavior to minimize risk.
  2. More importantly, the existing logic is not expecting nested sendJavaGestureBeginEvent / sendJavaGestureEndEvent calls, and will not track the state correctly if they are nested. I did a quick test implementing your suggestion and confirmed that it misbehaves.

Worth noting, the logic that synthesizes the begin and end gesture notifications that are sent by the Java gesture code already takes care of the case of switching from zoom to rotate and sends the correct sequence to the event listener.

if (gesturesBeganMask == 0) {
[self sendJavaGestureEndEvent:theEvent];
}
}

This comment has been minimized.

@arapte

arapte Apr 6, 2020
Member

In continuation to the change in maybeBeginGestureWithEvent, we would need to remove the inner if condition change here,

if ((gesturesBeganMask & theMask) != 0) {
    gesturesBeganMask &= ~theMask;
    [self sendJavaGestureEndEvent:theEvent];
}

@arapte
arapte approved these changes Apr 6, 2020
Copy link
Member

@arapte arapte left a comment

looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2020

@kevinrushforth This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8236971: [macos] Gestures handled incorrectly due to missing events

Reviewed-by: mpaus, arapte
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 560ef1701dbb02acacc40976b1bd4023888a24ff.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 6, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Apr 6, 2020

/integrate

@openjdk openjdk bot closed this Apr 6, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2020

@kevinrushforth The following commits have been pushed to master since your change was applied:

  • 560ef17: 8241455: Memory leak on replacing selection/focusModel
  • 5906521: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034

Your commit was automatically rebased without conflicts.

Pushed as commit 247a65d.

@kevinrushforth kevinrushforth deleted the kevinrushforth:8236971-fix-gestures branch Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants