8294426: Two fingers tap generates wrong mouse modifiers on M2 MacBooks

[OnePatchGuy]

Hi there!
JetBrains has faced with a bug on Apple M2 MacBooks when tapping (not pressing) with two fingers on a trackpad generates wrong mouse modifiers (which are returned by MouseEvent.getModifiersEx).

As far as I see, OpenJDK bug tracker still doesn't contain such a bug and I don't have rights to create a new one, so the PR doesn't refer any id UPD: fixed. Here is the bug link from the JetBrains own tracker: JBR-4765.

The bug is 100% reproducible on M2 MacBooks (at least under macOS 12.5). It's also reproducible on M1 MacBooks, but much more rarely (about 10-15% of reproducibility).

Steps to reproduce

1. Enable System Preferences -> Trackpad -> Tap to click
2. Tap with two fingers in the following app:

import javax.swing.*;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;

class MouseWindow {
    final JFrame frame;

    MouseWindow() {
        frame = new JFrame();
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

        frame.setTitle("Mouse window");

        frame.addMouseListener(new MouseAdapter() {
            @Override
            public void mousePressed(MouseEvent e) {
                System.out.println(e);
            }
        });

        frame.pack();

        frame.setSize(300, 300);

        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(MouseWindow::new);
    }
}

Expected

Printed mouse event has modifiersEx is 4096 (which is InputEvent.BUTTON3_DOWN_MASK)

Actual

Printed mouse event has modifiersEx is 4352 (which is InputEvent.BUTTON3_DOWN_MASK | InputEvent.META_DOWN_MASK)

Evaluation

The following happens when a native mouse event reaches Java:

1. NSEvent.nsToJavaModifiers(0) called inside CPlatformResponder.handleMouseEvent():81 returns 0. Earlier it always returned 4096 (InputEvent.BUTTON3_DOWN_MASK) but not in the cases of M2 tapping. For a trackpad press (not a tap) it still returns 4096;
2. So, the 0 modifier comes into MouseEvent constructor which then goes into MouseEvent.setOldModifiers() which initializes the field MouseEvent.modifiers to the value 4 (it's InputEvent.BUTTON3_MASK);
3. Next, this constructed MouseEvent object is pushed into EDT queue.
4. Next, when a EDT thread pulls the event from the queue and starts to process it, it goes into java.awt.LightweightDispatcher.retargetMouseEvent and creates a new MouseEvent based on the pulled one with the new value of the modifiers == getModifiersEx() | getModifiers(). From the p.2 we know, that MouseEvent.modifiers of the pulled event is 4, so getModifiersEx() | getModifiers() is evaluated to 4.
5. Next, the constructor of the new MouseEvent goes into setNewModifiers() (instead of setOldModifiers as in p.2) and initializes its own field modifiers to the value 4356 (InputEvent.BUTTON3_MASK | InputEvent.BUTTON3_DOWN_MASK | InputEvent.META_DOWN_MASK, see setNewModifiers():1099, setNewModifiers():1129).
6. Thus, an app receives the instance of MouseEvent with getModifiers() == 4 and getModifiersEx() == 4352 so it thinks there is a Command keystroke.

Fixing

Let's set manually inside CPlatformResponder.handleMouseEvent a mouse modifier which corresponds to the pressed button.

What about a regression test

I've failed to write a regression test using Robot because it somehow forces the correct mouse modifiers (NSEvent.nsToJavaModifiers() correctly returns InputEvent.BUTTON3_DOWN_MASK), so I wrote a test that directly invokes CPlatformResponder.handleMouseEvent via reflection.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8294426: Two fingers tap generates wrong mouse modifiers on M2 MacBooks

Reviewers

• Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10429/head:pull/10429
$ git checkout pull/10429

Update a local copy of the PR:
$ git checkout pull/10429
$ git pull https://git.openjdk.org/jdk pull/10429/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10429

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10429.diff

[bridgekeeper]

👋 Welcome back NikitkoCent! 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]

@NikitkoCent 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.

[mkartashev]

I filed https://bugs.openjdk.org/browse/JDK-8294426 for this issue.

[mlbridge]

Webrevs

• 00: Full (bacac5f7)

[prrace]

You say it returns 0, so you expect to only need to do that if jmodifiers is 0, yet you don't check that.
Why not ?

No way to test this on an M2 so being sure it doesn't regress something is all I can offer here.

[OnePatchGuy]

Actually I mean that it returns 0 if you're tapping without holding any modifiers e.g. keyboard modifiers.
But if you're tapping holding a keyboard modifier it will probably return only this keyboard modifier without InputEvent.BUTTON3_DOWN_MASK. I didn't test this case but I would expect it that looking at the implementation of the NSEvent.nsToJavaModifiers.

[OnePatchGuy]

I don't have an M2 device to check, but I think (judging to the code) if we add jmodifiers == 0 check here then users making a tap holding the Control will receive the mouse events with modifiers == CTRL_DOWN_MASK | BUTTON3_MASK | CTRL_MASK, i.e. without modern BUTTON3_DOWN_MASK which is at least unusual and inconvenient.

[prrace]

OK .. well I've at least run all our automated tests and no regressions were found, so I'll approve this.

[OnePatchGuy]

I've checked behavior of tap holding modifiers on M2 and here are which mouse events are received:

• Ctrl + Two fingers tap:
  • event: java.awt.event.MouseEvent[MOUSE_PRESSED,(86,102),absolute(86,302),button=3,modifiers=⌘+⌃+Button3,extModifiers=⌃,clickCount=1] on MouseWindow$1[,0,136,200x200,layout=java.awt.FlowLayout,alignmentX=0.0,alignmentY=0.0,border=,flags=16777225,maximumSize=,minimumSize=,preferredSize=]
  • modifiers: 6 (⌘+⌃+Button3)
  • modifiersEx: 128 (⌃)
• Option + Two fingers tap:
  • event: java.awt.event.MouseEvent[MOUSE_PRESSED,(63,135),absolute(63,335),button=3,modifiers=⌥+⌘+Button2+Button3,extModifiers=⌥,clickCount=1] on MouseWindow$1[,0,136,200x200,layout=java.awt.FlowLayout,alignmentX=0.0,alignmentY=0.0,border=,flags=16777225,maximumSize=,minimumSize=,preferredSize=]
  • modifiers: 12 (⌥+⌘+Button2+Button3)
  • modifiersEx: 512 (⌥)

Thus, as I assumed, modifiersEx don't contain BUTTON3_DOWN_MASK, so we shouldn't restrict the patch by the jmodifiers == 0 condition.

[openjdk]

@NikitkoCent This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8294426: Two fingers tap generates wrong mouse modifiers on M2 MacBooks

Reviewed-by: prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 254 new commits pushed to the master branch:

• 312985e: 8295156: G1: Improve constant other time calculation
• 786ce1c: 8295024: Cyclic constructor error is non-deterministic and inconsistent
• 1efa93e: 8294844: Improve G1 young gen length revise trigger
• 3dbc38a: 8295288: Some vm_flags tests associate with a wrong BugID
• ef5210f: 8295149: Misnomer for_young_gc instead of for_young_only_phase in G1Analytics
• 64813f4: 8294850: Make rs length/pending card predictors dependent on gc phase
• 21e4f06: 8295274: HelidonAppTest.java fails "assert(event->should_commit()) failed: invariant" from compiled frame"
• f31c80d: 8294842: Pass actual pending cards to G1Policy::update_young_length_bounds during young gen revise
• 7133fc9: 7172359: HTML parser StackOverflowError on invalid HTML:
• tag inside an
• 3d75e88: 8295270: RISC-V: Clean up and refactoring for assembler functions
• ... and 244 more: https://git.openjdk.org/jdk/compare/36b61c5d7e7732924f494fa24c0e286e41279fc3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[OnePatchGuy]

/integrate

[openjdk]

@NikitkoCent
Your change (at version bacac5f) is now ready to be sponsored by a Committer.

[avu]

/sponsor

[openjdk]

Going to push as commit 2da079c.
Since your change was applied there have been 255 commits pushed to the master branch:

• 449b52f: 8295158: G1: Increase card-based cost sample reporting threshold
• 312985e: 8295156: G1: Improve constant other time calculation
• 786ce1c: 8295024: Cyclic constructor error is non-deterministic and inconsistent
• 1efa93e: 8294844: Improve G1 young gen length revise trigger
• 3dbc38a: 8295288: Some vm_flags tests associate with a wrong BugID
• ef5210f: 8295149: Misnomer for_young_gc instead of for_young_only_phase in G1Analytics
• 64813f4: 8294850: Make rs length/pending card predictors dependent on gc phase
• 21e4f06: 8295274: HelidonAppTest.java fails "assert(event->should_commit()) failed: invariant" from compiled frame"
• f31c80d: 8294842: Pass actual pending cards to G1Policy::update_young_length_bounds during young gen revise
• 7133fc9: 7172359: HTML parser StackOverflowError on invalid HTML:
• tag inside an
• ... and 245 more: https://git.openjdk.org/jdk/compare/36b61c5d7e7732924f494fa24c0e286e41279fc3...master

Your commit was automatically rebased without conflicts.

[openjdk]

@avu @NikitkoCent Pushed as commit 2da079c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mrserb]

Can we report this behaviour change to Apple for investigation?

[OnePatchGuy]

/backport jdk17u-dev

[openjdk]

@NikitkoCent To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

[OnePatchGuy]

@mrserb,

Can we report this behaviour change to Apple for investigation?

I think this is our problem here - we ask for the currently pressed modifiers only after the event has happened (and dispatched to us), so there is no guarantee that a user doesn't manage to release the mouse button before the press event is dispatched to us.

Do you agree?