Reland: MouseRegion enter/exit event can be triggered with button pressed #83253

dkwingsmt · 2021-05-24T12:14:49Z

This PR relands #81148, which fixes a bug where mouse pointers can't trigger enter/exit events when pressed, credit to @xu-baolin .

Additionally, this PR also fixes MouseTracker to no longer trigger hit tests for non-mouse pointers.

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

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

…tton pressed (flutter#81148)" (flutter#81557)" This reverts commit d97f41c.

…425mouse

goderbauer · 2021-06-02T21:41:24Z

(PR triage) @dkwingsmt Do you still need this?

…425mouse

dkwingsmt · 2021-06-09T22:19:56Z

It seems that this PR does bring behavioral changes to internal tests in an unexpected way. I would investigate further but still haven't got time for it.

xu-baolin · 2021-06-10T13:32:07Z

Thanks.

xu-baolin · 2021-06-11T13:26:23Z

packages/flutter/lib/src/rendering/binding.dart

@@ -282,7 +282,14 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
        event is PointerAddedEvent ||
        event is PointerRemovedEvent) {
      assert(event.position != null);
-      _mouseTracker!.updateWithEvent(event, () => hitTestResult ?? renderView.hitTestMouseTrackers(event.position));
+      _mouseTracker!.updateWithEvent(


Could we only call _mouseTracker!.updateWithEven when it is a mouse event?

The idea is that RendererBinding provides all events and a way to compute hit test result for MouseTracker to retrieve when needed. In this way most logic is hidden in MouseTracker. Nevertheless I agree that the hit testing function is a little cumbersome. I'll take a moment to think if there's a better way but I'm probably sticking to it in the end.

It turns out that the if clause is unnecessary at all (according to the documentation, which I trust). Several assertions can also be removed since the NNBD. This should feel much cleaner.

gspencergoog

gspencergoog · 2021-06-11T17:49:40Z

packages/flutter/lib/src/rendering/binding.dart

@@ -282,7 +282,14 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
        event is PointerAddedEvent ||
        event is PointerRemovedEvent) {
      assert(event.position != null);
-      _mouseTracker!.updateWithEvent(event, () => hitTestResult ?? renderView.hitTestMouseTrackers(event.position));
+      _mouseTracker!.updateWithEvent(


xu-baolin

LGTM

xu-baolin · 2021-06-12T00:34:20Z

packages/flutter/lib/src/rendering/mouse_tracker.dart

  void updateWithEvent(PointerEvent event, ValueGetter<HitTestResult> getResult) {
-    assert(event != null);
-    final HitTestResult result = event is PointerRemovedEvent ? HitTestResult() : getResult();
-    assert(result != null);
    if (event.kind != PointerDeviceKind.mouse)
      return;
    if (event is PointerSignalEvent)
      return;


small nit:

if (event.kind != PointerDeviceKind.mouse || event is PointerSignalEvent) return;

I slightly prefer the current way since they're describing 2 different things and splitting them is clearer.

xu-baolin · 2021-06-12T00:42:18Z

packages/flutter/lib/src/rendering/binding.dart

@@ -278,12 +278,14 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture

  @override // from GestureBinding
  void dispatchEvent(PointerEvent event, HitTestResult? hitTestResult) {
-    if (hitTestResult != null ||
-        event is PointerAddedEvent ||
-        event is PointerRemovedEvent) {


Why did you think that this clause is unnecessary ?
For example hitTestResult == null && event is PointerMoveEvent, it seems that change some behaviours , right?

According to https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/gestures/binding.dart#L390,

/// The `hitTestResult` argument may only be null for [PointerAddedEvent]s or /// [PointerRemovedEvent]s.

Thanks for explanation, LGTM again!

Revert "Revert "MouseRegion enter/exit event can be triggered with bu…

01e3816

…tton pressed (flutter#81148)" (flutter#81557)" This reverts commit d97f41c.

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 24, 2021

google-cla bot added the cla: yes label May 24, 2021

Merge commit 'a217ab601ad6e8e4d6582ac6ed573c4d101f85e1' into reland-0…

01cc7bc

…425mouse

Merge commit '4562cce44b78e186661ac42e3be423367349fbfe' into reland-0…

a855653

…425mouse

dkwingsmt added 3 commits June 11, 2021 02:14

Merge remote-tracking branch 'upstream/master' into reland-0425mouse

8fcccea

Basic test

e68e33b

Add another test

39cc337

dkwingsmt marked this pull request as ready for review June 11, 2021 11:30

dkwingsmt changed the title ~~[WIP] Test if #81148 breaks~~ Reland: MouseRegion enter/exit event can be triggered with button pressed Jun 11, 2021

dkwingsmt requested a review from gspencergoog June 11, 2021 12:17

xu-baolin reviewed Jun 11, 2021

View reviewed changes

gspencergoog approved these changes Jun 11, 2021

View reviewed changes

xu-baolin approved these changes Jun 11, 2021

View reviewed changes

Move logic

10ff30f

xu-baolin reviewed Jun 12, 2021

View reviewed changes

dkwingsmt merged commit e708aa6 into flutter:master Jun 12, 2021

dkwingsmt deleted the reland-0425mouse branch June 12, 2021 03:31

This was referenced Aug 20, 2021

MouseRegion does not behave properly while dragging a Draggable over a MouseRegion #69458

Closed

MouseRegion onEnter/onExit with button pressed not firing #83092

Closed

MouseRegion rebuilds every time when the pointer does not tap up #80942

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reland: MouseRegion enter/exit event can be triggered with button pressed #83253

Reland: MouseRegion enter/exit event can be triggered with button pressed #83253

Uh oh!

dkwingsmt commented May 24, 2021 •

edited

Loading

Uh oh!

goderbauer commented Jun 2, 2021

Uh oh!

dkwingsmt commented Jun 9, 2021

Uh oh!

xu-baolin commented Jun 10, 2021

Uh oh!

xu-baolin Jun 11, 2021

Uh oh!

gspencergoog Jun 11, 2021

Uh oh!

dkwingsmt Jun 11, 2021

Uh oh!

dkwingsmt Jun 11, 2021

Uh oh!

gspencergoog left a comment

Uh oh!

gspencergoog Jun 11, 2021

Uh oh!

xu-baolin left a comment

Uh oh!

xu-baolin Jun 12, 2021

Uh oh!

dkwingsmt Jun 12, 2021

Uh oh!

xu-baolin Jun 12, 2021

Uh oh!

dkwingsmt Jun 12, 2021 •

edited

Loading

Uh oh!

xu-baolin Jun 12, 2021

Uh oh!

Uh oh!

Reland: MouseRegion enter/exit event can be triggered with button pressed #83253

Reland: MouseRegion enter/exit event can be triggered with button pressed #83253

Uh oh!

Conversation

dkwingsmt commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

goderbauer commented Jun 2, 2021

Uh oh!

dkwingsmt commented Jun 9, 2021

Uh oh!

xu-baolin commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkwingsmt commented May 24, 2021 •

edited

Loading

dkwingsmt Jun 12, 2021 •

edited

Loading