[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) #1463

rutvik110 · 2023-09-21T19:45:22Z

[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438)

Issue:

Steps to reproduce:

Wrap a SuperTextField within a Scrollable, for e.g. CustomScrollView.
Tap on SuperTextField to make it focused.
Press Page Up/Down.

This actions no longer trigger the scroll on the ancestor Scrollable. Intended behaviour is to have the ancestor Scrollable to scroll up/down depending on the key press.

Solution:

This seems to have been working before we updated how we handle shortcuts here. This change now propogates the OS level shortcuts to the IME instead of handling them through defined key handlers, which was causing some issue as discussed in that PR.

This PR introduces new selectors and key handlers at SuperDesktopTextField level to handle Page Up/Down, Home/End shortcuts to scroll the ancestor Scrollable if any present.

Screen.Recording.2023-09-23.at.12.26.21.AM.mov

Changes:

Adds 4 new keyboard handlers to defaultTextFieldKeyboardHandlers and defaultTextFieldImeKeyboardHandlers for SuperDesktopTextField,
- scrollOnPageUpKeyPress
- scrollOnPageDownKeyPress
- scrollOnCtrlOrCmdAndHomeKeyPress
- scrollOnCtrlOrCmdAndEndKeyPress
Adds 4 new selectors to defaultTextFieldSelectorHandlers for SuperDesktopTextField,
- _scrollPageUp
- _scrollPageDown
- _scrollToBeginningOfDocument
- _scrollToEndOfDocument
TextFieldKeyboardHandler updated with an additional Scrollable? parameter.
SuperTextFieldSelectorHandler updated with an additional Scrollable? parameter.
Added _findAncestorScrollable which looks for the ancestor Scrollable and returns it if one is present to be later used for performing scroll actions on the Scrollable through newly added scroll selectors and handlers.

rutvik110 · 2023-09-22T19:32:19Z

@angelosilvestre Let me know if this is the right direction that we wanna move in with this issue. If yes, then I'll get started on one additional thing @matthew-carroll mentioned in the issue and some docs and tests for this changes.

Some clarification on this, if a text field has enough content to scroll, page up/down should apply to the text field. If the text field doesn't have any scrollable content, then the text field should let the page up/down keys bubble up.

angelosilvestre

Sorry be the delayed review. I left some comments.

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

angelosilvestre · 2023-09-23T17:00:51Z

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

    return Actions(
      actions: defaultTargetPlatform == TargetPlatform.macOS ? disabledMacIntents : {},
      child: SuperTextFieldKeyboardInteractor(
        focusNode: _focusNode,
        textController: _controller,
        textKey: _textKey,
        keyboardActions: widget.keyboardHandlers,
+        ancestorScrollable: ancestorScrollable,


IMO, we could query the ancestor scrollable in SuperTextFieldKeyboardInteractor and SuperTextFieldImeInteractor instead of passing via constructor.

Won't that duplicate _findAncestorScrollable method if we had to query for ancestor scrollable in both of those places separately?

Yes, we will some duplication, but IMO this is acceptable. Passing an ancestorScrollable to SuperTextFieldKeyboardInteractor seems worse to me, as SuperTextFieldKeyboardInteractor can query itself for an ancestor scrollable.

Now that we've decided to introduce SuperTextFieldContext, do you think it would best to now just pass that context to SuperTextFieldKeyboardInteractor and to any widget that may've to deal with ancestor scrollable or the scroll controller of the SuperTextField?

We haven't decided this yet. Is that the only open question on this PR at this point? Is that blocking all other PR updates?

Sounds like we should pass both the BuildContext of the SuperTextField, as well as the internal scrollable.

Does SuperTextField have an interface for its scrolling system the way that SuperEditor does?

Is it clear to you what such a scrolling API should look like? Are you comfortable introducing that? Or would you prefer that I do it?

I've a vague idea abt it. Do we want to introduce an api like AutoScroller that will expose certain scrolling info and actions, so that we've a single source from where scrolling actions can be performed and affect the scrolling of the SuperTextField?
If yes, what kind of things should this scrolling api expose? and what parts of the existing scrolling system defined for each individual SuperDesktopTextField, SuperAndroidTextField and SuperIOSTextField should it replace?

If I can get a little bit idea of it, then I can start on it.

Sounds like I should take care of that. For the moment, go ahead and expose the existing scrolling artifact to implement this and then file an issue to introduce a scrolling API for SuperTextField.

angelosilvestre · 2023-09-23T17:03:58Z

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

@@ -966,6 +990,7 @@ class _SuperTextFieldKeyboardInteractorState extends State<SuperTextFieldKeyboar
        controller: widget.textController,
        textLayout: widget.textKey.currentState!.textLayout,
        keyEvent: keyEvent,
+        scrollable: widget.ancestorScrollable,


It doesn't seem right to be passing the scrollable directly as an argument to the handlers. Maybe would could introduce a SuperTextFieldContext, like we have in SuperEditor.

We should get Matt's opinion on this.

That would be much better.

@matthew-carroll Wdyt?

It seems like this question might be moot based on the answer to the question above this one, right? Or are both questions equally relevant?

I think both are equally relevant. What @angelosilvestre may be questioning is whether other selectors/handlers should be aware of the scrollable directly as opposed to it being used through a SuperTextFieldContext.
Passing the SuperTextFieldContext to later access scrollable if needed seems more appropriate but whether to introduce it for a scrollable alone is debatable.

@rutvik110 I think you should go ahead and introduce a SuperTextFieldContext. If at a later point we need to access another resource on the keyboard handlers we don't want to change the method signature again.

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

matthew-carroll · 2023-09-25T21:04:08Z

@rutvik110 @angelosilvestre before I take a deeper look at this, can you two determine which of the open issues are considered resolved? If you have issues that aren't resolved, but don't necessarily require my input, can you work through those, too?

rutvik110 · 2023-09-26T03:44:51Z

Yah, most of the open issues won't require you tagging in but just the one related to addition of SuperTextFieldContext here.

matthew-carroll · 2023-09-28T20:11:49Z

@rutvik110 please try to resolve everything you can with @angelosilvestre before bringing me in. Otherwise, the conversations are likely to overlap and become confusing. So I'll continue to wait until both of you think it's time for me to get involved.

angelosilvestre

I left some comments.

angelosilvestre · 2023-09-28T23:25:35Z

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

    return Actions(
      actions: defaultTargetPlatform == TargetPlatform.macOS ? disabledMacIntents : {},
      child: SuperTextFieldKeyboardInteractor(
        focusNode: _focusNode,
        textController: _controller,
        textKey: _textKey,
        keyboardActions: widget.keyboardHandlers,
+        ancestorScrollable: ancestorScrollable,


Yes, we will some duplication, but IMO this is acceptable. Passing an ancestorScrollable to SuperTextFieldKeyboardInteractor seems worse to me, as SuperTextFieldKeyboardInteractor can query itself for an ancestor scrollable.

angelosilvestre · 2023-09-28T23:26:31Z

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

@@ -966,6 +990,7 @@ class _SuperTextFieldKeyboardInteractorState extends State<SuperTextFieldKeyboar
        controller: widget.textController,
        textLayout: widget.textKey.currentState!.textLayout,
        keyEvent: keyEvent,
+        scrollable: widget.ancestorScrollable,


@rutvik110 I think you should go ahead and introduce a SuperTextFieldContext. If at a later point we need to access another resource on the keyboard handlers we don't want to change the method signature again.

rutvik110 · 2023-10-03T19:06:47Z

@angelosilvestre Want to hear your thoughts on this one particular edge case here,

Scenario: SuperDesktopField and ancestor scrollable both has scrollable content within them. On initial End key press, we scrolled to the bottom of the SuperDesktopField, and on next Page-Down or End key press, we scrolled towards ancestor scrollable's bottom.
Now, upon pressing the Page-Up or Home key....

Actual behaviour: Currently, we first try to scroll within the SuperDesktopField and once its scroll content is consumed, we scroll the ancestor scrollable. This presents a ux issue as seen in the below demo, where we are not able to scroll the ancestor scrollable unless the scrollable action can't be performed within SuperDesktopField,
for e.g. once we reach to the top of the SuperDesktopField, then only Page-Up/Home key press affects the ancestor scrollable.

Screen.Recording.2023-10-04.at.12.34.06.AM.mov

Expected behaviour: Need to be decided.

matthew-carroll · 2023-10-03T19:10:27Z

@rutvik110 what behavior do you get when you do something similar in a web browser, where your text field scrolls and the window scrolls?

rutvik110 · 2023-10-03T19:24:16Z

Tested it on github, it consumes the textfield content first before it scrolls the page. I think that's the natural behaviour across web for this scenario.

Screen.Recording.2023-10-04.at.12.50.09.AM.mov

matthew-carroll · 2023-10-03T19:40:13Z

@rutvik110 that video looks like it mixes directions. It doesn't seem to be testing exactly the same thing you showed in thep revious demo. Can you try testing the exact same thing in both cases to make sure we don't miss anything?

rutvik110 · 2023-10-03T19:51:32Z

Sure! Here it's, the behaviour remains the same in both directions.

Screen.Recording.2023-10-04.at.1.19.33.AM.mov

angelosilvestre · 2023-10-04T22:28:31Z

@rutvik110 If I understood it correctly, the problem is that the ancestor scrollable is scrolled all the way to the bottom, and the textfield is scrolling its own content up instead of the scrollable. Is that correct? If so, one thing we could do it to scroll the ancestor scrollable in PAGE UP/DOWN if the textfield isn't visible on the viewport. Once it's visible, we should use the regular behavior of scrolling the content first and then the ancestor scrollable.

rutvik110 · 2023-10-05T02:14:10Z

Hmm, but the natural behaviour for this case as seen across web is that textfields content is being scrolled first as demonstrated in my previous comment.

Shouldn't we follow that for keeping consistency with what a user may expect in this scenario?

angelosilvestre · 2023-10-05T22:14:36Z

@matthew-carroll What's your thoughts on this? Should we aim to be consistent with how textfields work on browsers? Should we try to improve the UX?

rutvik110 · 2023-10-14T08:10:10Z

@angelosilvestre Currently, within textfield, the newly added scrollOnHomeKeyPress and scrollOnEndKeyPress handlers are conflicting with the existing moveToLineStartWithHome and moveToLineEndWithEnd that also need to respond to the HOME and END key press.

Should we update this two scroll key handlers to may be this particular combination of keys for desktop, scrollOnCtrlOrCmdAndHomeKeyPress and scrollOnCtrlOrCmdAndEndKeyPress. This is how we earlier dealt with this particular issue in context of SuperEditor scrolling.

angelosilvestre · 2023-10-14T16:04:46Z

@rutvik110 I think moveToLineStartWithHome and moveToLineEndWithEnd run only for Windows and Linux. Also, scrolling with HOME and END seems to be a mac only shortcut.

I think we probably want to transmit this information in the method name, like moveToLineStartWithHomeOnWindowsAndLinux, scrollOnHomeKeyPressOnMac and so on.

rutvik110 · 2023-10-14T17:09:28Z

yah, that's right. Those two handlers run only for Windows and Linux.

Also, scrolling with HOME and END seems to be a mac only shortcut.

Yes but this keys may also scroll the document/page on other platforms but that would vary from app to app basis or whether they're pressed in an browser. Normally HOME and END would scroll to start/end of the document or move the cursor to the start/end of the line.

What we've done in case of SuperEditor was to attach this two scroll actions to ctrl/cmd+home/end shortcuts to get around this. Should we be following similar approach here? Seems only right as otherwise we may've conflict with existing handlers that may need to respond to home/end keys on other platforms except mac.

Also, we could may be add a standalone key handler for mac that scrolls the textfield based on home/end key press, and for windows and linux follow the SuperEditor approach for defining these handlers for scrolling that would combine ctrl+home/end to scroll the textfield.

angelosilvestre · 2023-10-14T17:50:42Z

If I understood it correctly, I think we should use separate key handlers. For example:

scrollOnHomeOnMacOrWeb,
scrollOnCtrlHome, (for both command on mac and ctrl on windows and linux)
moveToLineStartWithHomeOnWindowsAndLinux,
...

Does that make sense?

rutvik110 · 2023-10-14T18:06:28Z

scrollOnCtrlHome, (for both command on mac and ctrl on windows and linux)

Didn't got what you're trying to say here? For mac and web, won't the creation of scrollOnHomeOnMacOrWeb you mentioned above it would suffice?

angelosilvestre · 2023-10-14T20:06:56Z

Yeah, if command + home doesn't have any special behavior on Mac then we'll only need scrollOnCtrlHomeOnWindowsAndLinux

rutvik110 · 2023-10-15T09:26:07Z

@angelosilvestre Pushed the updates based on latest addition from #1507.

Overview:

We've moved to responding on ctrl/cmd + home/end press to scroll textfield/ancestor scrollable to the beginning/end of the content instead of only relying on home/end key press, which was conflicting with existing handlers that moved caret to start/end of line on home/end key press on windows and linux.

Also added scrollOnHomeOnMacOrWeb and scrollOnEndOnMacOrWeb as you suggested to scroll content on home/end press on mac/web as the general behaviour for this two keys on mac/web is to scroll the document/page.

And ancestorScrollable is now fetched within this and other mac scroll selectors as needed using passed BuildContext through TextFieldScroller, instead of passing it down through constructors.

rutvik110 · 2023-10-15T10:25:22Z

@angelosilvestre I also noticed that for TextInputSource.ime, the usual ctrl+a/e doesn't move the caret along the start/end of the line? Isn't that the expected behaviour on mac?

This seems to be happening due to the moveCaretToStartOrEnd handler is placed below sendKeyEventToMacOs within defaultTextFieldImeKeyboardHandlers, and thus never gets chance to respond to this shortcuts on mac.

Is that intentded? If not, I can update it within this PR or should I file a separate issue for it?

angelosilvestre · 2023-10-15T11:40:27Z

@rutvik110 Command + a should generate a selector on Mac. That's why the method is placed below sendKeyEventToMacOs.

Maybe I missed the implementation for that selection. Could you please confirm that onPerformSelector is being called when command + a is pressed?

…` for tapping within `SuperDesktopTextField`

…op platforms.

… tests

… to KeyEvent migration

matthew-carroll

LGTM

…selectors and handlers to SuperDesktopTextField (Resolves #1438) (#1463)

rutvik110 requested a review from angelosilvestre September 21, 2023 19:45

rutvik110 self-assigned this Sep 21, 2023

rutvik110 requested a review from matthew-carroll September 22, 2023 19:14

rutvik110 changed the title ~~Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438)~~ [SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) Sep 22, 2023

angelosilvestre reviewed Sep 23, 2023

View reviewed changes

angelosilvestre reviewed Sep 28, 2023

View reviewed changes

rutvik110 requested a review from angelosilvestre October 4, 2023 18:14

rutvik110 mentioned this pull request Oct 4, 2023

[SuperTextField] Introduce a scrolling api #1487

Closed

rutvik110 force-pushed the 1438_page_scroll_with_supertextfield branch from 352098c to 5bbc644 Compare October 15, 2023 09:14

rutvik110 added 21 commits December 8, 2023 13:09

scroll handlers docs updates

7d8d189

added placeCaretInSuperDesktopTextField within `SuperTextFieldRobot…

155432d

…` for tapping within `SuperDesktopTextField`

removed redundant parameter

550f88f

refactor

3dca044

test setup update

25b1b8e

added new test group for running a test across mac, and all web deskt…

273f459

…op platforms.

using SuperTextField in the widget tree

d65521b

setting platform configuration to desktop for SuperTextField

9e20b32

reverted unwanted changes

5de64d9

renamed 'placement' to verticalAlignment

3cfea2e

test files renamed

1ddb85b

refactored scrolling variant

46f2869

scroll variant updated

d0dc44d

removed SuperDesktopTextField references, and using SuperTextField in…

91d0251

… tests

added extension to find ancestor scrollable within current context

13c9820

docs updates

800a4b6

docs updates

d792a97

migrated usage of RawKeyDownEvent and other deprecated things related…

3176606

… to KeyEvent migration

renamed test file

f34990b

test descriptions update

45fc989

test description updates, removed multiline descriptions

565078e

rutvik110 force-pushed the 1438_page_scroll_with_supertextfield branch from 3ccc555 to 565078e Compare December 8, 2023 07:50

rutvik110 requested a review from matthew-carroll December 9, 2023 07:08

matthew-carroll approved these changes Dec 11, 2023

View reviewed changes

matthew-carroll merged commit ea742aa into main Dec 11, 2023
10 of 11 checks passed

matthew-carroll deleted the 1438_page_scroll_with_supertextfield branch December 11, 2023 03:39

github-actions bot pushed a commit that referenced this pull request Dec 11, 2023

[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End …

29c5ad5

…selectors and handlers to SuperDesktopTextField (Resolves #1438) (#1463)

github-actions bot mentioned this pull request Dec 11, 2023

Cherry Pick: [SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) #1662

Closed

matthew-carroll mentioned this pull request Dec 11, 2023

Cherry Pick: [SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) #1664

Merged

matthew-carroll pushed a commit that referenced this pull request Dec 11, 2023

[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End …

53b90ad

…selectors and handlers to SuperDesktopTextField (Resolves #1438) (#1463)

[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) #1463

[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) #1463

Conversation

rutvik110 commented Sep 21, 2023 • edited Loading

Issue:

Solution:

Changes:

rutvik110 commented Sep 22, 2023

angelosilvestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rutvik110 Sep 23, 2023 • 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

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 commented Sep 25, 2023

rutvik110 commented Sep 26, 2023

matthew-carroll commented Sep 28, 2023

angelosilvestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rutvik110 commented Oct 3, 2023

matthew-carroll commented Oct 3, 2023

rutvik110 commented Oct 3, 2023

matthew-carroll commented Oct 3, 2023

rutvik110 commented Oct 3, 2023

angelosilvestre commented Oct 4, 2023

rutvik110 commented Oct 5, 2023

angelosilvestre commented Oct 5, 2023

rutvik110 commented Oct 14, 2023 • edited Loading

angelosilvestre commented Oct 14, 2023

rutvik110 commented Oct 14, 2023 • edited Loading

angelosilvestre commented Oct 14, 2023

rutvik110 commented Oct 14, 2023 • edited Loading

angelosilvestre commented Oct 14, 2023

rutvik110 commented Oct 15, 2023 • edited Loading

rutvik110 commented Oct 15, 2023

angelosilvestre commented Oct 15, 2023

matthew-carroll left a comment

Choose a reason for hiding this comment

rutvik110 commented Sep 21, 2023 •

edited

Loading

rutvik110 Sep 23, 2023 •

edited

Loading

rutvik110 commented Oct 14, 2023 •

edited

Loading

rutvik110 commented Oct 14, 2023 •

edited

Loading

rutvik110 commented Oct 14, 2023 •

edited

Loading

rutvik110 commented Oct 15, 2023 •

edited

Loading