[SuperTextField][Mobile] Fix scrolling when changing the selection to a visible position (Resolves #1096) #1101

angelosilvestre · 2023-04-25T01:16:48Z

[SuperTextField][Mobile] Fix scrolling when changing ths selection to a visible position. Resolves #1096

When changing the selection in a multi-line textfield, if the selected position offset is greater than the text field height the text field is being scrolled, even if this offset is already visible on screen:

Screen.Recording.2023-04-24.at.16.16.40.mov

The issue is that we are only comparing the offset with the viewport height, ignoring the fact that we might already have scrolled to a position that makes the selection visible.

This PR changes the comparison to consider the current scroll offset.

I tried using the robot to change the selection, but it doesn't seem to work when the textfield scrolls.

I copied _findInnerPlatformTextField from SuperTextFieldRobot. Is there any shared file where we could de-dup this?

…isible position (Resolves superlistapp#1096)

matthew-carroll · 2023-04-25T02:00:09Z

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

@@ -995,7 +995,7 @@ class TextScrollController with ChangeNotifier {
        // Scroll the content down.
        _scrollOffset = rect.top;
        _log.finer(' - updated _scrollOffset to $_scrollOffset');
-      } else if (rect.bottom > _delegate!.viewportHeight!) {
+      } else if ((rect.bottom - _scrollOffset) > _delegate!.viewportHeight!) {


We should rename this rect and the incoming extentCharacterRect to reflect the space that the rectangle sits in.

rectInContentSpace, extentCharacterRectInContentSpace

This way we hopefully won't overlook details like this in the future.

Also, can you please confirm in the _delegate that this rect definitely is in content space, and not viewport space? Related to that, if the rect is in content space, then how is rect.top < 0 working? If rect is in content space, then rect.top would never be less than zero. So something seems inconsistent here.

The rect is in content space, we get it from:

final extentCharacterRectInContentSpace = _delegate!.getCharacterRectAtPosition(TextPosition(offset: characterIndex)); _ensureRectIsVisible(extentCharacterRectInContentSpace);

getCharacterRectAtPosition just calls _textLayout.getCharacterBox:

@override Rect getCharacterRectAtPosition(TextPosition position) { return _textLayout.getCharacterBox(position)?.toRect() ?? Rect.fromLTRB(0, 0, 0, _textLayout.estimatedLineHeight); }

Related to that, if the rect is in content space, then how is rect.top < 0 working?

Indeed, this doesn't seem to be working.

getCharacterRectAtPosition just calls _textLayout.getCharacterBox

What if there's padding in the text field? Does everything still work?

matthew-carroll · 2023-04-25T02:02:09Z

I tried using the robot to change the selection, but it doesn't seem to work when the textfield scrolls.

When saying that something doesn't work, please describe exactly what isn't working. What did you try? What was expected? What was the actual result?

I can't review a decision to replicate behavior if I can't see exactly what issue you're sidestepping.

angelosilvestre · 2023-04-25T02:10:09Z

@matthew-carroll

placeCaretInSuperTextField calls a method that contains:

if (!textFieldBox.size.contains(adjustedOffset)) {  
  print("Couldn't tap at $adjustedOffset in text field with size ${textFieldBox.size}");
  return false;
}

This code doesn't take into account the cases where the content height is bigger than the textfield height.

matthew-carroll · 2023-04-25T02:25:03Z

@angelosilvestre that explanation still requires me to go read some source code to contextualize what you're saying. How are you trying to use the tool, what's the expected output, what's the actual output?

angelosilvestre · 2023-04-25T02:32:50Z

I'm trying to place the selection at the end of the text. This offset isn't visible, because the content height is bigger than the viewport height.

I call tester.placeCaretInSuperTextField(offset).

Expected result: the selection is placed at offset

Actual result: An exception is thrown with the message "The desired text offset wasn't tappable in SuperTextField: $offset"

The exception also happens if we already scrolled to the end.

matthew-carroll · 2023-04-25T03:13:28Z

Ok, that's a much more helpful description. Thanks.

I'll think about that.

matthew-carroll · 2023-04-25T03:44:29Z

I copied _findInnerPlatformTextField from SuperTextFieldRobot. Is there any shared file where we could de-dup this?

Do we have an Inspector for SuperTextField? If so, we should probably have the Inspector answer questions about current scroll offset, max scroll offset, etc, rather than implement inspection behavior in the robot or other tests.

super_editor/test/super_textfield/super_textfield_scroll_test.dart

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

matthew-carroll · 2023-04-26T01:29:09Z

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

-    final extentCharacterRect = _delegate!.getCharacterRectAtPosition(TextPosition(offset: characterIndex));
-    _ensureRectIsVisible(extentCharacterRect);
+
+    final padding = _delegate!.padding ?? const EdgeInsets.all(0.0);


There's a widget.padding, which you also exposed as padding, and now there's _delegate!.padding? Which padding is the real padding? Or am I looking at two different classes? I can't tell with the way that GitHub is showing the code.

Also, my initial reaction is that I don't think we want to expose arbitrary layout details like padding. What happens if there's also a border, or margin, or anything else that impacts position? It seems to me that the getCharacterRectAtPosition should just return the actual character rect. To me, this looks like pieces of the layout implementation are being pulled into the scrolling system.

Or am I looking at two different classes?

There are different classes

Updated getCharacterRectAtPosition to return the adjusted rect and removed the padding from the delegate.

matthew-carroll · 2023-04-26T02:08:35Z

super_editor/test/super_textfield/super_textfield_scroll_test.dart

+      expect(
+        SuperTextFieldInspector.findScrollOffset(),
+        10.0,
+      );


I don't see anything in this test that shows what it has to do with padding. If the test of padding is incidental, then please update the comments to focus on what we're proving with this behavior.

matthew-carroll · 2023-04-26T23:34:49Z

super_editor/lib/src/infrastructure/super_textfield/desktop/desktop_textfield.dart

@@ -1185,15 +1185,29 @@ class SuperTextFieldScrollviewState extends State<SuperTextFieldScrollview> with
    const gutterExtent = 0; // _dragGutterExtent
    final extentLineIndex = (extentOffset.dy / widget.estimatedLineHeight).round();

+    final firstCharY = _textLayout.getCharacterBox(const TextPosition(offset: 0))?.top ?? 0.0;
+    final lastCharY =
+        _textLayout.getCharacterBox(TextPosition(offset: widget.textController.text.text.length - 1))?.top ??


Shouldn't we use the bottom of the last character, not the top?

I think we should use the top, because the extentOffset refers to the top-left. I made an adjustment to use zero when _textLayout.getCharacterBox returns null.

I didn't understand that explanation. We're trying to determine the bottom most y-value that should be visible, right? I mean, imagine that only the top pixel of a line were visible - that means that nearly the entire line is below the bottom of the viewport, right?

Correct me if I'm wrong, but for the top line we essentially do "topCharacter.top - halfLineHeight", right?

I would expect for the bottom would do "bottomCharacter.bottom + halfLineHeight".

Am I getting something wrong?

But we only use lastCharY to determine if we are at the last line of the textfield, not to determine the rect that should be visible.

super_editor/lib/src/infrastructure/super_textfield/desktop/desktop_textfield.dart

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

matthew-carroll · 2023-04-26T23:47:06Z

super_editor/test_goldens/super_textfield/goldens/super_textfield_scrolled_down_android.png

It's not obvious what details we care about in these goldens. At a minimum, can you make the text field not as wide as the parent, center it, add a border around the viewport? Also, can you take a few minutes to see if it's easy for us to use the golden toolkit to render multiple of these images, together, along with a brief description of each one?

Updated to use the golden toolkit.

super_editor/lib/src/infrastructure/super_textfield/desktop/desktop_textfield.dart

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

matthew-carroll · 2023-04-27T00:53:14Z

super_editor/test_goldens/super_textfield/goldens/super_textfield_scrolled_down.png

These descriptions state what's painted, but not what we're testing. Why do we care that we're at the end? If a developer eventually got a different golden result, and had to re-evaluate it, what should they look for?

Also, should there a caret visible in here?

Updated.

The caret isn't visible because we are not focusing the textfield. Also, using the robot to place the selection is throwing an exception when the textfield is scrollable, as we mentioned before.

Remind me how this situation is happening to users, if we can't simulate it with user interaction in the test?

Well, this test was added to ensure the textfield is being displayed correctly when we scroll it to make the selected line visible. This also happens if the selection is changed without direct interaction with the textfield...

What if you alter the test to place the caret where visible, and then press the down arrow enough times to get to the end, and then press up enough times to get back to the start? Then we'd test real user behavior, and we'd see the caret...

As now we are using the three text fields in the same golden test, when we focus one textfield to press down it will unfocus the others. When SuperTextField loses focus it clears the selection...

matthew-carroll

LGTM

… a visible position (Resolves superlistapp#1096) (superlistapp#1101)

… a visible position (Resolves #1096) (#1101)

[SuperTextField][Mobile] Fix scrolling when changing selection to a v…

ef1c517

…isible position (Resolves superlistapp#1096)

angelosilvestre requested a review from matthew-carroll April 25, 2023 01:29

matthew-carroll reviewed Apr 25, 2023

View reviewed changes

angelosilvestre added 2 commits April 25, 2023 18:52

PR update

b36b3de

Fix scrolling up

6c655fe

angelosilvestre requested a review from matthew-carroll April 26, 2023 00:09

matthew-carroll reviewed Apr 26, 2023

View reviewed changes

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

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

PR update

2938c45

matthew-carroll reviewed Apr 26, 2023

View reviewed changes

angelosilvestre added 2 commits April 25, 2023 22:38

PR updates

30e4968

Add test

74c2662

matthew-carroll reviewed Apr 26, 2023

View reviewed changes

angelosilvestre added 4 commits April 25, 2023 23:55

Add golden test

81b568d

Add goldens and change scrolling with padding

e1497f8

Adjust padding

a8e96a0

PR update

6a752e1

matthew-carroll reviewed Apr 26, 2023

View reviewed changes

angelosilvestre added 2 commits April 26, 2023 21:10

PR updates

e677fe8

Use golden toolkit to build test scenarios

5cfc404

matthew-carroll reviewed Apr 27, 2023

View reviewed changes

PR update

ed35972

angelosilvestre requested a review from matthew-carroll April 27, 2023 01:14

matthew-carroll approved these changes Apr 28, 2023

View reviewed changes

matthew-carroll merged commit 4eefcc5 into superlistapp:main Apr 28, 2023

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Apr 29, 2023

[SuperTextField][Mobile] Fix scrolling when changing the selection to…

2f79fcb

… a visible position (Resolves superlistapp#1096) (superlistapp#1101)

angelosilvestre mentioned this pull request Apr 29, 2023

Cherry-pick "[SuperTextField][Mobile] Fix scrolling when changing the selection to a visible position (Resolves #1096) (#1101)" to stable #1106

Merged

angelosilvestre deleted the 1096_fix_scrolling branch April 29, 2023 02:00

matthew-carroll pushed a commit that referenced this pull request Apr 29, 2023

[SuperTextField][Mobile] Fix scrolling when changing the selection to…

c05eaf8

… a visible position (Resolves #1096) (#1101)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperTextField][Mobile] Fix scrolling when changing the selection to a visible position (Resolves #1096) #1101

[SuperTextField][Mobile] Fix scrolling when changing the selection to a visible position (Resolves #1096) #1101

angelosilvestre commented Apr 25, 2023

matthew-carroll Apr 25, 2023

angelosilvestre Apr 25, 2023

matthew-carroll Apr 26, 2023

matthew-carroll commented Apr 25, 2023

angelosilvestre commented Apr 25, 2023

matthew-carroll commented Apr 25, 2023

angelosilvestre commented Apr 25, 2023

matthew-carroll commented Apr 25, 2023

matthew-carroll commented Apr 25, 2023

matthew-carroll Apr 26, 2023

angelosilvestre Apr 26, 2023

matthew-carroll Apr 26, 2023

matthew-carroll Apr 26, 2023

angelosilvestre Apr 27, 2023

matthew-carroll Apr 27, 2023

angelosilvestre Apr 27, 2023

matthew-carroll Apr 26, 2023

angelosilvestre Apr 27, 2023

matthew-carroll Apr 27, 2023

angelosilvestre Apr 27, 2023

matthew-carroll Apr 27, 2023

angelosilvestre Apr 27, 2023

matthew-carroll Apr 27, 2023

angelosilvestre Apr 27, 2023

matthew-carroll left a comment

[SuperTextField][Mobile] Fix scrolling when changing the selection to a visible position (Resolves #1096) #1101

[SuperTextField][Mobile] Fix scrolling when changing the selection to a visible position (Resolves #1096) #1101

Conversation

angelosilvestre commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Apr 25, 2023

angelosilvestre commented Apr 25, 2023

matthew-carroll commented Apr 25, 2023

angelosilvestre commented Apr 25, 2023

matthew-carroll commented Apr 25, 2023

matthew-carroll commented Apr 25, 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

matthew-carroll left a comment

Choose a reason for hiding this comment