From 9b672714ad26431ce01ec76f3c6fe3d02aad20f5 Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Tue, 2 Jul 2024 14:05:18 +0100 Subject: [PATCH 1/8] feat: allow vertical scrolling for multiline SuperTextFields --- .../lib/src/super_textfield/infrastructure/text_scrollview.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart b/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart index f867a188d3..0bde8b33c8 100644 --- a/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart +++ b/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart @@ -423,7 +423,7 @@ class _TextScrollViewState extends State child: SingleChildScrollView( key: _textFieldViewportKey, controller: _scrollController, - physics: const NeverScrollableScrollPhysics(), + physics: isMultiline ? null : const NeverScrollableScrollPhysics(), scrollDirection: isMultiline ? Axis.vertical : Axis.horizontal, child: Padding( padding: widget.padding ?? EdgeInsets.zero, From afc5f3ce0b56c5b9bf9fb1a6f655881d923bc080 Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Wed, 3 Jul 2024 14:52:12 +0100 Subject: [PATCH 2/8] test: write test cases for vertical scrolling on mobile + desktop --- ...uper_textfield_gesture_scrolling_test.dart | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart index a03fd15a76..ce7805d276 100644 --- a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart +++ b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart @@ -249,6 +249,80 @@ void main() { // Ensure the selection didn't change. expect(SuperTextFieldInspector.findSelection(), TextRange.empty); }); + + testWidgetsOnMobile("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { + final controller = AttributedTextEditingController( + text: AttributedText("A\nB\nC"), + ); + + // Pump the widget tree with a SuperTextField with a maxHeight of 2 lines + // of text, which should overflow considering there are 3 lines of text. + await _pumpTestApp( + tester, + textController: controller, + minLines: 1, + maxLines: 2, + maxHeight: 40, + ); + + // Move selection to the end of the text + // TODO: change to simulate user input when IME simulation is available + controller.selection = const TextSelection.collapsed(offset: 5); + await tester.pumpAndSettle(); + + // Ensure the text field has scrolled to the bottom. + expect( + SuperTextFieldInspector.findScrollOffset(), + greaterThan(0.0), + ); + + // Scroll upwards towards the top of the text field. + await tester.drag(find.byType(SuperTextField), const Offset(0, -1000)); + await tester.pumpAndSettle(); + + // Ensure the text field has scrolled back to the top. + expect( + SuperTextFieldInspector.findScrollOffset(), + 0.0, + ); + }); + + testWidgetsOnDesktop("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { + final controller = AttributedTextEditingController( + text: AttributedText("A\nB\nC"), + ); + + // Pump the widget tree with a SuperTextField with a maxHeight of 2 lines + // of text, which should overflow considering there are 3 lines of text. + await _pumpTestApp( + tester, + textController: controller, + minLines: 1, + maxLines: 2, + maxHeight: 40, + ); + + // Move selection to the end of the text + // TODO: change to simulate user input when IME simulation is available + controller.selection = const TextSelection.collapsed(offset: 5); + await tester.pumpAndSettle(); + + // Ensure the text field has scrolled to the bottom. + expect( + SuperTextFieldInspector.findScrollOffset(), + greaterThan(0.0), + ); + + // Scroll upwards towards the top of the text field. + await tester.drag(find.byType(SuperTextField), const Offset(0, -1000)); + await tester.pumpAndSettle(); + + // Ensure the text field has scrolled back to the top. + expect( + SuperTextFieldInspector.findScrollOffset(), + 0.0, + ); + }); }); } From d8c836a6093d069c120cd4314d5a83ad1e8a55bf Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Wed, 17 Jul 2024 16:22:14 +0100 Subject: [PATCH 3/8] chore: fix multiline scrolling tests for desktop + mobile chore: fix desktop test --- ...uper_textfield_gesture_scrolling_test.dart | 78 ++++++++++++------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart index ce7805d276..76fd8dcec4 100644 --- a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart +++ b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart @@ -251,8 +251,11 @@ void main() { }); testWidgetsOnMobile("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { + final initialText = "The first line of text in the field\n" + "The second line of text in the field\n" + "The third line of text in the field"; final controller = AttributedTextEditingController( - text: AttributedText("A\nB\nC"), + text: AttributedText(initialText), ); // Pump the widget tree with a SuperTextField with a maxHeight of 2 lines @@ -265,31 +268,38 @@ void main() { maxHeight: 40, ); - // Move selection to the end of the text - // TODO: change to simulate user input when IME simulation is available - controller.selection = const TextSelection.collapsed(offset: 5); + double getTextBottom() { + return tester.getBottomRight(find.byType(SuperTextField)).dy; + } + + double getViewportBottom() { + return tester.getBottomRight(find.byType(SuperText)).dy; + } + + // Ensure the text field has not yet scrolled. + expect(getTextBottom(), lessThan(getViewportBottom())); + + // Scroll down to reveal the last line of text. + await tester.drag(find.byType(SuperTextField), const Offset(0, -1000.0)); await tester.pumpAndSettle(); // Ensure the text field has scrolled to the bottom. - expect( - SuperTextFieldInspector.findScrollOffset(), - greaterThan(0.0), - ); + expect(getTextBottom(), equals(getViewportBottom())); - // Scroll upwards towards the top of the text field. - await tester.drag(find.byType(SuperTextField), const Offset(0, -1000)); + // Scroll back up to the top of the text field. + await tester.drag(find.byType(SuperTextField), const Offset(0, 1000.0)); await tester.pumpAndSettle(); // Ensure the text field has scrolled back to the top. - expect( - SuperTextFieldInspector.findScrollOffset(), - 0.0, - ); + expect(getTextBottom(), lessThan(getViewportBottom())); }); testWidgetsOnDesktop("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { + final initialText = "The first line of text in the field\n" + "The second line of text in the field\n" + "The third line of text in the field"; final controller = AttributedTextEditingController( - text: AttributedText("A\nB\nC"), + text: AttributedText(initialText), ); // Pump the widget tree with a SuperTextField with a maxHeight of 2 lines @@ -302,26 +312,38 @@ void main() { maxHeight: 40, ); - // Move selection to the end of the text - // TODO: change to simulate user input when IME simulation is available - controller.selection = const TextSelection.collapsed(offset: 5); + double getTextBottom() { + return tester.getBottomRight(find.byType(SuperTextField)).dy; + } + + double getViewportBottom() { + return tester.getBottomRight(find.byType(SuperText)).dy; + } + + // Ensure the text field has not yet scrolled. + expect(getTextBottom(), lessThan(getViewportBottom())); + + // Scroll down to reveal the last line of text. + await tester.drag( + find.byType(SuperTextField), + const Offset(0, -1000.0), + kind: PointerDeviceKind.trackpad, + ); await tester.pumpAndSettle(); // Ensure the text field has scrolled to the bottom. - expect( - SuperTextFieldInspector.findScrollOffset(), - greaterThan(0.0), - ); + expect(getTextBottom(), equals(getViewportBottom())); - // Scroll upwards towards the top of the text field. - await tester.drag(find.byType(SuperTextField), const Offset(0, -1000)); + // Scroll back up to the top of the text field. + await tester.drag( + find.byType(SuperTextField), + const Offset(0, 1000.0), + kind: PointerDeviceKind.trackpad, + ); await tester.pumpAndSettle(); // Ensure the text field has scrolled back to the top. - expect( - SuperTextFieldInspector.findScrollOffset(), - 0.0, - ); + expect(getTextBottom(), lessThan(getViewportBottom())); }); }); } From aaa5fd403c632199e14fa3fe0b8edd1c62744819 Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Mon, 12 Aug 2024 15:08:58 +0100 Subject: [PATCH 4/8] chore: remove inline functions to calculate top and bottom positions; equals() -> moreOrLessEquals() --- ...uper_textfield_gesture_scrolling_test.dart | 40 +++++++++---------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart index 76fd8dcec4..5ec84ef77b 100644 --- a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart +++ b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart @@ -268,30 +268,28 @@ void main() { maxHeight: 40, ); - double getTextBottom() { - return tester.getBottomRight(find.byType(SuperTextField)).dy; - } - - double getViewportBottom() { - return tester.getBottomRight(find.byType(SuperText)).dy; - } - // Ensure the text field has not yet scrolled. - expect(getTextBottom(), lessThan(getViewportBottom())); + var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + expect(textBottom, lessThan(viewportBottom)); // Scroll down to reveal the last line of text. await tester.drag(find.byType(SuperTextField), const Offset(0, -1000.0)); await tester.pumpAndSettle(); // Ensure the text field has scrolled to the bottom. - expect(getTextBottom(), equals(getViewportBottom())); + textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + expect(textBottom, moreOrLessEquals(viewportBottom)); // Scroll back up to the top of the text field. await tester.drag(find.byType(SuperTextField), const Offset(0, 1000.0)); await tester.pumpAndSettle(); // Ensure the text field has scrolled back to the top. - expect(getTextBottom(), lessThan(getViewportBottom())); + textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + expect(textBottom, lessThan(viewportBottom)); }); testWidgetsOnDesktop("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { @@ -312,16 +310,10 @@ void main() { maxHeight: 40, ); - double getTextBottom() { - return tester.getBottomRight(find.byType(SuperTextField)).dy; - } - - double getViewportBottom() { - return tester.getBottomRight(find.byType(SuperText)).dy; - } - // Ensure the text field has not yet scrolled. - expect(getTextBottom(), lessThan(getViewportBottom())); + var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + expect(textBottom, lessThan(viewportBottom)); // Scroll down to reveal the last line of text. await tester.drag( @@ -332,7 +324,9 @@ void main() { await tester.pumpAndSettle(); // Ensure the text field has scrolled to the bottom. - expect(getTextBottom(), equals(getViewportBottom())); + textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + expect(textBottom, moreOrLessEquals(viewportBottom)); // Scroll back up to the top of the text field. await tester.drag( @@ -343,7 +337,9 @@ void main() { await tester.pumpAndSettle(); // Ensure the text field has scrolled back to the top. - expect(getTextBottom(), lessThan(getViewportBottom())); + textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + expect(textBottom, lessThan(viewportBottom)); }); }); } From 8541fe148cb336232e3255c28a09431281cf38f3 Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Mon, 12 Aug 2024 15:15:19 +0100 Subject: [PATCH 5/8] chore: lessThan() -> lessThanOrEqualTo() --- .../super_textfield_gesture_scrolling_test.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart index 5ec84ef77b..5b8b867109 100644 --- a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart +++ b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart @@ -271,7 +271,7 @@ void main() { // Ensure the text field has not yet scrolled. var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThan(viewportBottom)); + expect(textBottom, lessThanOrEqualTo(viewportBottom)); // Scroll down to reveal the last line of text. await tester.drag(find.byType(SuperTextField), const Offset(0, -1000.0)); @@ -289,7 +289,7 @@ void main() { // Ensure the text field has scrolled back to the top. textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThan(viewportBottom)); + expect(textBottom, lessThanOrEqualTo(viewportBottom)); }); testWidgetsOnDesktop("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { @@ -313,7 +313,7 @@ void main() { // Ensure the text field has not yet scrolled. var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThan(viewportBottom)); + expect(textBottom, lessThanOrEqualTo(viewportBottom)); // Scroll down to reveal the last line of text. await tester.drag( @@ -339,7 +339,7 @@ void main() { // Ensure the text field has scrolled back to the top. textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThan(viewportBottom)); + expect(textBottom, lessThanOrEqualTo(viewportBottom)); }); }); } From 6ab46d63a404d658da4fc056c3434eb87a865d6e Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Mon, 12 Aug 2024 15:29:09 +0100 Subject: [PATCH 6/8] fix: make test more robust, and fail (correctly) more easily if scroll amount changes --- ...uper_textfield_gesture_scrolling_test.dart | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart index 5b8b867109..b6fdd598a3 100644 --- a/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart +++ b/super_editor/test/super_textfield/super_textfield_gesture_scrolling_test.dart @@ -269,17 +269,17 @@ void main() { ); // Ensure the text field has not yet scrolled. - var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; - var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThanOrEqualTo(viewportBottom)); + var textTop = tester.getTopRight(find.byType(SuperTextField)).dy; + var viewportTop = tester.getTopRight(find.byType(SuperText)).dy; + expect(textTop, moreOrLessEquals(viewportTop)); // Scroll down to reveal the last line of text. await tester.drag(find.byType(SuperTextField), const Offset(0, -1000.0)); await tester.pumpAndSettle(); // Ensure the text field has scrolled to the bottom. - textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; - viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; expect(textBottom, moreOrLessEquals(viewportBottom)); // Scroll back up to the top of the text field. @@ -287,9 +287,9 @@ void main() { await tester.pumpAndSettle(); // Ensure the text field has scrolled back to the top. - textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; - viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThanOrEqualTo(viewportBottom)); + textTop = tester.getTopRight(find.byType(SuperTextField)).dy; + viewportTop = tester.getTopRight(find.byType(SuperText)).dy; + expect(textTop, moreOrLessEquals(viewportTop)); }); testWidgetsOnDesktop("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async { @@ -311,9 +311,9 @@ void main() { ); // Ensure the text field has not yet scrolled. - var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; - var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThanOrEqualTo(viewportBottom)); + var textTop = tester.getTopRight(find.byType(SuperTextField)).dy; + var viewportTop = tester.getTopRight(find.byType(SuperText)).dy; + expect(textTop, moreOrLessEquals(viewportTop)); // Scroll down to reveal the last line of text. await tester.drag( @@ -324,8 +324,8 @@ void main() { await tester.pumpAndSettle(); // Ensure the text field has scrolled to the bottom. - textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; - viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; + var textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; + var viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; expect(textBottom, moreOrLessEquals(viewportBottom)); // Scroll back up to the top of the text field. @@ -337,9 +337,9 @@ void main() { await tester.pumpAndSettle(); // Ensure the text field has scrolled back to the top. - textBottom = tester.getBottomRight(find.byType(SuperTextField)).dy; - viewportBottom = tester.getBottomRight(find.byType(SuperText)).dy; - expect(textBottom, lessThanOrEqualTo(viewportBottom)); + textTop = tester.getTopRight(find.byType(SuperTextField)).dy; + viewportTop = tester.getTopRight(find.byType(SuperText)).dy; + expect(textTop, moreOrLessEquals(viewportTop)); }); }); } From bd67a6822fcb692ec562aa016977159dab8835d1 Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Wed, 14 Aug 2024 20:25:06 +0100 Subject: [PATCH 7/8] chore: add a comment to explain scroll physics --- .../lib/src/super_textfield/infrastructure/text_scrollview.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart b/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart index 0bde8b33c8..4032eabeb8 100644 --- a/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart +++ b/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart @@ -423,6 +423,8 @@ class _TextScrollViewState extends State 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. physics: isMultiline ? null : const NeverScrollableScrollPhysics(), scrollDirection: isMultiline ? Axis.vertical : Axis.horizontal, child: Padding( From 7be800c69a1421feefa99103f2996927438bba2d Mon Sep 17 00:00:00 2001 From: Cillian Myles Date: Wed, 14 Aug 2024 21:56:05 +0100 Subject: [PATCH 8/8] chore: update comment to explain scroll physics --- .../src/super_textfield/infrastructure/text_scrollview.dart | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart b/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart index 4032eabeb8..dcf6846adb 100644 --- a/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart +++ b/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart @@ -423,8 +423,10 @@ class _TextScrollViewState extends State 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. + // For single-line text fields, we do not allow horizontal scrolling, + // therefor we apply NeverScrollableScrollPhysics. For multi-line text + // fields, we pass null to allow the SingleChildScrollView to default + // to the appropriate scroll physics based on the host platform. physics: isMultiline ? null : const NeverScrollableScrollPhysics(), scrollDirection: isMultiline ? Axis.vertical : Axis.horizontal, child: Padding(