Expose GestureBinding.handlePointerEvent, replacing dispatchEvent as the preferred way to dispatch events by dkwingsmt · Pull Request #64846 · flutter/flutter

dkwingsmt · 2020-08-29T01:27:31Z

Description

This PR exposes GestureBinding.handlePointerEvent (previously a private method), which is used to replace all occurrences of GestureBinding.dispatchEvent. This fixes an issue where post-route cancel events can not be received in unit tests, and reduces a number of boilerplates and duplicate code.

Difference between the two methods

The handlePointerEvent takes a PointerEvent. It performs a hit test or reuse existing ones based on event types, then calls dispatchEvent with the event and the result.

The dispatchEvent takes a PointerEvent and a HitTestResult?. It sends the event to every targets of the result, as well as all registered routes.

Improvements

Remove boilerplates: Most of the cases (actually all cases found in this repo), one who calls dispatchEvent only wants to dispatch an event to emulate event dispatching, but since handlePointerEvent is not exposed, it has to perform hitTest and even remember the result itself. Some classes even have to duplicate the entire handlePointerEvent. This PR removes all of these boilerplates.

Expose and fix incorrect tests: A few tests are currently passing incorrectly due to this duplicate logic. These are tests that compares focus highlight of a widget after removing a mouse pointer, but focus highlights should only be displayed when at least one mouse device is connected, so the tests aren't supposed to work. This PR exposes this issue and corrects the tests.

Fix unit test framework: The current structure has an issue where the "post-route cancel events" can't be detected in unit tests, despite working in real apps. Post-route cancel events are synthesized events that are dispatched when there are "down" pointers at the moment a route is popped. These events can't be detected in unit tests because pointer events are dispatched in unit tests using TestGesture, which performs its own hit tests and remembers them (for reasons stated above). In this way GestureBinding._hitTestResults are not aware of these events, and cancelPointer will not cancel anything. This PR fixes this problem by unifying these states.

API Changes

GestureBinding:

New: handlePointerEvent
New protected: resetGesture

TestWidgetsFlutterBinding

New private: pointerEventSource (used in the same file)
handlePointerEvent is overridden with an additional parameter source
dispatchEvent is no longer overridden with an additional parameter source

TestGesture

Constructor
- No longer takes HitTester hitTester
- EventDispatcher dispatcher no longer takes HitTestResult result

WidgetTester

sendEventToBinding changes signature: no longer takes HitTestResult

Related Issues

Required before fixing Cupertino Back swipe gesture causing null-unsafe operations #63984

Tests

I added the following tests:

Popping routes should cancel down events
- This currently works in real apps, but not in unit tests

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 Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

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

Yes, but this is not a breaking change. Some existing tests failed because they are written in a way that does not reflect actual behavior (see the section Expose and fix incorrect tests above), and were not supposed to pass even before this PR. They have been fixed in this PR.

…-in-tests

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

packages/flutter/test/cupertino/route_test.dart

goderbauer · 2020-09-08T21:13:57Z

packages/flutter/test/cupertino/route_test.dart

    expect(find.text('second'), findsOneWidget);
  });
+
+  testWidgets('Popping routes should cancel down events', (WidgetTester tester) async {


how does this assert that the down event is cancelled?

I've added the following comment, is it clearer?

// The test target, _TestPostRouteCancel, shows on the home route how many // [PointerCancelEvent] it has received. Initially it will show "Home 0". // After the route is popped, it should show "Home 1", meaning it received // one [PointerCancelEvent].

packages/flutter_test/lib/src/controller.dart

packages/flutter_test/lib/src/binding.dart

goderbauer · 2020-09-08T22:31:50Z

packages/flutter_test/lib/src/binding.dart

+  TestBindingEventSource pointerEventSource = TestBindingEventSource.device;
+
  @override
-  void dispatchEvent(


Presumably, we would want to keep this override to ensure that only TestBindingEventSource.test reach this method?

It's something I've thought for a long time, and I don't think this design is either sufficient or necessary to achieve what it seems to.
Redefining TestWidgetsFlutterBinding.dispatchEvent provides a way to diverge the behavior of dispatchEvent based on event source, although the new behavior is only implemented by its subclass LiveTestWidgetsFlutterBinding. How it behaves based on the source is shown as follows:

// TestWidgetsFlutterBinding binding; binding.dispatchEvent(event, result) // AssertionError binding.dispatchEvent(event, result, source: TestBindingEventSource.test) // Test behavior (binding as GestureBinding).dispatchEvent(event, result) // Original behavior

But this is why the assertion is unnecessary: If some class calls dispatchEvent without adding source, the class must be unaware of TestWidgetsFlutterBinding, which means it will always call the method as GestureBinding.dispatchEvent. This assertion protects no one.

And why shouldn't any class aware of TestWidgetsFlutterBinding be allowed call the device version? The LiveTestWidgetsFlutterBinding.dispatchEvent even defines its own way of handling device-sourced events. The only dispatchEvent that does not accept device-sourced events is TestWidgetsFlutterBinding's, which itself does not even redefine test-sourced events. If we really want to diverge by the source, we better do it in LiveTestWidgetsFlutterBinding (which does not need it either, since it allows device-sourced events).

What's worse, it forces a test in gesture_binding_resample_event_on_widget_test.dart to shadow the argument.

Since the assertion is not needed, and the divergence is already implemented by handlePointerEvent, and it brings trouble, I can really think of no reason to keep this assertion.

goderbauer · 2020-09-08T22:32:48Z

Can you also check that this API change doesn't break any customers (e.g. google3)?

…-in-tests

dkwingsmt · 2020-09-10T23:48:37Z

@goderbauer I've fixed per your comments, and FRoB shows that this PR does not break g3.

goderbauer

LGTM

goderbauer · 2020-09-14T17:18:47Z

packages/flutter/test/cupertino/route_test.dart

+    await tester.pumpWidget(_TestPostRouteCancel());
+
+    final TestGesture gesture = await tester.createGesture();
+    await gesture.down(tester.getCenter(find.text('Home 0')));


I think the intentions of the test would be a little clearer if the app would more clearly say what the "0" means. You could for example have an additional text widget that says "PointerCancelEvents: 0". The test would then be self-explanatory and you wouldn't even need the comment above.

goderbauer · 2020-09-14T17:25:26Z

packages/flutter_test/lib/src/binding.dart

+  // The [pointerEventSource] is set as the `source` parameter of
+  // [handlePointerEvent] and can be used in the immediate enclosing
+  // [dispatchEvent].
+  TestBindingEventSource _pointerEventSource = TestBindingEventSource.device;


Looks like this property (and the logic for setting/retrieving it) can be moved into the LiveTestWidgetsFlutterBinding class since it is only needed there?

Sadly not. WidgetTester calls the handlePointerEvent with source but WidgetTester.binding is of type TestWidgetFlutterBinding, despite its concrete type being LiveTestWidgetsFlutterBinding.

…-in-tests

CareF · 2020-09-23T21:21:17Z

Thanks.
This may open up possible improvement of the smoothness test to use PointerEvent rather than PointerData. @liyuqian

liyuqian · 2020-09-25T00:03:40Z

@dkwingsmt : can you please check if all these big jumps are expected? At least some of them seem to be unexpected according to #66608

liyuqian · 2020-09-25T00:07:37Z

Also CC @kf6gpe as triaging this alert might be difficult. Maybe some of them are legit improvements while some are unexpected regressions. Let's wait for @dkwingsmt 's check, and then figure out if we need to ask help from Skia perf to see how to handle this difficult case.

dkwingsmt added 4 commits August 26, 2020 21:06

Add test

21638f1

First passing impl

4c816e0

Add docs

8dd55d9

Fix others

69c7dba

googlebot added the cla: yes label Aug 29, 2020

dkwingsmt added 4 commits August 28, 2020 18:59

Merge remote-tracking branch 'upstream/master' into post-route-cancel…

4ef8bbf

…-in-tests

Merge remote-tracking branch 'upstream/master' into post-route-cancel…

52cad9c

…-in-tests

Focus color is only available when a mouse is connected

10ff41e

API fix

d812824

flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Aug 31, 2020

dkwingsmt added 4 commits August 31, 2020 15:32

Fix test

b2bac2c

Don't reset pointer route

b5c4aa1

Merge remote-tracking branch 'upstream/master' into post-route-cancel…

95a3929

…-in-tests

Merge remote-tracking branch 'upstream/master' into post-route-cancel…

5392cd1

…-in-tests

goderbauer reviewed Sep 8, 2020

View reviewed changes

dkwingsmt added 2 commits September 10, 2020 15:38

Fix per comments

437ba45

Merge remote-tracking branch 'upstream/master' into post-route-cancel…

a491070

…-in-tests

goderbauer approved these changes Sep 14, 2020

View reviewed changes

dkwingsmt and others added 4 commits September 14, 2020 13:50

Merge remote-tracking branch 'upstream/master' into post-route-cancel…

81cf02f

…-in-tests

Make test self explanatory

5a651c7

Fix signatures

e260a7c

PointerDataTestBinding

17ea6e8

Hixie mentioned this pull request Sep 15, 2020

Cupertino Back swipe gesture causing null-unsafe operations #63984

Closed

dkwingsmt added the waiting for tree to go green label Sep 15, 2020

fluttergithubbot merged commit a48e143 into flutter:master Sep 16, 2020

dkwingsmt mentioned this pull request Sep 23, 2020

Navigator can still get spurious writes during navigation #4770

Open

3 tasks

dkwingsmt deleted the post-route-cancel-in-tests branch September 23, 2020 22:14

prefanatic mentioned this pull request May 14, 2021

[integration_test] HitTest regression with setSurfaceSize #82557

Closed

goderbauer mentioned this pull request May 20, 2021

[flutter_test/integration_test] added setSurfaceSize test coverage #82712

Merged

8 tasks

goderbauer mentioned this pull request Jul 13, 2021

[codelab][flutter_app_testing] Widget test fails when run on device #82542

Closed

Conversation

dkwingsmt commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Difference between the two methods

Improvements

API Changes

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

goderbauer Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 10, 2020 • 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!

goderbauer commented Sep 8, 2020

Uh oh!

dkwingsmt commented Sep 10, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

goderbauer Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 14, 2020 • 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!

CareF commented Sep 23, 2020

Uh oh!

liyuqian commented Sep 25, 2020

Uh oh!

liyuqian commented Sep 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dkwingsmt commented Aug 29, 2020 •

edited

Loading

dkwingsmt Sep 10, 2020 •

edited

Loading

dkwingsmt Sep 14, 2020 •

edited

Loading