[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140 and #1795) #2139

CillianMyles · 2024-07-03T10:21:48Z

No description provided.

CillianMyles · 2024-07-03T14:28:31Z

super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart

@@ -423,7 +423,7 @@ class _TextScrollViewState extends State<TextScrollView>
      child: SingleChildScrollView(
        key: _textFieldViewportKey,
        controller: _scrollController,
-        physics: const NeverScrollableScrollPhysics(),
+        physics: isMultiline ? null : const NeverScrollableScrollPhysics(),


Passing null for scroll physics let's the platform dictate the appropriate scroll physics.
Also as far as I could tell this change only applies to mobile SuperTextFields.

Also as far as I could text this change only applies to mobile SuperTextFields.

Can you mention what that's based on? Without opening the source code I can't tell. Is there a mobile platform check somewhere in this code, or a related place?

This code is within this widget called TextScrollView, which is only used in SuperAndroidTextField and SuperIOSTextField:

I said as far "as I could tell" because I'm not intimately familiar with the codebase, and did not want to say something incorrect or misleading.

Can you add a code comment that explains why this ternary is here, and can you also add a breadcrumb in that comment for where a developer should look to find scrolling behavior in the case of a single-line text field (for which we disable the physics)?

@CillianMyles I wanted to make sure you saw the above comment

Comment added + updated! ✅

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart

matthew-carroll · 2024-07-17T07:05:49Z

I checked usages of TextScrollView and you're correct that it's only currently used for Android and iOS SuperTextFields.

Did you check what happens with auto-scrolling when dragging selection handles, dragging the caret, etc?

Wrapped around the TextScrollView is a AndroidTextFieldTouchInteractor and a IOSTextFieldTouchInteractor, respectively. Both of those interactors are given access to the scroll controller for the TextScrollView.

I assume there was a reason that I disabled the standard scroll physics. I'd like to find what that was and then ensure we didn't break those requirements with this change. If this does break those requirements, then we might need to find another way to accomplish this.

matthew-carroll · 2024-07-17T07:06:14Z

Also, let's move this out of draft mode, and can you also check the failing tests in CI?

CillianMyles · 2024-07-17T16:04:14Z

@matthew-carroll - the tests I added for mobile and desktop are now passing. These tests check to see that multi-line SuperText fields can scroll up and down when the text overflows.

I'm not sure if I'll be able to help figure out the reason scroll physics were originally disabled.

And in terms of checking the existing functionality with selection handle, magnifiers, scrolling etc.: all existing tests pass, which I hope cover the rules in more detail than I can manually test. In any case I did some manual checks - see:

CleanShot.2024-07-17.at.16.54.22.mp4

Let me know if there is more you would like me to manually test, or if I should write any more tests.

I also now have some golden failures. I tried to update the goldens locally but it ended up with way too many unrelated files changed. Is there something I'm missing with regard to updating the goldens? I am running latest Flutter main:

Flutter 3.24.0-1.0.pre.153 • channel main • https://github.com/flutter/flutter.git
Framework • revision 49d6d520cb (45 minutes ago) • 2024-07-17 11:18:10 -0400
Engine • revision 7e25796340
Tools • Dart 3.6.0 (build 3.6.0-48.0.dev) • DevTools 2.37.1

super_editor/test_goldens/super_textfield/super_textfield_toolbar_test.dart

CillianMyles · 2024-07-24T09:47:46Z

@matthew-carroll all tests (including new tests & goldens) are now passing.

Not sure what to do about the Netlify/deploymant job failures.

It seems to be in the middle of setting up Flutter, then switches to stable - I guess this is a problem, as it should be on main/master?

I don't think anything I did should affect these jobs?

CillianMyles · 2024-07-24T11:05:08Z

@matthew-carroll I think we should fix the goldens: merge either (#2183) or (#2184), and then I will rebase this PR.

I see that option 1 works and 2 does not, and also the Netlify jobs didn't fail in option 1 so probably will succeed here after I rebase this PR.

Please let me know how we should proceed.

CillianMyles · 2024-07-27T19:49:54Z

@matthew-carroll this one has been rebased and tests are passing, and ready for another look!

matthew-carroll · 2024-07-27T20:16:43Z

@CillianMyles do you know if those demo site failures are new to this PR vs broken in general? I don't think I saw those failing in other PRs...

CillianMyles · 2024-07-27T20:50:01Z

@matthew-carroll over the past few days/weeks working on a few PRs, I have noticed these fail sometimes. I can't remember in which PRs and it's a bit hard to track down older runs due to rebasing.

To be honest thought I can't think why the code changes in this PR could have any affect on these demo site failures.

Not sure if it's any use, but now come to think of it, something I do remember when I created the option 1 and option 2 PRs at virtually the same time, the PR checks were actually running at the same time on both, and I noted that the demo site jobs failed on one PR and not the other. Not sure if it could be anything to do with concurrent jobs.

Aside from that, I don't have any ideas unfortunately.

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart

matthew-carroll · 2024-07-27T20:51:36Z

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart

+      }
+
+      // Ensure the text field has not yet scrolled.
+      expect(getTextBottom(), lessThan(getViewportBottom()));


Is this how we verify scroll offsets in other tests? I don't remember, but I would assume that we'd access a scroll controller value. Perhaps we even have some test tools for this?

In different tests in this file, we use both this method, and the SuperTextFieldInspector.findScrollOffset() method:
https://github.com/superlistapp/super_editor/blob/main/super_editor/test/super_textfield/super_textfield_inspector.dart#L157-L191

I tried the SuperTextFieldInspector.findScrollOffset() method, but found it did not seem to have the correct value after scrolling back up (0), even though I visually validated the scroll by taking some golden images.

During some debugging, I found the values of the scroll controller, and the values of TextScrollController.scrollOffset to be off, even though you would expect them to be the same.

When I saw the following comment, it made me think that it would be easier to compare the top and bottom values, since this method is used already in this file, and as I did not really know how to debug this any further.

Can you file an issue about the inspector method returning the wrong results, and include repro steps?

Yes I can do that. It would be easier after landing this PR, as I could modify the tests I've added to use this SuperTextFieldInspector.findScrollOffset() and show that it produces incorrect results. Would that work?

@matthew-carroll as promised: #2256

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart

matthew-carroll · 2024-07-27T20:53:08Z

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart

+      await tester.pumpAndSettle();
+
+      // Ensure the text field has scrolled back to the top.
+      expect(getTextBottom(), lessThan(getViewportBottom()));


Is lessThan a sufficient check? One if we only scrolled up 1px....wouldn't this still pass?

You are correct, when changing to scroll only 1 pixel, the test still passes (incorrectly).

I switched approaches to always validate that either the tops or bottoms are aligned (now the test fails when scrolling by only 1 pixel).

CillianMyles · 2024-08-12T15:38:31Z

@matthew-carroll I rebased and addressed all your points. Looking forward to your feedback!

matthew-carroll · 2024-08-14T19:34:28Z

super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart

@@ -423,6 +423,8 @@ class _TextScrollViewState extends State<TextScrollView>
      child: SingleChildScrollView(
        key: _textFieldViewportKey,
        controller: _scrollController,
+        // Passing null for scroll physics for multiline text fields will
+        // apply an appropriate physics for the underlying host platform.


The operative details we need to capture in this comment is why there's a difference, i.e., why we sometimes disable physics vs sometimes we want the underlying standard physics. Please see the request in my original comment.

Updated ✅

chore: fix desktop test

… equals() -> moreOrLessEquals()

…l amount changes

matthew-carroll

LGTM

matthew-carroll · 2024-08-16T21:02:23Z

@angelosilvestre can you cherry pick this?

…hen its text overflows (Resolves #2140)(Resolves #1795) (#2139)

CillianMyles mentioned this pull request Jul 3, 2024

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows #2140

Closed

CillianMyles changed the title ~~feat: allow vertical scrolling for multiline SuperTextFields~~ [FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140) Jul 3, 2024

CillianMyles commented Jul 3, 2024

View reviewed changes

matthew-carroll mentioned this pull request Jul 9, 2024

[SuperTextField][mobile] Multi-line textfield doesn't allow drag to scroll vertically #1795

Closed

CillianMyles marked this pull request as ready for review July 17, 2024 12:42

CillianMyles force-pushed the feature/allow-scrolling-within-super-text-field branch from f987f50 to 11f3649 Compare July 17, 2024 15:47

CillianMyles requested a review from matthew-carroll July 17, 2024 16:04

CillianMyles force-pushed the feature/allow-scrolling-within-super-text-field branch from 11f3649 to ea96050 Compare July 24, 2024 08:15

CillianMyles commented Jul 24, 2024

View reviewed changes

super_editor/test_goldens/super_textfield/super_textfield_toolbar_test.dart Outdated Show resolved Hide resolved

CillianMyles requested review from matthew-carroll and removed request for matthew-carroll July 24, 2024 09:52

This was referenced Jul 24, 2024

Fix Goldens - Option 1 #2183

Closed

Fix Goldens - Option 2 #2184

Merged

CillianMyles changed the title ~~[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140)~~ [FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140 and #1795) Jul 24, 2024

CillianMyles force-pushed the feature/allow-scrolling-within-super-text-field branch from 2a19189 to a2cf922 Compare July 27, 2024 19:34

matthew-carroll reviewed Jul 27, 2024

View reviewed changes

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart Outdated Show resolved Hide resolved

matthew-carroll reviewed Jul 27, 2024

View reviewed changes

super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart Outdated Show resolved Hide resolved

matthew-carroll reviewed Jul 27, 2024

View reviewed changes

CillianMyles force-pushed the feature/allow-scrolling-within-super-text-field branch from ab37160 to 8de952e Compare August 12, 2024 13:57

CillianMyles requested a review from matthew-carroll August 12, 2024 15:38

matthew-carroll reviewed Aug 14, 2024

View reviewed changes

CillianMyles added 7 commits August 14, 2024 21:34

feat: allow vertical scrolling for multiline SuperTextFields

9b67271

test: write test cases for vertical scrolling on mobile + desktop

afc5f3c

chore: fix multiline scrolling tests for desktop + mobile

d8c836a

chore: fix desktop test

chore: remove inline functions to calculate top and bottom positions;…

aaa5fd4

… equals() -> moreOrLessEquals()

chore: lessThan() -> lessThanOrEqualTo()

8541fe1

fix: make test more robust, and fail (correctly) more easily if scrol…

6ab46d6

…l amount changes

chore: add a comment to explain scroll physics

bd67a68

CillianMyles force-pushed the feature/allow-scrolling-within-super-text-field branch from 6d517dd to bd67a68 Compare August 14, 2024 20:35

chore: update comment to explain scroll physics

7be800c

matthew-carroll approved these changes Aug 16, 2024

View reviewed changes

matthew-carroll merged commit 1ffd28d into superlistapp:main Aug 16, 2024
12 checks passed

angelosilvestre pushed a commit that referenced this pull request Aug 17, 2024

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField w…

7c4d715

…hen its text overflows (Resolves #2140)(Resolves #1795) (#2139)

angelosilvestre mentioned this pull request Aug 17, 2024

Cherry-pick "[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140)(Resolves #1795) (#2139)" to stable #2250

Merged

matthew-carroll pushed a commit that referenced this pull request Aug 17, 2024

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField w…

9c179f1

…hen its text overflows (Resolves #2140)(Resolves #1795) (#2139)

CillianMyles deleted the feature/allow-scrolling-within-super-text-field branch August 19, 2024 07:46

CillianMyles mentioned this pull request Aug 19, 2024

Demonstrate issue where SuperTextFieldInspector.findScrollOffset() value is incorrect #2256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140 and #1795) #2139

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140 and #1795) #2139

CillianMyles commented Jul 3, 2024

CillianMyles Jul 3, 2024 •

edited

Loading

matthew-carroll Jul 8, 2024

CillianMyles Jul 9, 2024

matthew-carroll Jul 27, 2024

matthew-carroll Aug 14, 2024

CillianMyles Aug 14, 2024

matthew-carroll commented Jul 17, 2024

matthew-carroll commented Jul 17, 2024

CillianMyles commented Jul 17, 2024 •

edited

Loading

CillianMyles commented Jul 24, 2024 •

edited

Loading

CillianMyles commented Jul 24, 2024 •

edited

Loading

CillianMyles commented Jul 27, 2024

matthew-carroll commented Jul 27, 2024

CillianMyles commented Jul 27, 2024

matthew-carroll Jul 27, 2024

CillianMyles Aug 12, 2024

matthew-carroll Aug 14, 2024

CillianMyles Aug 14, 2024

CillianMyles Aug 19, 2024

matthew-carroll Jul 27, 2024

CillianMyles Aug 12, 2024 •

edited

Loading

CillianMyles commented Aug 12, 2024 •

edited

Loading

matthew-carroll Aug 14, 2024

CillianMyles Aug 14, 2024

matthew-carroll left a comment

matthew-carroll commented Aug 16, 2024

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140 and #1795) #2139

[FEATURE] - Allow vertical scrolling of a multi-line SuperTextField when its text overflows (Resolves #2140 and #1795) #2139

Conversation

CillianMyles commented Jul 3, 2024

CillianMyles Jul 3, 2024 • 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

matthew-carroll commented Jul 17, 2024

matthew-carroll commented Jul 17, 2024

CillianMyles commented Jul 17, 2024 • edited Loading

CillianMyles commented Jul 24, 2024 • edited Loading

CillianMyles commented Jul 24, 2024 • edited Loading

CillianMyles commented Jul 27, 2024

matthew-carroll commented Jul 27, 2024

CillianMyles commented Jul 27, 2024

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

CillianMyles Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

CillianMyles commented Aug 12, 2024 • edited Loading

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

matthew-carroll commented Aug 16, 2024

CillianMyles Jul 3, 2024 •

edited

Loading

CillianMyles commented Jul 17, 2024 •

edited

Loading

CillianMyles commented Jul 24, 2024 •

edited

Loading

CillianMyles commented Jul 24, 2024 •

edited

Loading

CillianMyles Aug 12, 2024 •

edited

Loading

CillianMyles commented Aug 12, 2024 •

edited

Loading