add pointer data santizing in flutter web engine #14082

chunhtai · 2019-12-02T23:12:41Z

Framework moved the pointer data sanitization and metrics-related calculation to engine side.
It is now engine's responsibility to provide those metrics and clean pointer data stream.

This pr implements those calculation and sanitization in flutter web engine.
Fixes flutter/flutter#45510

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

chunhtai · 2019-12-03T00:58:35Z

lib/web_ui/test/engine/pointer_binding_test.dart

+      expect(packets[0].data[1].physicalY, equals(10.0));
+      expect(packets[0].data[1].physicalDeltaX, equals(0.0));
+      expect(packets[0].data[1].physicalDeltaY, equals(0.0));
+      // Scroll event will fire twice


I am seeing this weird behavior which I am not sure it is by design or not. glassPane.dispatchEvent(html.WheelEvent(..)) will actually fire two scroll event back to back to glassPaneElement listener

/cc @mdebbar

Hmm, could we possibly have the pointer binding double-subscribed on the glasspane?

It seems like the some tests do not clear the subscription in between test. It will fire more event if i ran the entire test suite

Ah yes, if it's over-firing in tests, then some tests are probably not cleaning up after themselves.

chunhtai · 2019-12-03T01:00:20Z

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

@@ -15,23 +15,17 @@ class PointerBinding {
  static PointerBinding get instance => _instance;
  static PointerBinding _instance;

-  // Set of pointerIds that are added before routing hover and mouse wheel


we no longer need to guess whether the framework will synthesize an add or not, because it won't.

chunhtai · 2019-12-03T01:01:17Z

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

@@ -490,34 +470,6 @@ Duration _eventTimeStampToDuration(num milliseconds) {
  return Duration(milliseconds: ms, microseconds: micro);
 }

-void _ensureMouseDeviceAdded(List<ui.PointerData> data, double clientX,


synthesizing logic is moved to pointer_converter.dart

yjbanov · 2019-12-03T04:39:33Z

@chunhtai

It is not engine's responsibility

Did you mean "It is now"?

yjbanov

@mdebbar should definitely review this. I'll have a second look at it tomorrow.

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

yjbanov · 2019-12-03T04:48:09Z

lib/web_ui/test/engine/pointer_binding_test.dart

+      expect(packets[0].data[1].physicalY, equals(10.0));
+      expect(packets[0].data[1].physicalDeltaX, equals(0.0));
+      expect(packets[0].data[1].physicalDeltaY, equals(0.0));
+      // Scroll event will fire twice


/cc @mdebbar

Hmm, could we possibly have the pointer binding double-subscribed on the glasspane?

mdebbar · 2019-12-03T06:20:40Z

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

+class _PointerState {
+  _PointerState(this.x, this.y);
+
+  int get pointer => _pointer; // The identifier used in framework hit test.


This should probably be a dartdoc comment.

mdebbar · 2019-12-03T06:23:37Z

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

+  _PointerState _ensureStateForPointer(ui.PointerData datum, double x, double y) {
+    return _pointers.putIfAbsent(
+      datum.device,
+        () => _PointerState(x, y),


nit: indentation is off.

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

mdebbar · 2019-12-03T06:35:11Z

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

+
+  /// Convert the given list pointer data into a sequence of framework-compatible
+  /// pointer data.
+  Iterable<ui.PointerData> convert(List<ui.PointerData> data) sync* {


I understand that this is how the Flutter framework used to implement this. But on the web, sync* is expensive and we try to avoid it. Are there any downsides if we change this method to mutate the data list in-place instead of generating a new iterable? (we might also rename the method to make it clear that it mutates the list in-place).

sync* is expensive everywhere, if we're doing it on dart:ui it should be removed

mdebbar · 2019-12-03T06:42:49Z

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

+              yield _synthesizeFrom(datum, ui.PointerChange.add);
+              yield _synthesizeFrom(datum, ui.PointerChange.down);
+            }
+            assert(state.down);


I'm probably missing something here, but it seems like this assertion always fails if alreadyAdded is false. We could do one of two things:

Do state.down = true; when we synthesize PointerChange.down above.

Disallow PointerChange.move if the pointer hasn't already been added.

I'm in favor of option 2, but I'm open for discussing it further.

oh I missing a state.down=true. I added this part when i was confused at #14082 (comment)
I should remove this synthesizing all together

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

tvolkert · 2019-12-03T20:13:52Z

Is this a fix for flutter/flutter#45510?

chunhtai · 2019-12-03T20:31:01Z

@tvolkert yes

chunhtai · 2019-12-03T22:29:51Z

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

-}
-
-List<ui.PointerData> _convertWheelEventToPointerData(
-  html.WheelEvent event,


I moved all these helper function to baseadapter abstract class

chunhtai · 2019-12-03T22:31:38Z

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

-void _addWheelEventListener(void listener(html.WheelEvent e)) {
-  final dynamic eventOptions = js_util.newObject();
-  js_util.setProperty(eventOptions, 'passive', false);
-  js_util.callMethod(PointerBinding.instance.domRenderer.glassPaneElement,


This is a bit confusing to me. If we will call preventDefault, why don't we just use capture = true, and then we can just use the more generic function BaseAdapter. _addEventListener?

I'm not sure I understand.

The browser by default sets passive=true on scroll and wheel event listeners. So we need to explicitly set it to passive=false to be able to call event.preventDefault(). This is independent from capture.

cc @ferhatb who implemented wheel events.

If we use usecapture= true, we don't have to call event.preventDefault()?

Calling event.preventDefault() is necessary regardless of the value of useCapture.

useCapture controls when your listener gets called. If it's true, your listener is called before your children. If it's false, your children are called before your listener.

event.preventDefault tells the browser to not perform the default behavior for that event (which in this case is scrolling). We want Flutter to handle the scrolling, so we prevent the browser from doing any kind of scrolling.

I just check we can still call preventdefault if we have usecapture = true, and from the api documentation of usecapture I can't see why we dont want to use it.

The reason why I want to move away from js_util.callMethod is that i am having trouble remove the listener afterward.

I updated it so that add wheel listener goes through BaseAdapter._addEventListener as well. Let me know if I missed something.

offline discussion: we have to use js interop to remove the wheel event listener (js_util.callMethod(glassPane, 'removeEventListener', ...)).

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

chunhtai · 2019-12-03T22:33:26Z

@mdebbar this is ready for re-review

yjbanov

LGTM if LGT @mdebbar

mdebbar

Great tests! Thanks for fixing this!

…ine#14082)

…ine#14082) (#46049) git@github.com:flutter/engine.git/compare/90e28c027c0b...3e6d6bc git log 90e28c0..3e6d6bc --first-parent --oneline 2019-12-04 47866232+chunhtai@users.noreply.github.com add pointer data santizing in flutter web engine (#14082) 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 chinmaygarde@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

Ahmadre · 2019-12-04T12:20:29Z

Great! Thank you soo much @chunhtai ❤️

add pointer data santizing in flutter web engine

cefc382

auto-assign bot requested a review from flar December 2, 2019 23:12

googlebot added the cla: yes label Dec 2, 2019

chunhtai commented Dec 2, 2019

View reviewed changes

lib/web_ui/lib/src/engine/pointer_binding.dart Show resolved Hide resolved

chunhtai added 2 commits December 2, 2019 15:20

fix license golden

86320f5

added test

42bb5e9

chunhtai commented Dec 3, 2019

View reviewed changes

lib/web_ui/lib/src/engine/pointer_binding.dart Outdated Show resolved Hide resolved

update

228def7

chunhtai commented Dec 3, 2019

View reviewed changes

yjbanov reviewed Dec 3, 2019

View reviewed changes

yjbanov requested a review from mdebbar December 3, 2019 04:48

mdebbar added the platform-web Code specifically for the web engine label Dec 3, 2019

mdebbar reviewed Dec 3, 2019

View reviewed changes

chunhtai added the Work in progress (WIP) Not ready (yet) for review! label Dec 3, 2019

jonahwilliams mentioned this pull request Dec 3, 2019

[web] : Scrolling using touch is broken on the latest version 1.12.15 flutter/flutter#45922

Closed

chunhtai commented Dec 3, 2019

View reviewed changes

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Dec 3, 2019

yjbanov approved these changes Dec 4, 2019

View reviewed changes

refactor pointer binding

71a1504

chunhtai force-pushed the issues/45510 branch from 2a12939 to 71a1504 Compare December 4, 2019 00:32

mdebbar approved these changes Dec 4, 2019

View reviewed changes

chunhtai merged commit 3e6d6bc into flutter:master Dec 4, 2019

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

3e6d6bc add pointer data santizing in flutter web engine (flutter/eng…

474d7c8

…ine#14082)

ferhatb mentioned this pull request Dec 4, 2019

Flutter Draggable doesn't work in Web (Master) Channel flutter/flutter#45959

Closed

dkwingsmt mentioned this pull request Dec 12, 2019

[Web] Fix pointer binding #14378

Merged

mdebbar mentioned this pull request Jan 8, 2020

[web]: When scrolling anywhere on the page, console keeps reporting "Uncaught Error: Assertion failed: is not true" flutter/flutter#32481

Closed

filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020

add pointer data santizing in flutter web engine (flutter#14082)

f2f2a7c

mdebbar mentioned this pull request Apr 8, 2020

[web] Write e2e comprehensive test for pointers flutter/flutter#54311

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add pointer data santizing in flutter web engine #14082

add pointer data santizing in flutter web engine #14082

chunhtai commented Dec 2, 2019 •

edited

Loading

chunhtai Dec 3, 2019 •

edited

Loading

yjbanov Dec 3, 2019

chunhtai Dec 3, 2019

mdebbar Dec 3, 2019

chunhtai Dec 3, 2019

chunhtai Dec 3, 2019

yjbanov commented Dec 3, 2019

yjbanov left a comment

yjbanov Dec 3, 2019

mdebbar Dec 3, 2019

mdebbar Dec 3, 2019

mdebbar Dec 3, 2019

jonahwilliams Dec 4, 2019

mdebbar Dec 3, 2019

chunhtai Dec 3, 2019

tvolkert commented Dec 3, 2019

chunhtai commented Dec 3, 2019

chunhtai Dec 3, 2019

chunhtai Dec 3, 2019

mdebbar Dec 3, 2019

chunhtai Dec 3, 2019

mdebbar Dec 3, 2019

chunhtai Dec 3, 2019

mdebbar Dec 4, 2019

chunhtai Dec 4, 2019

chunhtai commented Dec 3, 2019

yjbanov left a comment

mdebbar left a comment •

edited

Loading

Ahmadre commented Dec 4, 2019

add pointer data santizing in flutter web engine #14082

add pointer data santizing in flutter web engine #14082

Conversation

chunhtai commented Dec 2, 2019 • edited Loading

chunhtai Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov commented Dec 3, 2019

yjbanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvolkert commented Dec 3, 2019

chunhtai commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai commented Dec 3, 2019

yjbanov left a comment

Choose a reason for hiding this comment

mdebbar left a comment • edited Loading

Choose a reason for hiding this comment

Ahmadre commented Dec 4, 2019

chunhtai commented Dec 2, 2019 •

edited

Loading

chunhtai Dec 3, 2019 •

edited

Loading

mdebbar left a comment •

edited

Loading