[Web] Fix pointer binding #14378

dkwingsmt · 2019-12-12T01:50:01Z

Overview

This PR refactors pointer_binding, fixes a few issues, and adds a number of tests for all three adapters.

Details

Rewrite adapters

A number of issues are fixed and tested in this PR, namely:

Right clicks that pop up the context menu will cause crashes.
Button changes during a click sequence will cause crashes.
Buttons other than primary or secondary are not recognized.
TouchAdapter does not work with multiple pointers.
Neither TouchAdapter nor PointerAdapter generates remove event for touch end, leading to memory leak.
Cancel events or remove events do not work, due to converters being too strict.
If a wheel event are seen as the first event, PointerAdapter will not properly work.

Some of the issues emerged after #14082, while some have existed since the first write.

This PR introduces the following non-trivial changes:

Expansion of coalesced events is only done during pointermove, as it's only available during this event according to specs.
The buttons bitfield is limited to the right-most 30 bits (_kButtonsMask).

This PR introduces the following minor improvements:

Reduces the duplicate code between PointerAdapter and MouseAdapter.
Removes unnecessary public symbols.
Fixes an error where domRenderer is read during its constructor.

Improve tests

This PR also creates a test environment to test the 3 adapters, and added a number of tests. With the help of "test contexts", we can test all applicable adapters without duplicate code.

mdebbar · 2019-12-12T03:03:10Z

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

+
+  void handleDown({@required _PointerStateDataCallback dataCallback, @required int buttons}) {
+    // TODO(flutter_web): Remove this temporary fix for right click
+    // on web platform once context guesture is implemented.


typo: s/guesture/gesture

Fixed. Also I didn't add this TODO, but only moved it (since there was a corresponding test), and I have no idea why it was there.

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

mdebbar · 2019-12-12T17:41:35Z

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

+    _pressedButtons = 0;
+    dataCallback(change: ui.PointerChange.up, buttons: _pressedButtons);


Same comment as above: did you mean to reverse the order of these two statements?

Why do we want to reverse these two statements? We indeed want to dispatch an up event with "buttons: 0". Events should contain the current state of buttons of this pointer.

Maybe I'm misunderstading something here. I thought we tell Flutter which button are being released with this pointerup event. Is that not true?

Flutter's PointerEvent class doesn't have a "button" property, but only a "buttons" property. https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/gestures/events.dart#L211
The "button" property (as only seen in HTML PointerEvent class) tells which button is changed, while the "buttons" property (as defined in HTML and Flutter) is a bitfield of which buttons are pressed after the event.

Yeah, I meant buttons (and used "are" after it) but I misspelled it 😛

That's good to know, TIL. Is it documented anywhere that buttons specifies the buttons that are pressed after the event?

Not really, and I'll add it when I got the opportunity.

mdebbar · 2019-12-12T17:45:53Z

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

-  ) : super(callback, domRenderer, _pointerDataConverter);
+  ) : super(callback, glassPaneElement, _pointerDataConverter);
+
+  final Map<int, _PointerStateManager> _pointerStates = <int, _PointerStateManager>{};


Concern: this map will keep growing indefinitely since on touch devices, the browser gives us a new pointer id on every interaction.

mdebbar · 2019-12-12T18:00:22Z

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

+    return buttons & _kButtonsMask;
+  }
+
+  void handleDown({@required _PointerStateDataCallback dataCallback, @required int buttons}) {


I wish Dart had support for tuples. We would have avoided the dataCallback and just returned a Tuple<ui.PointerChange, int>.

We can always create a class for that, but I think it's not as flexible as callbacks, since some handlers are stateful, and returning the list would be a little more complicated.

On a second thought, I'll make it a list to address the issue mentioned in #14378 (comment)

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

mdebbar · 2019-12-12T18:14:21Z

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

-    final int len = touches.length;
-    for (int i = 0; i < len; i++) {
-      final html.Touch touch = touches[i];
+    for (html.Touch touch in event.changedTouches) {


The reason we avoided for in here was to avoid the creation of unnecessary iterators.

Are you worrying about performance? I think it's a premature optimization unless it's backed by a benchmark. In my opinion, loops in this file is of the same impact as loops in app files, so if app files do not care about the overhead of for in, so shouldn't we.

I agree it's premature optimization and it's not backed by a benchmark. This specific code path though, could be a gazillion times when the user is moving their fingers. And we know for sure that Dart generates sub-optimal JS code in some cases for for in. That's why we avoided it here.

You can keep the for in if you want. We are gonna have to tackle performance at some point anyway. Then, we would more concrete data to optimize against.

Ok, I'll leave it to you. Thanks for explaining.

…onverter

* 988b8f1 Fix FontLoader does not remove the cache in web engine (flutter/engine#14536) * 0aacac7 Roll src/third_party/skia 21df075cab00..e6a2ad81ab40 (1 commits) (flutter/engine#14552) * 417dd7e Roll fuchsia/sdk/core/mac-amd64 from esDH2... to NHgyx... (flutter/engine#14554) * f2e841d [Web] Fix pointer binding (flutter/engine#14378) * 105eb66 Roll src/third_party/skia e6a2ad81ab40..8fec4140f614 (17 commits) (flutter/engine#14557) * 1ecfdcb [web] Calculate align offset for each paragraph line (LineMetrics.left) (flutter/engine#14537) * dda3619 Roll src/third_party/dart 270966b16044..171059d27689 (19 commits) (flutter/engine#14558) * 9c1bd8a Fixes Objective-C objects memory leaks (flutter/engine#14326) * f2dbeb8 Reland Wire up Opacity on Fuchsia (flutter/engine#14559) * 2f536e5 Roll fuchsia/sdk/core/linux-amd64 from jsuQq... to VdBKA... (flutter/engine#14560) * 4312d37 Revert "[fuchsia] Add diagnostics directory to the set of remote dirs (#14470)" (flutter/engine#14566) * 5c77ea1 Roll src/third_party/skia 8fec4140f614..9e0afb791ac2 (4 commits) (flutter/engine#14563) * a09ff7c Roll src/third_party/dart 171059d27689..aa6709974dea (11 commits) (flutter/engine#14567) * 0f90e65 Revert "[web] Calculate align offset for each paragraph line (LineMetrics.left) (#14537)" (flutter/engine#14569)

Refactors pointer_binding, fixes a few issues related to the pointer data converter in Web, and adds a number of tests for all three adapters.

dkwingsmt added 8 commits December 9, 2019 18:18

Fix right click

c998423

Add button converter, refactor _pressedButtons

9afaffe

Add correct handling of multiple buttons

5ce4989

Use buttons

6d2e42e

Add test for context menu and hold

ac1038f

Refactor into state manager

db4884d

Renames

3e1142b

Remove test function

199b709

auto-assign bot requested a review from stuartmorgan December 12, 2019 01:50

googlebot added the cla: yes label Dec 12, 2019

dkwingsmt requested review from yjbanov and mdebbar December 12, 2019 02:01

dkwingsmt added the platform-web Code specifically for the web engine label Dec 12, 2019

mdebbar reviewed Dec 12, 2019

View reviewed changes

dkwingsmt added 16 commits December 13, 2019 10:49

Rewrite using expand

19dcf7d

Add listener utility method

9b678a9

Add test for Mouse

6051260

Refactor to context

4baa5ba

Test touch

f9fe5c7

Iterator

c83ad2e

Change API

c3a0083

Mixins

7863442

Use array, and rewrite touch

fdbe171

Multi pointer tests

f3251e2

Add todo for cancel

44d29ec

Merge remote-tracking branch 'upstream/master' into fix-web-pointer-c…

1403e6d

…onverter

Add remove events; test event generators

859749e

Fix mouse event generator

e9778e2

Fix mouse adapter

4744b4b

Add doc

447964a

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

bd5dc28

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47413

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

e68bc57

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47415

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

33e6af3

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47417

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

a059f83

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47418

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

9a642bc

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47420

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

15d4efd

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47421

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

05c8b68

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47422

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

0e1a30a

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..a09ff7c424bd (13 commits) flutter/flutter#47423

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

c97b648

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..0f90e6546bf3 (14 commits) flutter/flutter#47424

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

cc4988f

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..0f90e6546bf3 (14 commits) flutter/flutter#47425

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

23838fc

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..0f90e6546bf3 (14 commits) flutter/flutter#47427

Closed

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

f2e841d [Web] Fix pointer binding (flutter/engine#14378)

8fa77bf

engine-flutter-autoroll mentioned this pull request Dec 19, 2019

Roll engine 1e1f37107a07..0f90e6546bf3 (14 commits) flutter/flutter#47429

Merged

ferhatb mentioned this pull request Dec 19, 2019

flutter web: assertion on mouse click flutter/flutter#47440

Closed

This was referenced Jan 3, 2020

Scrollable listview are flingable, but not dragable in web. flutter/flutter#47758

Closed

[Web] ListView freezes only on iOS. flutter/flutter#47825

Closed

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

[Web] Fix pointer binding (flutter#14378)

26f54ec

Refactors pointer_binding, fixes a few issues related to the pointer data converter in Web, and adds a number of tests for all three adapters.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Web] Fix pointer binding #14378

[Web] Fix pointer binding #14378

dkwingsmt commented Dec 12, 2019 •

edited

Loading

mdebbar Dec 12, 2019

dkwingsmt Dec 12, 2019 •

edited

Loading

mdebbar Dec 12, 2019

dkwingsmt Dec 12, 2019

mdebbar Dec 12, 2019

dkwingsmt Dec 12, 2019

mdebbar Dec 12, 2019

dkwingsmt Dec 12, 2019

mdebbar Dec 12, 2019

mdebbar Dec 12, 2019

dkwingsmt Dec 12, 2019

dkwingsmt Dec 12, 2019

mdebbar Dec 12, 2019

dkwingsmt Dec 18, 2019 •

edited

Loading

mdebbar Dec 18, 2019

dkwingsmt Dec 18, 2019

		_pressedButtons = 0;
		dataCallback(change: ui.PointerChange.up, buttons: _pressedButtons);

[Web] Fix pointer binding #14378

[Web] Fix pointer binding #14378

Conversation

dkwingsmt commented Dec 12, 2019 • edited Loading

Overview

Details

Rewrite adapters

Improve tests

Choose a reason for hiding this comment

dkwingsmt Dec 12, 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

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

dkwingsmt Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkwingsmt commented Dec 12, 2019 •

edited

Loading

dkwingsmt Dec 12, 2019 •

edited

Loading

dkwingsmt Dec 18, 2019 •

edited

Loading