Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CillianMyles
Copy link
Member

@CillianMyles CillianMyles commented Aug 19, 2024

As requested in a previous PR: #2139 (comment)

This PR demonstrates that the value of SuperTextFieldInspector.findScrollOffset() is incorrect sometimes after vertical scrolling on mobile.

I guess maybe this comment might contain a clue:
https://github.com/superlistapp/super_editor/blob/main/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart#L523-L537

@CillianMyles
Copy link
Member Author

cc/ @matthew-carroll

@CillianMyles CillianMyles changed the title Demonstrate issue where SuperTextFieldInspector.findScrollOffset() value is incorrect Demonstrate issue where SuperTextFieldInspector.findScrollOffset() value is incorrect Aug 19, 2024
@matthew-carroll
Copy link
Contributor

I see that you added a number of expectations. I assume they all fail? What values are each of them reporting?

@matthew-carroll
Copy link
Contributor

@CillianMyles ping on the earlier review questions

Copy link
Member Author

@CillianMyles CillianMyles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @matthew-carroll

Apologies for the delay in getting back to you on this one!

I added some information around what happens in the tests + values you were looking for.

Comment on lines +285 to +288
// Since the scrollable content is taller than the viewport, and since
// the bottom of the text field is aligned with the bottom of the
// viewport, the scroll offset should be greater than 0.
expect(SuperTextFieldInspector.findScrollOffset(), greaterThan(0.0));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the test:

testWidgetsOnMobile("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async {

And for both Android and iOS, the test fails because the value for SuperTextFieldInspector.findScrollOffset() is 0

Comment on lines +337 to +340
// Issue is not present on desktop, further leading me to think that the
// issue is somehow related to that comment about the scroll offsets
// being out of sync in the mobile scroll view.
expect(SuperTextFieldInspector.findScrollOffset(), greaterThan(0.0));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desktop equivalents pass (so the value is > 0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printed the actual value here as 20, which makes sense considering we have a scrollable of height 40 with content of height 60

@angelosilvestre
Copy link
Collaborator

@CillianMyles I ran into this issue while working on #2316, but I'm not sure if it's the same issue you are facing.

It seems that sometimes the TextScrollController and the ScrollController can be out of sync. In my case, the ScrollController was being restored from page storage. I changed SuperTextFieldInspector.findScrollOffset() to read from the ScrollController instead of the TextScrollController and it fixed the test method. We still have the problem of the two controllers being out of sync though...

@matthew-carroll matthew-carroll requested review from angelosilvestre and removed request for matthew-carroll September 22, 2024 19:42
@matthew-carroll
Copy link
Contributor

@angelosilvestre I assigned this to you so that you can dig further into the root cause after we decide what to do about restoring the scroll offset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants