Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Correct button state on synthetic pointer events #23111

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Dec 16, 2020

Description

This corrects the button state emitted on synthetic pointer move and
hover events generated by the engine.

When an embedder notifies the engine of a pointer up or down event, and
the pointer's position has changed since the last move or hover event,
PointerDataPacketConverter generates a synthetic move or hover to
notify the framework of the change in position. In these cases, the
current event from the embedder contains the new button state after
the pointer up/down event, but the move/hover needs to be synthesized
such that it occurs before the pointer up/down, with the previous
button state.

This patch stores the button state after each pointer down, up, move, or
hover event such that it can be used by the next event if a synthetic
event must be issued.

The bug in the previous logic was revealed by the release of macOS 11
(Big Sur), which appears to issue move events between mouse down and
mouse up, which did not use to be the case.

Related Issues

This fixes flutter/flutter#64961, which is the
desktop-specific tracking bug for the more general Big Sur mouse click
umbrella issue flutter/flutter#71190.

Tests

I added the following tests:

  • Test that synthesised move and hover events have the correct button state.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@cbracken cbracken requested review from ferhatb and chunhtai December 16, 2020 18:16
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. This change looks good, just need a test

@chunhtai
Copy link
Contributor

We may have same issue in web pointer converter https://github.com/flutter/engine/blob/master/lib/web_ui/lib/src/engine/pointer_converter.dart

Not sure if this will affect web

@cbracken
Copy link
Member Author

cbracken commented Dec 16, 2020

The web side was already landed in #22813. Test WIP but should be momentarily :)

This corrects the button state emitted on synthetic pointer move and
hover events generated by the engine.

When an embedder notifies the engine of a pointer up or down event, and
the pointer's position has changed since the last move or hover event,
`PointerDataPacketConverter` generates a synthetic move or hover to
notify the framework of the change in position. In these cases, the
current event from the embedder contains the new button state *after*
the pointer up/down event, but the move/over needs to be synthesized
such that it occurs *before* the pointer up/down, with the previous
button state.

This patch stores the button state after each pointer down, up, move, or
hover event such that it can be used by the next event if a synthetic
event must be issued.

The bug in the previous logic was revealed by the release of macOS 11
(Big Sur), which appears to issue move events between mouse down and
mouse up, which did not use to be the case.

This fixes flutter/flutter#64961, which is the
desktop-specific tracking bug for the more general Big Sur mouse click
umbrella issue flutter/flutter#71190.
@cbracken
Copy link
Member Author

Tests added -- caught an issue with synthesised events emitted when scrolling.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cbracken cbracken merged commit 69d7e8e into flutter:master Dec 16, 2020
@cbracken cbracken deleted the correct-buttons-for-move-hover branch December 16, 2020 19:57
Copy link
Contributor

@ferhatb ferhatb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2020
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021
This corrects the button state emitted on synthetic pointer move and
hover events generated by the engine.

When an embedder notifies the engine of a pointer up or down event, and
the pointer's position has changed since the last move or hover event,
`PointerDataPacketConverter` generates a synthetic move or hover to
notify the framework of the change in position. In these cases, the
current event from the embedder contains the new button state *after*
the pointer up/down event, but the move/hover needs to be synthesized
such that it occurs *before* the pointer up/down, with the previous
button state.

This patch stores the button state after each pointer down, up, move, or
hover event such that it can be used by the next event if a synthetic
event must be issued.

The bug in the previous logic was revealed by the release of macOS 11
(Big Sur), which appears to issue move events between mouse down and
mouse up, which did not use to be the case.

This fixes flutter/flutter#64961, which is the
desktop-specific tracking bug for the more general Big Sur mouse click
umbrella issue flutter/flutter#71190.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] Clicks don't always register on Big Sur
4 participants