[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409) #1421

rutvik110 · 2023-09-09T11:22:01Z

[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409)

Currently in the SuperEditor or SuperReader, the document is scrollable even if the content fits within the available space as seen in the below demo.

This demo wraps the SuperEditor within an IntrinsicHeight widget to demonstrate the issue,

Screen.Recording.2023-09-07.at.1.02.09.PM.mov

This PR tries to address this issue by locking the scrolling for the document in such cases. We lock the scrolling when the content of the document fits within the available viewport space and there isn't any unconsumed content within the Scrollable.

Scroll lock is introduced within both DocumentScroller and AutoScrollController. When the scroll lock is enabled, any calls/actions related to scrolling are ignored to prevent the document from scrolling.

After this latest updates, the scrolling is locked whenever content fits within available space as seen in the following demo,

f_scroll_lock.mp4

rutvik110 · 2023-09-09T11:34:24Z

@matthew-carroll Could you please take a look at the approach I took to fix this issue? It's still WIP but wanted your opinion on the approach and see if whether this is the direction that we want to move in with this issue.

matthew-carroll · 2023-09-11T20:23:35Z

@rutvik110 before evaluating your choice of solution, can you describe what you investigated? For example, why is the scrollable letting us scroll when there's no content to scroll?

rutvik110 · 2023-09-12T03:23:04Z

@rutvik110 before evaluating your choice of solution, can you describe what you investigated? For example, why is the scrollable letting us scroll when there's no content to scroll?

There seems to be two reasons here that are causing the overscroll.

The first reason is related to our handling of the scrolling within Scrollable. In case of auto-scrolling in response to mouse events, currently we are always trying to scroll irrespective of whether there's any content left to scroll.
This calls to scroll is leading to the overscroll/scroll-bounce that we are noticing in demo you shared.

Second reason is bit more ambiguous and it may be totally different issue that I may've confused with this one but here it's. It seems to be particularly related to usage of IntrinisicHeight. I noticed that when we have large chunks of paragraphs within SuperEditor, the final height calculated by the IntrinisicHeight to layout the SuperEditor isn't correct and is smaller by some magnitude. So the SuperEditor is laid out with the height smaller than it's contents actual height which results in introducing some content within SuperEditor beyond its scrollable viewport.

Screen.Recording.2023-09-12.at.8.45.55.AM.mov

…to the actual issue

…o scroll

rutvik110 · 2023-09-12T04:01:55Z

super_editor/lib/src/default_editor/document_scrollable.dart

@@ -387,6 +387,12 @@ class AutoScrollController with ChangeNotifier {
      return;
    }

+    // There's no content to scroll. So, we can't go ballistic as it would result


@matthew-carroll I've reverted the previous additions I made to this issue. I think those additions were targeting a different issue than the one we're trying to solve here.

Wrt the first reason I mentioned above for overscrolling, this update resolves that.
Currently, we do a final scroll call with goBallistic to animate to a new scroll position at the end of the mouse events(specifically pan events) that results in scrolling the content.
I added a check to prevent making this call if there's no content to scroll to or maxScrollExtent is 0.

This should take care of the overscroll that was happening when the content fits within the available space.

super_editor/lib/src/default_editor/document_scrollable.dart

matthew-carroll · 2023-09-12T19:11:29Z

super_editor/example/lib/demos/demo_switch_document_content.dart

@@ -50,7 +50,8 @@ class _SwitchDocumentDemoState extends State<SwitchDocumentDemo> {
      child: Column(
        children: [
          _buildDocSelector(),
-          Expanded(
+          IntrinsicHeight(


Please revert this.

rutvik110 · 2023-09-16T16:08:12Z

Also, what about when the user is using the scroll wheel? I was able to overscroll using the scroll wheel which I believe is unrelated to going ballistic.

ah yah, had to add the same max extent check to jumpBy which resolved this. @matthew-carroll Could you please re-check this on your end?

Also, what are your thoughts on the previous comment I made regarding extent check in the reverse direction?

matthew-carroll · 2023-09-20T22:38:25Z

I think the current changes are probably good as-is. Can you confirm that these changes work for SuperReader as well as SuperEditor? Also, can you write a test for each of these scenarios in the appropriate test file?

And please don't forget to revert the code that I mentioned in an earlier comment.

rutvik110 · 2023-09-21T02:36:11Z

Great! Will check on SuperReader once which I believe should work fine as well and get onto writing tests for this.

super_editor/test/super_editor/supereditor_scrolling_test.dart

matthew-carroll · 2023-09-28T20:27:24Z

super_editor/test/super_editor/supereditor_scrolling_test.dart

+
+      group("doesn't scroll editor when content fits within the available space", () {
+        testWidgetsOnDesktop(
+          "attempts to scroll editor vertically in both directions using trackpad",


These test names aren't great. Look at what the final test name is:

"doesn't scroll editor when content fits within the available space attempts to scroll editor vertically in both directions using trackpad"

I think you can describe the group and test in a way that's much more concise, and makes sense when read as the final test description.

super_editor/test/super_editor/supereditor_scrolling_test.dart

super_editor/test/super_reader/super_reader_scrolling_test.dart

matthew-carroll

LGTM

…available space (Resolves #1409) (#1421)

…available space (Resolves superlistapp#1409) (superlistapp#1421)

rutvik110 added 3 commits September 9, 2023 16:02

lock scrolling when content fits within available space

e492bb8

test tools update

82190bc

refactor

9d53c2e

rutvik110 requested a review from matthew-carroll September 9, 2023 11:25

rutvik110 added 2 commits September 12, 2023 09:05

reverted current scroll lock mechansim as the approach was wrong wrt …

76a3586

…to the actual issue

returning early instead of going ballistic when there's not content t…

d12e5f4

…o scroll

rutvik110 commented Sep 12, 2023

View reviewed changes

update

f4aae47

matthew-carroll reviewed Sep 12, 2023

View reviewed changes

updated jumpBy to check for max extent == 0, refactor

dd8725f

rutvik110 added 2 commits September 23, 2023 15:13

tests added

3245327

reverted example changes

cf930f8

rutvik110 force-pushed the 1409_content_fit_scroll_lock branch from 7e9353d to cf930f8 Compare September 23, 2023 09:53

rutvik110 requested a review from matthew-carroll September 23, 2023 09:55

matthew-carroll reviewed Sep 25, 2023

View reviewed changes

super_editor/test/super_editor/supereditor_scrolling_test.dart Show resolved Hide resolved

super_editor/test/super_editor/supereditor_scrolling_test.dart Outdated Show resolved Hide resolved

rutvik110 added 2 commits September 28, 2023 00:32

tests for scroll wheel added

dc5ff49

updates

37231fa

rutvik110 requested a review from matthew-carroll September 27, 2023 19:09

matthew-carroll reviewed Sep 28, 2023

View reviewed changes

test description updates

981d39a

rutvik110 requested a review from matthew-carroll September 29, 2023 18:17

matthew-carroll reviewed Sep 29, 2023

View reviewed changes

test updates

da0c7b1

matthew-carroll approved these changes Sep 30, 2023

View reviewed changes

matthew-carroll merged commit a49960f into main Sep 30, 2023
10 of 11 checks passed

matthew-carroll deleted the 1409_content_fit_scroll_lock branch September 30, 2023 18:09

rutvik110 added a commit that referenced this pull request Oct 2, 2023

[SuperEditor][SuperReader] - Lock scrolling when content fits within …

79201cb

…available space (Resolves #1409) (#1421)

matthew-carroll pushed a commit that referenced this pull request Oct 2, 2023

[SuperEditor][SuperReader] - Lock scrolling when content fits within …

be059c2

…available space (Resolves #1409) (#1421)

raulmabe-labhouse pushed a commit to LabhouseMobile/super_editor that referenced this pull request Oct 10, 2023

[SuperEditor][SuperReader] - Lock scrolling when content fits within …

ff36084

…available space (Resolves superlistapp#1409) (superlistapp#1421)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409) #1421

[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409) #1421

rutvik110 commented Sep 9, 2023 •

edited

Loading

rutvik110 commented Sep 9, 2023 •

edited

Loading

matthew-carroll commented Sep 11, 2023

rutvik110 commented Sep 12, 2023

rutvik110 Sep 12, 2023 •

edited

Loading

matthew-carroll Sep 12, 2023

rutvik110 commented Sep 16, 2023

matthew-carroll commented Sep 20, 2023

rutvik110 commented Sep 21, 2023

matthew-carroll Sep 28, 2023

matthew-carroll left a comment

[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409) #1421

[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409) #1421

Conversation

rutvik110 commented Sep 9, 2023 • edited Loading

rutvik110 commented Sep 9, 2023 • edited Loading

matthew-carroll commented Sep 11, 2023

rutvik110 commented Sep 12, 2023

rutvik110 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

matthew-carroll Sep 12, 2023

Choose a reason for hiding this comment

rutvik110 commented Sep 16, 2023

matthew-carroll commented Sep 20, 2023

rutvik110 commented Sep 21, 2023

matthew-carroll Sep 28, 2023

Choose a reason for hiding this comment

matthew-carroll left a comment

Choose a reason for hiding this comment

rutvik110 commented Sep 9, 2023 •

edited

Loading

rutvik110 commented Sep 9, 2023 •

edited

Loading

rutvik110 Sep 12, 2023 •

edited

Loading