[web] PointerBinding per view #48248

mdebbar · 2023-11-20T22:55:58Z

Move initialization of PointerBinding/KeyboardBinding out of FlutterViewEmbedder.
computeEventOffsetToTarget properly handles events within a given view.
PointerBinding operates on the given Flutter view (it still listens to some domWindow events for the implicit view).
- Stop using globals e.g. ui.window, KeyboardBinding.instance, SafariPointerEventWorkaround.instance, etc.
pointer_binding_test.dart doesn't use globals either.
clickDebouncer is now a static property on PointerBinding.

tugorez · 2023-12-04T23:57:46Z

lib/web_ui/lib/src/engine/pointer_binding.dart


+  DomElement get _viewTarget => _view.dom.rootElement;
+  DomEventTarget get _globalTarget {
+    // When the Flutter app owns the full page, we want to listen on window for


Isn't returning thew view target implictily covering this case?

Nope. The root element is always a <flutter-view> element that we insert somewhere. But in the implicitView case, we want this to return window.

ditman

Looks good! I had a bunch of comments here and there, but I don't think they're blocking! (I hope! :P)

ditman · 2023-12-08T15:33:08Z

lib/web_ui/lib/src/engine/pointer_binding.dart

+        _detector = detector {
    if (isIosSafari) {
-      SafariPointerEventWorkaround.instance.workAroundMissingPointerEvents();
+      _safariWorkaround = safariWorkaround ?? SafariPointerEventWorkaround();


Looks like this Safari workaround could be a singleton? It only needs to be applied once into the document, and not on every view, right?

(The only thing it's doing is adding an empty listener to document.ontouchstart it seems?)

I made the "default" workaround instance a static member on PointerBinding (so we only ever create one). And the instance only attaches the event listener once (even it gets called multiple times).

ditman · 2023-12-08T15:37:09Z

lib/web_ui/lib/src/engine/embedder.dart

-    KeyboardBinding.initInstance();
-    PointerBinding.initInstance(


So little stuff remaining in this reset function!!

Yeah, I can't wait to get rid of it!

ditman · 2023-12-08T15:42:57Z

lib/web_ui/lib/src/engine/pointer_binding.dart

+  /// Resets global pointer state that's not tied to any single [PointerBinding]
+  /// instance.
+  static void resetGlobalState() {


I cannot find any prod code calling this, so I'm assuming it's only for tests?

Can this be called debugResetGlobalState (or indicate that this is for tests in any other way, with @visibleForTesting or documentation?)

ditman · 2023-12-08T15:53:18Z

lib/web_ui/lib/src/engine/pointer_binding.dart

+    if (_view == EnginePlatformDispatcher.instance.implicitView) {
+      return domWindow;
+    }


Maybe have an eventsTarget getter on the view that returns the proper object? The embedding strategy for "full page" already has a bunch of domDocument hardcoded in certain places, it could return a domWindow very easily.

Also: do you think we could migrate the "embedded" mode to use the rootElement as its target in this PR as well? That way we'd only have two cases:

embedded element -> rootElement

whole window -> domWindow

Rather than distinguishing them by being an "implicitView" or not.

I made the change but I'm not sure if I captured exactly what you wanted. Please see 1de6ab9

ditman · 2023-12-08T16:02:13Z

lib/web_ui/lib/src/engine/pointer_binding.dart

    // Why `domWindow` you ask? See this fiddle: https://jsfiddle.net/ditman/7towxaqp
-    _addPointerEventListener(domWindow, 'pointermove', (DomPointerEvent event) {
+    _addPointerEventListener(_globalTarget, 'pointermove', (DomPointerEvent event) {


(Rant) I wonder if the solution to all this "listening to global events craze" is to have:

A per-view listener that is targeted to the view itself for normal mouse moves

A global listener on window that we can tap from as needed in special cases (when the mouse leaves the window, etc...)

That way we might simplify the general case of pointer handling :/

We do need to figure this out at some point, yes. For example, do we care about hover events happening outside flutter views? What if the pointer is down and moves outside of the view? etc.

ditman · 2023-12-08T16:04:15Z

lib/web_ui/lib/src/engine/pointer_converter.dart

+    assert(() {
+      registerHotRestartListener(reset);
+      return true;
+    }());


Are we landing in the following pattern:

reset for hot restart + tests

dispose for actual code paths that could happen at runtime?

The way I think about these is reset means the instance is back to its original state and can be used further. OTOH, dispose means the object cleaned after itself and can't be used anymore.

ditman · 2023-12-08T16:08:45Z

lib/web_ui/lib/src/engine/pointer_converter.dart

-    assert(_pointers.containsKey(device));
-    final _PointerState state = _pointers[device]!;
+    assert(globalPointerState.pointers.containsKey(device));
+    final _PointerDeviceState state = globalPointerState.pointers[device]!;


I don't mind globalPointerState being a global, but it could be modeled as a property of a PointerDevice object and do:

_PointerDeviceState state = pointerDevices[deviceId].state!;

But since we don't care about anything else of the pointerDevice -> ship it like this!

ditman · 2023-12-08T16:17:59Z

lib/web_ui/test/engine/pointer_binding_test.dart

+    // Remove <body> margins to avoid messing up with all the test coordinates.
+    domDocument.body!.style.margin = '0';


I guess we didn't need this before because the (full-screen) implicitView has this?

Exactly! The full page embedding strategy was doing this for us.

…139936) flutter/engine@5587d26...4c30919 2023-12-11 [email protected] [web] PointerBinding per view (flutter/engine#48248) 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 [email protected],[email protected],[email protected] 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

github-actions bot added the platform-web Code specifically for the web engine label Nov 20, 2023

mdebbar force-pushed the pointer_binding_view branch from 252b4df to df9bbd8 Compare November 22, 2023 21:29

mdebbar force-pushed the pointer_binding_view branch from df9bbd8 to bc10542 Compare November 29, 2023 22:22

mdebbar marked this pull request as ready for review November 29, 2023 22:25

mdebbar requested review from ditman and yjbanov November 29, 2023 22:51

[web] PointerBinding per view

7281436

mdebbar force-pushed the pointer_binding_view branch from bc10542 to 7281436 Compare November 30, 2023 22:17

Merge branch 'main' into pointer_binding_view

ffc4e22

flutter-dashboard bot added the will affect goldens label Dec 1, 2023

flutter deleted a comment from flutter-dashboard bot Dec 1, 2023

Merge branch 'main' into pointer_binding_view

80b2426

tugorez reviewed Dec 4, 2023

View reviewed changes

ditman approved these changes Dec 8, 2023

View reviewed changes

mdebbar added 5 commits December 11, 2023 11:04

Merge branch 'main' into pointer_binding_view

0ad2398

safari workaround attach listener once

5845957

debugResetGlobalState

903d765

embedding strategy global target

1de6ab9

fix initialization order

e36557b

mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 11, 2023

auto-submit bot merged commit 4c30919 into flutter:main Dec 11, 2023

mdebbar deleted the pointer_binding_view branch December 11, 2023 20:54

engine-flutter-autoroll mentioned this pull request Dec 11, 2023

Roll Flutter Engine from 5587d26aa2d4 to 4c309195b79d (1 revision) flutter/flutter#139936

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2023

4c309195b [web] PointerBinding per view (flutter/engine#48248)

3fe65b3

		// Remove <body> margins to avoid messing up with all the test coordinates.
		domDocument.body!.style.margin = '0';

[web] PointerBinding per view #48248

[web] PointerBinding per view #48248

Uh oh!

Conversation

mdebbar commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman 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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdebbar commented Nov 20, 2023 •

edited

Loading