[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539) #1541

angelosilvestre · 2023-10-21T17:22:41Z

[SuperEditor][SuperReader][Android] Fix scroll physics. Resolves #1539

On Android, we are noticing underscroll/overscroll with the editor being pinned at the position where the drag ended:

screen-20231020-111047.2.mp4

Android uses ClampingScrollPhysics, so it shouldn't even allow for underscroll/overscroll. It seems that the way we handle scrolling doesn't honor the scroll physics correctly.

I replaced the call to jumpTo with a call to pointerScroll and the issue seems to be solved:

android.webm

The difference between jumpTo and pointerScroll is that pointerScroll calls forcePixels clamping the jump offset between minScrollExtent and maxScrollExtent, whereas jumpTo calls it with the given jump offset. jumpTo also updates the userScrollDirection.

I'm not sure that using pointerScroll is the right way for us to handle scrolling. I also tried using ScrollPosition.drag at onPanStart and then Drag.update at onPanUpdate, but doing so caused the following exception:

════════ Exception caught by gesture ═══════════════════════════════════════════
The following assertion was thrown while handling a gesture:
'package:flutter/src/widgets/scroll_activity.dart': Failed assertion: line 370 pos 12: 'details.primaryDelta != null': is not true.
scroll_activity.dart:370

When the exception was thrown, this was the stack
#2      ScrollDragController.update scroll_activity.dart:370
#3      _AndroidDocumentTouchInteractorState._onPanUpdate document_gestures_touch_android.dart:785
#4      DragGestureRecognizer._checkUpdate.<anonymous closure> monodrag.dart:548

I'm open to other ideas to fix this issue.

angelosilvestre · 2023-10-21T17:23:11Z

@Jethro87 Could you please try this PR to see if it solves the issue for your app?

matthew-carroll · 2023-10-22T23:29:45Z

@angelosilvestre is there any reason you chose to switch the scrolling method rather than check the min/max pixels and do nothing if there's nothing more to scroll?

matthew-carroll · 2023-10-22T23:30:29Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

@@ -500,6 +500,76 @@ void main() {
      expect(SuperEditorInspector.findDocumentSelection(), isNull);
    });

+    testWidgetsOnAndroid("doesn't underscroll when dragging down", (tester) async {


Is "underscroll" the correct word? I've only heard of it as "overscroll" regardless of which direction you're scrolling.

Also, are you sure we don't already have tests that are supposed to be verifying this? I'm surprised if we don't, and if we do, something is wrong with those tests.

I saw "underscroll" in a comment in the ClampingScrollPhysics class while debugging, so I thought it was a thing. I changed to "overscroll".

I didn't find tests for this...

matthew-carroll · 2023-10-22T23:31:52Z

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

@@ -787,7 +787,7 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt
    }

    // The user is trying to scroll the document. Change the scroll offset.
-    scrollPosition.jumpTo(scrollPosition.pixels - details.delta.dy);
+    scrollPosition.pointerScroll(-details.delta.dy);


Did you happen to check how we're handling this on iOS?

Updated in #1541 (comment)

Jethro87 · 2023-10-23T14:05:44Z

@angelosilvestre I am not having this issue in my Android app after this update.

FYI on iOS, there is a similar over-scroll issue, though it does not "pin" the screen as it did on Android. Just in case you want to include iOS changes in this branch as well:

RPReplay_Final1698069813.mov

angelosilvestre · 2023-10-24T03:31:51Z

@matthew-carroll After Jeff's last comment I took a deeper look into the iOS scrolling. Here's how it looks right now:

Gravacao.de.Tela.2023-10-23.as.21.58.35.mov

On iOS, we are also using jumpTo. We can see in the video that we seem to be "fighting" the scroll physics. I'm convinced we shouldn't be using jumpTo, as each call to jumpTo is like an entire drag gesture: tapping down/dragging/releasing the tap. Also, dragging a bit and then holding down for a moment causes the editor to scroll back to the start. When holding the pointer down, the scroll position shouldn't jump back to the start.

I tried using pointerScroll on iOS, but then we can't overscroll and trigger the bouncing effect.

I changed this PR to use ScrollPosition.drag, here's how it's looking:

Gravacao.de.Tela.2023-10-23.as.22.09.24.mov

Using this method on Android, we can see the stretch animation on scroll:

android_dragging.webm

I mentioned in the PR description that using this API caused an exception on Android. On iOS, it worked without issues, so I looked at what's different between Android on iOS. The difference is that on Android we are using PanGestureRecognizer and on iOS VerticalDragGestureRecognizer. Switching the Android implementation to user VerticalDragGestureRecognizer solved the issue.

matthew-carroll · 2023-10-24T20:01:35Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

@@ -809,15 +812,12 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
    //       placement during onTapDown, and then pick that up here. I think the little
    //       bit of slop might be the problem.
    final selection = widget.selection.value;
-    if (selection == null) {


By removing this null check, you've allowed all following behaviors to execute with a null selection. Is that really what you intended?

Those behaviors won't be executed. _isOverBaseHandle and _isOverExtentHandle return false if there isn't a selection.

Also, I added a guard clause after all of the if statements to return early if there isn't a selection.

The change seems likely to create confusion when reading the code. We have if-statements that check if the user is pressing a handle, but moving this selection check creates the possibility that there is no handle, which probably isn't expected when you're reading code that checks for it.

Is there are more expectable order of code that won't lead to surprises?

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

matthew-carroll · 2023-10-24T20:03:29Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

@@ -911,8 +919,8 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
  void _onPanUpdate(DragUpdateDetails details) {
    // If the user isn't dragging a handle, then the user is trying to
    // scroll the document. Scroll it, accordingly.
-    if (_dragMode == null) {
-      scrollPosition.jumpTo(scrollPosition.pixels - details.delta.dy);
+    if (_dragMode == null && _drag != null) {


Should the drag mode have a value for scrolling or something? It seems strange that we're checking for a null drag mode and then checking for a non-null drag. From a reader's standpoint, that looks we're checking for opposites at the same time.

It makes sense. I included a DragMode for scrolling.

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

…rlistapp#1539)

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

matthew-carroll · 2023-10-25T02:22:54Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

@@ -809,15 +812,12 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
    //       placement during onTapDown, and then pick that up here. I think the little
    //       bit of slop might be the problem.
    final selection = widget.selection.value;
-    if (selection == null) {


The change seems likely to create confusion when reading the code. We have if-statements that check if the user is pressing a handle, but moving this selection check creates the possibility that there is no handle, which probably isn't expected when you're reading code that checks for it.

Is there are more expectable order of code that won't lead to surprises?

matthew-carroll · 2023-10-25T02:23:56Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

-    // scroll the document. Scroll it, accordingly.
-    if (_dragMode == null) {
-      scrollPosition.jumpTo(scrollPosition.pixels - details.delta.dy);
+    if (_dragMode == DragMode.scroll && _dragToScroll != null) {


Should it ever be possible that our drag mode is scroll and _dragToScroll is null?

It shouldn't be possible. Updated.

matthew-carroll · 2023-10-25T02:28:34Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

+      return;
+    }
+
+    if (_dragMode != null) {


Between this if-statement and the previous one, we're creating an easy opportunity for future developers to create bugs here. Though it's not immediately obvious, this 2nd if-statement isn't merely looking for a non-null _dragMode, it's looking for a _dragMode that isn't scroll. The average reader won't make that connection.

Please rework these two if-statements so that it's clear that there's two related conditions, not two independent conditions. In general, if _dragToScroll is always non-null when _dragMode is scroll, then we shouldn't check for null _dragToScroll, we should just look for a _dragMode of scroll.

I don't think this latest version has resolved the concern. We still have two independent if-statements, and the 2nd one depends on the 1st one. This isn't obvious.

Please switch this to a switch statement and have it do nothing on null

matthew-carroll · 2023-10-29T23:46:07Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

+      return;
+    }
+
+    if (_dragMode != null) {


I don't think this latest version has resolved the concern. We still have two independent if-statements, and the 2nd one depends on the 1st one. This isn't obvious.

Please switch this to a switch statement and have it do nothing on null

matthew-carroll · 2023-10-29T23:49:10Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+      // Ensure the scrollview didn't start scrolled.
+      expect(scrollController.offset, 0);
+
+      final editorRect = tester.getRect(find.byType(SuperEditor));


I don't think we need to get the editor rectangle do we? Don't we just need to attempt to scroll by some significant amount?

matthew-carroll · 2023-10-29T23:50:11Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+
+      final editorRect = tester.getRect(find.byType(SuperEditor));
+
+      const dragFrameCount = 10;


I assume this multi-frame drag gesture is repeated in these tests. I think I've done similar things elsewhere. Let's define an extension on WidgetTester that does this for an arbitrary dragPerFrame Offset.

I think that instead of passing a dragPerFrame, it makes more sense to pass the starting location and the total amount of dragging, letting the extension compute the dragPerFrame. What do you think?

matthew-carroll · 2023-10-29T23:50:25Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+        await tester.pump();
+
+        // Ensure we don't scroll.
+        expect(scrollController.offset, 0);


Can't we check this one time after the drag is over?

super_editor/test/super_editor/supereditor_scrolling_test.dart

matthew-carroll · 2023-10-29T23:51:19Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+      await tester
+          .createDocument()
+          .withSingleParagraph()
+          .withInputSource(TextInputSource.ime)


Do we care that this is IME? Why not use the default input source?

We don't. Removed.

matthew-carroll · 2023-10-29T23:51:29Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+          .createDocument()
+          .withSingleParagraph()
+          .withInputSource(TextInputSource.ime)
+          .withEditorSize(const Size(300, 300))


Do we care about the editor size in this test?

Nope, removed.

matthew-carroll · 2023-11-02T21:07:26Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+      // Ensure the editor didn't start scrolled.
+      expect(scrollController.offset, 0);
+
+      // Drag an amount of pixels chosen experimentally from the top of the editor.


This says "pixels chosen experimentally" - what was the experiment? Isn't this amount arbitrary?

Yeah, it's arbitrary.

matthew-carroll · 2023-11-02T21:08:55Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+
+      // Drag an amount of pixels chosen experimentally from the top of the editor.
+      final dragGesture = await tester.dragInMultipleFrames(
+        startLocation: tester.getTopLeft(find.byType(SuperEditor)),


Let's not start a drag from the top left corner of the editor. That's a very unnatural place for a drag to ever begin with a real user, and it creates opportunities for things like safe areas, drawer drag areas, or other edge-based effects to interfere. A more natural place to test the drag would be the horizontal center, and at least some distance down from the top of the editor.

super_editor/test/super_editor/supereditor_scrolling_test.dart

matthew-carroll · 2023-11-02T21:11:05Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+        await tester
+            .createDocument()
+            .withSingleParagraph()
+            .withCustomWidgetTreeBuilder(


Do we need any of this custom tree complexity for this test?

We need to setup an ancestor CustomScrollView for this test.

Ok. Why do we need to constrain the height to 200px?

Also, if we need the same tree in multiple tests, is there any reason not to extract it to either a private widget or a private method? Or, perhaps what would make even more sense is to either break it into a widget shared within the test suite, or even add it as a configuration option to the builder, because it looks like there are tests across multiple files that need this same setup.

Added methods to the test builder to configure the widget tree with a CustomScrollView.

super_editor/test/test_tools.dart

matthew-carroll · 2023-11-13T20:03:14Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

-            )
+            .withEditorSize(const Size(200, 200))
+            .insideCustomScrollView()
+            .withCustomScrollViewScrollController(scrollController)


Should this be an optional argument to insideCustomScrollView() instead? Would it ever make sense to call withCustomScrollViewController() but without having previously called insideCustomScrollView()?

Yeah, I also thought about that. I guess it makes sense to add the scrollController as an optional argument.

super_editor/test/super_editor/supereditor_scrolling_test.dart

super_editor/test/super_editor/supereditor_test_tools.dart

matthew-carroll · 2023-11-13T20:06:13Z

super_editor/test/super_editor/supereditor_test_tools.dart

@@ -474,6 +507,9 @@ class SuperEditorTestConfiguration {
  final plugins = <SuperEditorPlugin>{};

  WidgetTreeBuilder? widgetTreeBuilder;
+
+  bool insideCustomScrollView = false;


These properties look out of place given the order and separation of everything else.

Moved up right below the scrollController.

super_editor/test/super_reader/super_reader_scrolling_test.dart

matthew-carroll

LGTM

…rlistapp#1539) (superlistapp#1541)

… (#1541)

angelosilvestre requested a review from matthew-carroll October 21, 2023 17:23

matthew-carroll reviewed Oct 22, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

angelosilvestre force-pushed the 1539_android_scroll branch from 53b0967 to 8b11fee Compare October 24, 2023 03:21

angelosilvestre requested a review from matthew-carroll October 24, 2023 03:35

matthew-carroll reviewed Oct 24, 2023

View reviewed changes

angelosilvestre added 4 commits October 24, 2023 22:20

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves supe…

f6a8d9b

…rlistapp#1539)

User ScrollPosition.drag to perform scrolling

f4c2f36

Fix merge conflicts

1648990

PR updates

97cd440

angelosilvestre force-pushed the 1539_android_scroll branch from 6b0094f to 97cd440 Compare October 25, 2023 01:23

angelosilvestre requested a review from matthew-carroll October 25, 2023 02:22

matthew-carroll reviewed Oct 25, 2023

View reviewed changes

PR updates

5dcc192

angelosilvestre requested a review from matthew-carroll October 25, 2023 23:37

matthew-carroll reviewed Oct 29, 2023

View reviewed changes

angelosilvestre added 2 commits October 30, 2023 19:59

Refactor iOS pan end

e4ed3c5

Create extension for drag gestures

1e8f286

angelosilvestre requested a review from matthew-carroll October 31, 2023 00:18

matthew-carroll reviewed Nov 2, 2023

View reviewed changes

PR Updates

3a86384

angelosilvestre requested a review from matthew-carroll November 2, 2023 22:22

angelosilvestre added 3 commits November 6, 2023 21:42

Update comments

e0cf920

Add test configuration to use a CustomScrollView

7097060

Add docs

9ab59dc

matthew-carroll reviewed Nov 13, 2023

View reviewed changes

angelosilvestre added 2 commits November 13, 2023 19:25

PR updates

8c19084

Use withScrollController to define ancestor scrollable controller

5378373

angelosilvestre requested a review from matthew-carroll November 15, 2023 21:00

matthew-carroll approved these changes Nov 21, 2023

View reviewed changes

matthew-carroll merged commit 8378305 into superlistapp:main Nov 21, 2023
10 of 11 checks passed

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Nov 22, 2023

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves supe…

3d0f3d3

…rlistapp#1539) (superlistapp#1541)

angelosilvestre mentioned this pull request Nov 22, 2023

Cherry-pick "[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539) (#1541)" to stable #1612

Merged

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Nov 26, 2023

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves supe…

271f310

…rlistapp#1539) (superlistapp#1541)

matthew-carroll pushed a commit that referenced this pull request Nov 26, 2023

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539)…

a28d2a2

… (#1541)


		final editorRect = tester.getRect(find.byType(SuperEditor));

		const dragFrameCount = 10;

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539) #1541

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539) #1541

Conversation

angelosilvestre commented Oct 21, 2023

angelosilvestre commented Oct 21, 2023

matthew-carroll commented Oct 22, 2023

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

Jethro87 commented Oct 23, 2023

This comment was marked as outdated.

angelosilvestre commented Oct 24, 2023

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

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

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

matthew-carroll left a comment

Choose a reason for hiding this comment