[web] Notify engine of handled PointerScrollEvents.#145500
[web] Notify engine of handled PointerScrollEvents.#145500auto-submit[bot] merged 10 commits intoflutter:masterfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
7522da2 to
33cfcaf
Compare
|
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! |
27da7ff to
b1efa0b
Compare
678fec2 to
e61b766
Compare
There was a problem hiding this comment.
Just wondering why this is implemented as a mixin instead of just defining the respond method directly on PointerSignalEvent?
There was a problem hiding this comment.
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.
|
I think I addressed all the comments @goderbauer, PTAL! PS: I have a change in:
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? |
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
This reverts commit bde00ea.
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.
4c749b5 to
f34eef4
Compare
|
This has been rebased to the latest |
|
I've checked the 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. |
|
(Retrying) |
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
#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.
Notifies the engine when
PointerSignalEventshave been ignored by the framework, through theui.PointerData.respondmethod.This allows the web to "preventDefault" (or not) on
wheelevents.Issues
Tests
respondis called at the right time, with the right value.Demo
Previous versions
PointerScrollEvent, not shippable.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.