Skip to content

[web] Notify engine of handled PointerScrollEvents.#145500

Merged
auto-submit[bot] merged 10 commits intoflutter:masterfrom
ditman:mutable-pointer-data-packet
Jun 10, 2024
Merged

[web] Notify engine of handled PointerScrollEvents.#145500
auto-submit[bot] merged 10 commits intoflutter:masterfrom
ditman:mutable-pointer-data-packet

Conversation

@ditman
Copy link
Member

@ditman ditman commented Mar 20, 2024

Notifies the engine when PointerSignalEvents have been ignored by the framework, through the ui.PointerData.respond method.

This allows the web to "preventDefault" (or not) on wheel events.

Issues

Tests

  • Added tests to ensure respond is called at the right time, with the right value.

Demo

Previous versions

  1. Modified PointerScrollEvent, not shippable.
  2. Modified when events were handled, instead of the opposite.

Pre-launch Checklist

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. f: gestures flutter/packages/flutter/gestures repository. labels Mar 20, 2024
@ditman ditman changed the title Add ability to ignore PointerScrollEvents. [web] RFC - Add ability to ignore PointerScrollEvents. Mar 20, 2024
@ditman ditman requested review from goderbauer and yjbanov March 20, 2024 21:40
@ditman ditman changed the title [web] RFC - Add ability to ignore PointerScrollEvents. [web] RFC - Notify engine of unhandled PointerScrollEvents. Apr 11, 2024
@ditman

This comment was marked as outdated.

@ditman ditman requested review from goderbauer and mdebbar April 11, 2024 21:30
@ditman ditman force-pushed the mutable-pointer-data-packet branch 2 times, most recently from 7522da2 to 33cfcaf Compare May 10, 2024 20:26
@ditman ditman changed the title [web] RFC - Notify engine of unhandled PointerScrollEvents. [web] RFC - Notify engine of handled PointerScrollEvents. May 11, 2024
@ditman
Copy link
Member Author

ditman commented May 11, 2024

I simplified this a good bunch, so it's easy to rip out when we come up with a better (bi-directional) event system. PTAL @goderbauer @yjbanov @mdebbar!

@ditman ditman force-pushed the mutable-pointer-data-packet branch 4 times, most recently from 27da7ff to b1efa0b Compare May 24, 2024 02:10
@ditman ditman force-pushed the mutable-pointer-data-packet branch from 678fec2 to e61b766 Compare May 29, 2024 04:02
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering why this is implemented as a mixin instead of just defining the respond method directly on PointerSignalEvent?

Copy link
Member Author

@ditman ditman May 29, 2024

Choose a reason for hiding this comment

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

This is implemented as a mixin, because the "_TransformedT" subclasses of T extends PointerSignalEvent are implements (not extends), so I'd have to duplicate the empty method a few times.

(PS: Not a fan of the OOP design of this file, it's starting to show its age!)

(PPS: If there's a cleaner way of doing this, I'm all ears. I just didn't want to add more cruft to the base class, having a specific PointerSignalEvent class.

@github-actions github-actions bot added the a: tests "flutter test", flutter_test, or one of our tests label May 31, 2024
@ditman ditman changed the title [web] RFC - Notify engine of handled PointerScrollEvents. [web] Notify engine of handled PointerScrollEvents. May 31, 2024
@ditman ditman requested review from goderbauer and yjbanov May 31, 2024 23:20
@ditman
Copy link
Member Author

ditman commented May 31, 2024

I think I addressed all the comments @goderbauer, PTAL!

PS: I have a change in:

packages/flutter_test/lib/src/test_pointer.dart

Do I need to split this PR in two, so I can land and version the change to packages/flutter_test, or is that handled by the sdk-dependency on flutter?

auto-submit bot pushed a commit to flutter/engine that referenced this pull request Jun 5, 2024
Adds a function to each 'wheel' DataPacket sent to the framework so it can signal whether to `allowPlatformDefault` or not.

The current default is to always `preventDefault` on browser events that get sent to the framework.

This PR enables the framework to call a method on the `DataPacket`s to `allowPlatformDefault: true`, if the framework won't handle the Signal (signals are handled synchronously on the framework).

This lets the engine "wait" for the framework to decide whether to `preventDefault` on a `wheel` event or not.

## Issues

* Needed for: flutter/flutter#139263

## Tests

* Added unit tests for the feature in the engine repo, veryfing whether the event has had its `defaultPrevented` or not.
* Manually tested in a demo app (see below)

## Demo

* https://dit-multiview-scroll.web.app

<details>
<summary>

## Previous approaches

</summary>

1. Add a `handled` bool property to `PointerDataPacket` that the framework can write to (brittle)
2. Modifications to the `PlatformDispatcher` so the framework can `acknowledgePointerData` with a `PointerDataResponse` (fffffatttt change)
3. `acknowledge` function in `PointerDataPacket`

</details>

> [!IMPORTANT]
> * Related: flutter/flutter#145500

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
ditman added 9 commits June 6, 2024 14:40
This reverts commit 33cfcaf206f7f407b18360388fc63f7fd735f267.
The framework now controls when the scroll events on the web are
stopped.

The PointerEvent now have a respond method that forward a call to the
underlying PointerData.respond implementation.

Some cleanup in the class hierarchy.
@ditman ditman force-pushed the mutable-pointer-data-packet branch from 4c749b5 to f34eef4 Compare June 6, 2024 21:44
@ditman ditman marked this pull request as ready for review June 6, 2024 21:59
@ditman
Copy link
Member Author

ditman commented Jun 6, 2024

This has been rebased to the latest master and everything seems to be working as expected. Will apply autosubmit when CI looks happy.

@ditman
Copy link
Member Author

ditman commented Jun 6, 2024

I've checked the Google testing failures, and the problem is:

ERROR: **third_party/dart/flutter/lib/src/gestures/converter.dart:286**
The getter 'respond' isn't defined for the type 'PointerData'. #undefined_getter

                onRespond: datum.respond,
                                 ^^^^^^^

Which makes sense, because I guess my changes to the engine haven't rolled into Google yet. I'll check again in a couple of days.

@ditman
Copy link
Member Author

ditman commented Jun 7, 2024

(Retrying)

@ditman ditman added autosubmit Merge PR when tree becomes green via auto submit App labels Jun 10, 2024
@auto-submit auto-submit bot merged commit 0c2ee84 into flutter:master Jun 10, 2024
@ditman ditman deleted the mutable-pointer-data-packet branch June 10, 2024 18:15
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 11, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 11, 2024
flutter/flutter@32081aa...14df7be

2024-06-11 [email protected] Revert "Add tests for scaffold drawer and end drawer" (flutter/flutter#150045)
2024-06-11 [email protected] Add tests for scaffold drawer and end drawer (flutter/flutter#149383)
2024-06-11 [email protected] Add high-contrast theme (flutter/flutter#149779)
2024-06-11 [email protected] Manual Pub Roll (flutter/flutter#150025)
2024-06-10 [email protected] [docs] Per-platform desktop triage instructions (flutter/flutter#150019)
2024-06-10 [email protected] Fix copy-paste-o in MethodChannel.invokeListMethod doc (flutter/flutter#149976)
2024-06-10 [email protected] Unpin `camera_android` and remove its only usage (flutter/flutter#150017)
2024-06-10 [email protected] Fixes a bug where NavigatorState.pop does not consider any possible sâ�¦ (flutter/flutter#150014)
2024-06-10 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland: [CupertinoActionSheet] Match colors to native (#149568) (#150015)" (flutter/flutter#150021)
2024-06-10 [email protected] Reland: [CupertinoActionSheet] Match colors to native (#149568) (flutter/flutter#150015)
2024-06-10 [email protected] Temporarily run Mac_arm64 framework_tests_misc on only Mac-13 (flutter/flutter#150009)
2024-06-10 [email protected] Fixes TextField hinttext in a11y_assessment (flutter/flutter#150007)
2024-06-10 [email protected] Use const bool.fromEnvironment("dart.tool.dart2wasm") to detect dart2wasm (flutter/flutter#149996)
2024-06-10 [email protected] Roll Packages from 8a2c4e4 to e95fe4a (3 revisions) (flutter/flutter#149997)
2024-06-10 [email protected] [web] Notify engine of handled PointerScrollEvents. (flutter/flutter#145500)
2024-06-10 [email protected] Cut no-longer-accurate microtask reference in finalizeTree doc (flutter/flutter#149941)
2024-06-10 [email protected] Update hasTrailingSpaces (flutter/flutter#149698)
2024-06-10 [email protected] [web] Change `--web-renderer` default from `auto` to `canvaskit` (flutter/flutter#149773)
2024-06-10 [email protected] Retain the toString method for subclasses of Key in profile/release mode (flutter/flutter#149926)
2024-06-10 [email protected] Remove package:platform from issue template (flutter/flutter#149995)
2024-06-10 [email protected] Revert "[CupertinoActionSheet] Match colors to native (#149568)" (flutter/flutter#149998)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
github-merge-queue bot pushed a commit that referenced this pull request Mar 19, 2026
#183302)

## This PR
- Fixes #152588


## Some history and reasoning
### Root cause
The bug was introduced in PR #145500, which fixed issue #139263 by
ensuring that the Web engine knows when a scroll event is not handled by
Flutter. This allows the browser to perform default actions like page
scrolling or back/forward navigation. Without this, Flutter might
"swallow" events it doesn't use, leading to a "dead" feel on the web.

### Why reverting is safe and won't regress Issue #139263
Reverting the manual `respond(true)` calls in Scrollable is safe because
`PointerSignalResolver` (the centralized mediator for these events)
already has the logic to notify the engine. More specifically,
`PointerSignalResolver.resolve` calls
`event.respond(allowPlatformDefault: true)` if and only if no widget in
the hit-test tree registered interest in the event.

Moving this responsibility back to the `PointerSignalResolver` ensures
that:
1. If **_any_** widget handles the event: It registers with the
resolver, and `respond(true)` is **_not called_**.
2. If **_no_** widget handles the event: No one registers, and the
resolver **_calls_** `respond(true)` at the end of the dispatch cycle.

The current bug exists because `Scrollable` was trying to be too helpful
by calling `respond(true)` based only on its local state, which
overrides any handling done by other widgets in the hierarchy.

### Testing and verifying

A new regression test was created that highlight how the respond calls
should work in nested `Scrollable` elements.

The "fallback" nature of `PointerSignalResolver` is already 
1. documented in `pointer_signal_resolver.dart`:
```dart
  void resolve(PointerSignalEvent event) {
    if (_firstRegisteredCallback == null) {
      assert(_currentEvent == null);
      // Nothing in the framework/app wants to handle the `event`. Allow the
      // platform to trigger any default native actions.
      event.respond(allowPlatformDefault: true);
      return;
    }
    // ... handles the registered callback ...
  }

```
2. tested in `pointer_signal_resolver_test.dart`:
```dart
  test('Resolving with no entries should notify engine of no-op', () {
    var allowedPlatformDefault = false;
    final tester = PointerSignalTester();
    tester.event = PointerScrollEvent(
      onRespond: ({required bool allowPlatformDefault}) {
        allowedPlatformDefault = allowPlatformDefault;
      },
    );
    tester.resolver.resolve(tester.event);
    expect(
      allowedPlatformDefault,
      isTrue,
      reason: 'Should have called respond with allowPlatformDefault: true',
    );
  });

```

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants