Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` #55157

gmackall · 2024-09-12T20:04:12Z

... when the input device type is touch.

Fixes (partially) flutter/flutter#154842 for touch events. Does not fix when using a stylus, that case will require a follow up PR.

Without fix:

wofix.mp4

With fix:

wfix.mp4

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gmackall · 2024-09-12T22:34:08Z

@moffatman Added you as a reviewer based on git history/github suggestion, feel free to remove if you think you aren't the right person.

moffatman · 2024-09-13T03:24:27Z

won't this break android external mouse? when you click, it will remove the mouse pointer that is being used for hover? the approach doesn't seem right...

gmackall · 2024-09-13T05:35:11Z

won't this break android external mouse? when you click, it will remove the mouse pointer that is being used for hover? the approach doesn't seem right...

I can find an external mouse to test, but I suspect it won't because we synthesize add events when the pointer is missing in most places.

It didn't break hovering with a mouse when mirroring my phone to AS, but I'm not sure how similarly that behaves to an actual external mouse.

gmackall · 2024-09-13T05:45:37Z

I can also change to only synthesize remove events when the device is one of { PointerDeviceKind.TOUCH, PointerDeviceKind.STYLUS, PointerDeviceKind.INVERTED_STYLUS} , if you prefer.

moffatman · 2024-09-13T15:59:23Z

Yep that sounds good for touch I agree no need to keep the pointer around

…est it

gmackall · 2024-09-13T18:57:13Z

Yep that sounds good for touch I agree no need to keep the pointer around

Updated to only remove if the input device is one of { PointerDeviceKind.TOUCH, PointerDeviceKind.STYLUS, PointerDeviceKind.INVERTED_STYLUS }

dkwingsmt · 2024-09-13T23:40:37Z

I think this change makes sense, since for a touch screen, once the pointer becomes up, the pointer can be considered removed. But why do you include styli as well? Many styli allow hovering (by literally hovering the stylus slightly above the screen).

gmackall · 2024-09-13T23:49:12Z

I think this change makes sense, since for a touch screen, once the pointer becomes up, the pointer can be considered removed. But why do you include styli as well? Many styli allow hovering (by literally hovering the stylus slightly above the screen).

Mentioned in another chat but this was due to me not knowing that a stylus can hover, thanks for pointing it out! Changed to only remove for PointerDeviceKind.TOUCH.

dkwingsmt · 2024-09-13T23:49:18Z

Yeah I think while this PR definitely fixes the issue for touch screen, it doesn't for styli. As styli support hovering, I don't think we should remove hovering. However, they do suffer from the issue, since if the user puts away the stylus and then use the stylus to press a button, then the button receives a HOVER event and then a DOWN event, which opens and closes the menu back to back. We should think more carefully how to solve it for styli, but that can be left to another PR.

gmackall · 2024-09-13T23:53:10Z

Yeah I think while this PR definitely fixes the issue for touch screen, it doesn't for styli. As styli support hovering, I don't think we should remove hovering. However, they do suffer from the issue, since if the user puts away the stylus and then use the stylus to press a button, then the button receives a HOVER event and then a DOWN event, which opens and closes the menu back to back. We should think more carefully how to solve it for styli, but that can be left to another PR.

Agreed, I'd be happy to discuss further on the fix for the stylus case

dkwingsmt

Mostly LGTM but I have a question as in the comments.

dkwingsmt · 2024-09-14T00:04:27Z

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

+        // Flutter's pointer handling synthesizes an add event for each new pointer, but does not
+        // synthesize remove events. Remove each pointer on it's corresponding ACTION_UP or
+        // ACTION_POINTER_UP if and only if the input device is touch.
+        // See https://github.com/flutter/flutter/issues/154842.


I rewrote this part so that the motivation is more directly explained instead of sounding just like a patch to an issue. How do you think?

// Synthesizes remove events immediately after the UP event so that the touches // are divided into distinct segments, each beginning with a DOWN event and ending with an UP event. // This approach makes sense since each segment can be considered a separate pointer, // and it prevents Flutter from generating hover events between these segments, which are // not applicable to touch screens, as hovering is not possible. // (Flutter will automatically synthesize an add event before the pointer makes its next contact.)

I think this is a better explanation - changed

dkwingsmt · 2024-09-14T00:07:53Z

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

+
+    int deviceType = getPointerDeviceTypeForToolType(event.getToolType(event.getActionIndex()));
+    boolean shouldRemovePointer =
+        updateForMultiplePointers && (deviceType == PointerDeviceKind.TOUCH);


Can you illustrate why the synthesis is only needed for updateForMultiplePointers?

Oh I see, is it because updateForMultiplePointers is basically this-event-is-up?

Longwinded ish answer: I think the naming makes it a little confusing, but my understanding is that if we take the definition of updateForMultiplePointers

boolean updateForMultiplePointers = !updateForSinglePointer && (maskedAction == MotionEvent.ACTION_UP || maskedAction == MotionEvent.ACTION_POINTER_UP);

which is equivalent to

boolean firstCondition = !(maskedAction == MotionEvent.ACTION_DOWN || maskedAction == MotionEvent.ACTION_POINTER_DOWN); boolean secondCondition = (maskedAction == MotionEvent.ACTION_UP || maskedAction == MotionEvent.ACTION_POINTER_UP); boolean updateForMultiplePointers = firstCondition && secondCondition;

If secondCondition is true, then firstCondition is also guaranteed to be true. And if secondCondition is false, then updateForMultiplePointers is going to be false, regardless of what firstCondition is.
So in reality updateForMultiplePointers is actually just

boolean updateForMultiplePointers = secondCondition; // or boolean updateForMultiplePointers = (maskedAction == MotionEvent.ACTION_UP || maskedAction == MotionEvent.ACTION_POINTER_UP);

Which is exactly the case we are looking to remove pointers for - on ACTION_UP and ACTION_POINTER_UP, aka when a finger leaves the screen.

Oh I see, is it because updateForMultiplePointers is basically this-event-is-up?

Yeah this comment hadn't loaded for me when I wrote the longwinded answer above haha

dkwingsmt

LGTM

…PointerChange.ACTION_POINTER_UP` (flutter/engine#55157)

…155194) flutter/engine@ab9daaa...4d8d851 2024-09-14 34871572+gmackall@users.noreply.github.com Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` (flutter/engine#55157) 2024-09-13 skia-flutter-autoroll@skia.org Roll Skia from bdc5e73cb6c9 to 2b8e33aa4824 (1 revision) (flutter/engine#55192) 2024-09-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 302b6472b849 to c0f7e399ff4a (1 revision) (flutter/engine#55191) 2024-09-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] Scene builder optimizations for platform view placement (#54949)" (flutter/engine#55193) 2024-09-13 flar@google.com Delete VolatilePathTracker in favor of Dispatch tracking (flutter/engine#55125) 2024-09-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 3YH1DEYJ-s93fHBw5... to -kKh_AYzPh_iEmTxK... (flutter/engine#55190) 2024-09-13 skia-flutter-autoroll@skia.org Roll Skia from 9877f459399a to bdc5e73cb6c9 (1 revision) (flutter/engine#55189) 2024-09-13 jonahwilliams@google.com [impeller] add Android flag for disabling surface control for debugging. (flutter/engine#55185) 2024-09-13 skia-flutter-autoroll@skia.org Roll Skia from a5a6d12b3642 to 9877f459399a (2 revisions) (flutter/engine#55187) 2024-09-13 skia-flutter-autoroll@skia.org Roll Dart SDK from eb664303c5ff to 302b6472b849 (2 revisions) (flutter/engine#55182) 2024-09-13 jacksongardner@google.com [skwasm] Scene builder optimizations for platform view placement (flutter/engine#54949) 2024-09-13 reidbaker@google.com add back test itSendsTextShowPasswordToFrameworkOnAttach with new mock for display metrics (flutter/engine#55110) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 3YH1DEYJ-s93 to -kKh_AYzPh_i If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Gray Mackall added 5 commits September 11, 2024 14:00

log

afb817b

wip

6d45475

wip to undo

15d315a

synthesize remove events

418fb9a

format

4c151d1

github-actions bot added the platform-android label Sep 12, 2024

Gray Mackall added 5 commits September 12, 2024 13:05

undo logs, experimental changes

df7f327

undo other log

6542dde

reorder slightly, make pointer count always correct size

8a126d6

ok actually do what the previous commit said

8d8f109

format

b23bbe6

gmackall mentioned this pull request Sep 12, 2024

Fix Menu buttons MouseRegion callbacks misfired on Android flutter/flutter#155023

Closed

9 tasks

Gray Mackall added 3 commits September 12, 2024 14:54

don't include the synthetic pointer event in the pointercount

8323737

add test

035a7a1

format

274111d

gmackall marked this pull request as ready for review September 12, 2024 22:33

gmackall requested review from moffatman and jason-simmons September 12, 2024 22:33

gmackall requested a review from johnmccutchan September 12, 2024 23:38

Gray Mackall added 3 commits September 13, 2024 11:50

change to only remove when stylus, stylus inverted, and touch - and t…

2970d17

…est it

format

6ae1ad4

remove packet assert for scroll wheel

b67584f

Merge branch 'main' into debug_mouseregion

f6b55fd

jason-simmons approved these changes Sep 13, 2024

View reviewed changes

gmackall mentioned this pull request Sep 13, 2024

[Android] Delay on first input flutter/flutter#155176

Open

gmackall requested a review from dkwingsmt September 13, 2024 20:50

Gray Mackall added 4 commits September 13, 2024 16:44

a stylus can hover too, actually - don't remove on action up

1d12768

format

58315fd

comment

8d1762e

comment 2

c74378f

dkwingsmt reviewed Sep 14, 2024

View reviewed changes

Gray Mackall added 2 commits September 13, 2024 17:11

better comment

c67c116

format

be5f1ec

dkwingsmt approved these changes Sep 14, 2024

View reviewed changes

dkwingsmt mentioned this pull request Sep 14, 2024

SubmenuButton submenus close automatically only on first activation [Android] flutter/flutter#154842

Open

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 14, 2024

auto-submit bot merged commit 4d8d851 into flutter:main Sep 14, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Sep 14, 2024

Roll Flutter Engine from ab9daaa0bcc7 to 4d8d8517a0ff (12 revisions) flutter/flutter#155194

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2024

4d8d8517a Synthesize remove events on PointerChange.ACTION_UP and `…

7be6882

…PointerChange.ACTION_POINTER_UP` (flutter/engine#55157)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` #55157

Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` #55157

gmackall commented Sep 12, 2024 •

edited

Loading

gmackall commented Sep 12, 2024

moffatman commented Sep 13, 2024

gmackall commented Sep 13, 2024

gmackall commented Sep 13, 2024

moffatman commented Sep 13, 2024

gmackall commented Sep 13, 2024

dkwingsmt commented Sep 13, 2024

gmackall commented Sep 13, 2024

dkwingsmt commented Sep 13, 2024 •

edited

Loading

gmackall commented Sep 13, 2024

dkwingsmt left a comment

dkwingsmt Sep 14, 2024

gmackall Sep 14, 2024

dkwingsmt Sep 14, 2024

dkwingsmt Sep 14, 2024

gmackall Sep 14, 2024 •

edited

Loading

gmackall Sep 14, 2024

dkwingsmt left a comment

Synthesize remove events on PointerChange.ACTION_UP and PointerChange.ACTION_POINTER_UP #55157

Synthesize remove events on PointerChange.ACTION_UP and PointerChange.ACTION_POINTER_UP #55157

Conversation

gmackall commented Sep 12, 2024 • edited Loading

Pre-launch Checklist

gmackall commented Sep 12, 2024

moffatman commented Sep 13, 2024

gmackall commented Sep 13, 2024

gmackall commented Sep 13, 2024

moffatman commented Sep 13, 2024

gmackall commented Sep 13, 2024

dkwingsmt commented Sep 13, 2024

gmackall commented Sep 13, 2024

dkwingsmt commented Sep 13, 2024 • edited Loading

gmackall commented Sep 13, 2024

dkwingsmt left a comment

Choose a reason for hiding this comment

dkwingsmt Sep 14, 2024

Choose a reason for hiding this comment

gmackall Sep 14, 2024

Choose a reason for hiding this comment

dkwingsmt Sep 14, 2024

Choose a reason for hiding this comment

dkwingsmt Sep 14, 2024

Choose a reason for hiding this comment

gmackall Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

gmackall Sep 14, 2024

Choose a reason for hiding this comment

dkwingsmt left a comment

Choose a reason for hiding this comment

Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` #55157

Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` #55157

gmackall commented Sep 12, 2024 •

edited

Loading

dkwingsmt commented Sep 13, 2024 •

edited

Loading

gmackall Sep 14, 2024 •

edited

Loading