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

Jump to bottom

Page Up / Page Down in text fields #107602

Merged
merged 43 commits into from Oct 25, 2022
Merged

Page Up / Page Down in text fields #107602

merged 43 commits into from Oct 25, 2022

Conversation

tgucio
Copy link
Contributor

@tgucio tgucio commented Jul 13, 2022 •
edited

This PR implements the page up/down and shift + page up/down keyboard shortcuts:

  • page up/down move caret
  • shift page up/down extend the selection by page
  • traversing text using page up/down keeps vertical text runs
  • no changes in ScrollAction

page-up-down

On macOS and iOS, the shortcuts use Alt+PageUp/Down. Without the Alt modifier, the keys scroll without moving the caret, which is implemented in PR #105497.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems. framework flutter/packages/flutter repository. See also f: labels. labels Jul 13, 2022
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jul 13, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

This was referenced Jul 13, 2022
Closed
Merged
@tgucio
Copy link
Contributor Author

tgucio commented Jul 14, 2022

/cc @justinmc @goderbauer as this is now ready for the 1st round of reviews.

@tgucio tgucio changed the title [WIP] Page Up / Page Down in text fields Page Up / Page Down in text fields Jul 14, 2022
@HansMuller HansMuller requested a review from justinmc Jul 15, 2022
justinmc
justinmc reviewed Sep 29, 2022
View changes
Copy link
Contributor

@justinmc justinmc left a comment

Some comments here but overall this is looking good. I think the test failures might be flakes, but I was having trouble re-running them. We'll see if they go away when you push a new commit.

packages/flutter/lib/src/rendering/editable.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/editable.dart Show resolved Hide resolved
packages/flutter/lib/src/widgets/default_text_editing_shortcuts.dart
const SingleActivator(LogicalKeyboardKey.arrowLeft): const ExtendSelectionByCharacterIntent(forward: false, collapseSelection: true),
const SingleActivator(LogicalKeyboardKey.arrowRight): const ExtendSelectionByCharacterIntent(forward: true, collapseSelection: true),
const SingleActivator(LogicalKeyboardKey.arrowUp): const ExtendSelectionVerticallyToAdjacentLineIntent(forward: false, collapseSelection: true),
const SingleActivator(LogicalKeyboardKey.arrowDown): const ExtendSelectionVerticallyToAdjacentLineIntent(forward: true, collapseSelection: true),

// Shift + Arrow: Extend Selection.
// Shift + Arrow: Extend selection.
Copy link
Contributor

@justinmc justinmc Sep 29, 2022

Choose a reason for hiding this comment

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

👍

packages/flutter/lib/src/widgets/editable_text.dart Outdated
VerticalCaretMovementRun? _verticalMovementRun;
TextSelection? _runSelection;

void stopCurrentVerticalRunIfSelectionChanges() {
Copy link
Contributor

@justinmc justinmc Sep 29, 2022

Choose a reason for hiding this comment

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

Can this method be removed?

Copy link
Contributor Author

@tgucio tgucio Oct 25, 2022 •
edited

Choose a reason for hiding this comment

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

I don't think so: vertical runs should be kept with both arrows and page up/down. I now see though that the call to it is missing in _didChangeTextEditingValue() so I'll add that.

I'll experiment with mergint the two Line/Page intents and actions into one to simplify things a bit.

packages/flutter/lib/src/widgets/editable_text.dart Outdated
(intent.forward ? 1.0 : -1.0) * state.renderEditable.size.height);
final TextPosition newExtent = shouldMove
? currentRun.current
: (intent.forward ? TextPosition(offset: state._value.text.length) : const TextPosition(offset: 0));
Copy link
Contributor

@justinmc justinmc Sep 29, 2022

Choose a reason for hiding this comment

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

Are these parentheses necessary?

Copy link
Contributor Author

@tgucio tgucio Oct 25, 2022

Choose a reason for hiding this comment

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

That was a copy&paste from the *Line class - should be OK to remove.

packages/flutter/lib/src/widgets/editable_text.dart Outdated
final bool collapseSelection = intent.collapseSelection || !state.widget.selectionEnabled;
final TextEditingValue value = state._textEditingValueforTextLayoutMetrics;
if (!value.selection.isValid) {
return;
Copy link
Contributor

@justinmc justinmc Sep 29, 2022

Choose a reason for hiding this comment

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

Can you explain in what situation this return will get called?

packages/flutter/test/widgets/editable_text_shortcuts_test.dart Outdated

await sendKeyCombination(tester, const SingleActivator(LogicalKeyboardKey.pageDown));
await tester.pump();
expect(controller.selection, const TextSelection.collapsed(offset: 81));
Copy link
Contributor

@justinmc justinmc Sep 29, 2022

Choose a reason for hiding this comment

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

Shouldn't the selection stay the same on Mac/iOS?

@Piinks Piinks self-requested a review Oct 4, 2022
@justinmc
Copy link
Contributor

justinmc commented Oct 19, 2022

@tgucio The test failures now seem to be legitimate, can you take a look?

@tgucio
Copy link
Contributor Author

tgucio commented Oct 25, 2022 •
edited

@justinmc indeed, a colon was missing in one of the macOS selector names. Fixed it and this should be good to go (for another review) now.

Edit: also merged *Line and *Page action classes into one to reuse code and ensure a vertical run is continued with arrow/page dn/up presses

justinmc
justinmc approved these changes Oct 25, 2022
View changes
Copy link
Contributor

@justinmc justinmc left a comment

LGTM 👍

Thanks for following up on this!

@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 25, 2022
@auto-submit auto-submit bot merged commit 563e0a4 into flutter:master Oct 25, 2022
66 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 25, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 25, 2022
@engine-flutter-autoroll
563e0a4aa Page Up / Page Down in text fields (flutter/flutter#107602)
e781ff1
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2022
@engine-flutter-autoroll
563e0a4aa Page Up / Page Down in text fields (flutter/flutter#107602)
208dd97
bparrishMines pushed a commit to flutter/packages that referenced this pull request Oct 27, 2022
@engine-flutter-autoroll
Roll Flutter from 637e5bce662f to d9a2229babbc (125 revisions) (#2740)
8496fe8 
* e4b979679 [flutter_tools] Implement NotifyingLogger.supportsColor (flutter/flutter#113635)

* 5a42f3337 Roll Flutter Engine from d62aa9526a56 to 490b06d13390 (20 revisions) (flutter/flutter#113761)

* 30161d4db Enable cache cleanup. (flutter/flutter#113729)

* 780ceb569 Roll Flutter Engine from 490b06d13390 to bcd83df4ecbd (4 revisions) (flutter/flutter#113766)

* 2668f90d1 Composing text shouldn't be part of undo/redo (flutter/flutter#108765)

* 4f373e1e6 Roll Plugins from e6184f9cca01 to a577c004a4b9 (4 revisions) (flutter/flutter#113769)

* 482466ebf Roll Flutter Engine from bcd83df4ecbd to 3a7acb3dbe9b (6 revisions) (flutter/flutter#113771)

* 483e1d9a6 Validate bins on path in doctor (flutter/flutter#113106)

* 103a5c98b Overlay always applies clip (flutter/flutter#113770)

* c0f822976 Roll Flutter Engine from 3a7acb3dbe9b to 69f2bc881f46 (3 revisions) (flutter/flutter#113780)

* 7caaac20c link "iOS PlatformView BackdropFilter design doc" in the BackdropFilter widget's documentation (flutter/flutter#113779)

* 46d29084e Roll Flutter Engine from 69f2bc881f46 to 3b6ecf9f1a66 (1 revision) (flutter/flutter#113789)

* 37af03830 Roll pub packages (flutter/flutter#113574)

* 23401885b Roll Flutter Engine from 3b6ecf9f1a66 to 2b21a3125ca2 (6 revisions) (flutter/flutter#113798)

* 3665dd835 Start generating coverage. (flutter/flutter#113587)

* 25b10ef9c Roll Flutter Engine from 2b21a3125ca2 to 85e4fa84d660 (3 revisions) (flutter/flutter#113803)

* 59d26c481 Roll Flutter Engine from 85e4fa84d660 to f24ea1a04d93 (3 revisions) (flutter/flutter#113806)

* 07319a986 revert last 2 engine rolls (flutter/flutter#113835)

* 782baecc5 Resolve 113705: Separated longer running tests from `runMisc` to prevent flakiness from timeouts (flutter/flutter#113784)

* 883469229 [web] Use TrustedTypes in flutter.js and other tools (flutter/flutter#112969)

* 1f95a7dfb Roll Plugins from a577c004a4b9 to 7a7480a5f392 (4 revisions) (flutter/flutter#113834)

* 1ebe53c20 Roll pub packages (flutter/flutter#113799)

* 70c1f262e Avoid creating map literal in `flutter.gradle` multidex check (flutter/flutter#113845)

* 8d13c8966 Followup 113705: Allow the `slow` tests to break the tree as all tests in that shard previously could (flutter/flutter#113846)

* 4323397a7 Roll Flutter Engine from 2b21a3125ca2 to 83092c04c105 (20 revisions) (flutter/flutter#113847)

* f4804ad92 Roll Flutter Engine from 2b21a3125ca2 to de83ef6d2c26 (21 revisions) (flutter/flutter#113849)

* 44c146abb Bump actions/upload-artifact from 3.1.0 to 3.1.1 (flutter/flutter#113859)

* 025a3ab3f Roll Flutter Engine from de83ef6d2c26 to 6aa683e365d5 (3 revisions) (flutter/flutter#113863)

* 4c6251a80 Add fontFamilyFallback to ThemeData (flutter/flutter#112976)

* a0731af23 Roll Flutter Engine from 6aa683e365d5 to c03114f1575d (2 revisions) (flutter/flutter#113864)

* b5ac8c551 Roll Flutter Engine from c03114f1575d to 30cec21e4b3c (1 revision) (flutter/flutter#113871)

* 92c3e29ed Roll Flutter Engine from 30cec21e4b3c to e35f850102e6 (1 revision) (flutter/flutter#113872)

* 1b5b469c3 Roll Flutter Engine from e35f850102e6 to 30bb44616bac (3 revisions) (flutter/flutter#113875)

* 35daf3755 Roll Flutter Engine from 30bb44616bac to 9a671daf1c92 (2 revisions) (flutter/flutter#113877)

* 271422633 Roll Flutter Engine from 9a671daf1c92 to 07bdae45e7f9 (1 revision) (flutter/flutter#113888)

* 777040dfc Roll Flutter Engine from 07bdae45e7f9 to 5abc1b8713be (1 revision) (flutter/flutter#113892)

* fd813de68 Roll Flutter Engine from 5abc1b8713be to 51f8ac74b558 (1 revision) (flutter/flutter#113901)

* 6ac2cc531 Roll Flutter Engine from 51f8ac74b558 to 360dcd1cf31a (1 revision) (flutter/flutter#113902)

* e4cc9161a Roll Flutter Engine from 360dcd1cf31a to fb9df8d65dc8 (1 revision) (flutter/flutter#113918)

* 20c4b6c53 Roll Flutter Engine from fb9df8d65dc8 to f62df692058c (1 revision) (flutter/flutter#113919)

* 4a6287654 Roll Flutter Engine from f62df692058c to 7fc208c07693 (1 revision) (flutter/flutter#113926)

* 12b05ca7e Roll Flutter Engine from 7fc208c07693 to 6bb2f03e6fd0 (1 revision) (flutter/flutter#113929)

* a25c86c4d Roll Flutter Engine from 6bb2f03e6fd0 to 2b5d630e4831 (1 revision) (flutter/flutter#113938)

* 28e0f089a Reland "[text_input] introduce TextInputControl" (flutter/flutter#113758)

* 0b1fbd297 Roll Plugins from 7a7480a5f392 to 84f5ec608be4 (2 revisions) (flutter/flutter#113942)

* 15a4b009c Use correct semantics for toggle buttons (flutter/flutter#113851)

* a7fe53656 [framework] re-rasterize when window size or insets changes (flutter/flutter#113647)

* bd9021a0b Roll Flutter Engine from 2b5d630e4831 to 168c711a330e (1 revision) (flutter/flutter#113947)

* 29397c2c7 Fix selectWordsInRange when last word is located before the first word (flutter/flutter#113224)

* 6415de46a Roll Flutter Engine from 168c711a330e to 7cd07782141c (1 revision) (flutter/flutter#113948)

* 700b449d0 Roll Flutter Engine from 7cd07782141c to 2f6c6583058b (2 revisions) (flutter/flutter#113951)

* b4058b95f Update Popup Menu to support Material 3 (flutter/flutter#103606)

* b571abfbf Scribble mixin (flutter/flutter#104128)

* 903e9fb5e Roll Flutter Engine from 2f6c6583058b to f445898c1a28 (1 revision) (flutter/flutter#113959)

* 2dd87fbdf Fix --local-engine for the new web/wasm mode (flutter/flutter#113759)

* 3c2f500b1 Fix edge scrolling on platforms that select word by word on long press move (flutter/flutter#113128)

* 5259e1bc6 Add --empty to the flutter create command (flutter/flutter#113873)

* 0c1430840 Add branch coverage to flutter test (flutter/flutter#113802)

* 7571f308a Roll Flutter Engine from f445898c1a28 to ecf2f85cab61 (5 revisions) (flutter/flutter#113968)

* 391330fdc Roll Flutter Engine from ecf2f85cab61 to 926bb80f49a2 (2 revisions) (flutter/flutter#113971)

* 884f4d058 Updated the Material Design tokens used to generate component defaults to v0.137. (flutter/flutter#113970)

* 694b25355 [Android] Fix spell check integration test guarded function conflict (flutter/flutter#113541)

* 806fb93c2 Fix ScrollPosition.isScrollingNotifier.value for pointer scrolling (flutter/flutter#113972)

* 14a736062 Roll Flutter Engine from 926bb80f49a2 to 7577fd46d50f (1 revision) (flutter/flutter#113974)

* 6154b3b4b Improve Scrollbar drag behavior (flutter/flutter#112434)

* 1037f9919 Roll Flutter Engine from 7577fd46d50f to 24d7b5f255c0 (1 revision) (flutter/flutter#113976)

* e62d519cb Roll Flutter Engine from 24d7b5f255c0 to 75bfcd73ca8a (1 revision) (flutter/flutter#113981)

* 49f0061af Roll Flutter Engine from 75bfcd73ca8a to b93c654bd158 (1 revision) (flutter/flutter#113984)

* 2a59bd521 Roll Flutter Engine from b93c654bd158 to 9abb459368d5 (2 revisions) (flutter/flutter#113992)

* 400136b00 Fix `Slider` overlay and value indicator interactive behavior on desktop. (flutter/flutter#113543)

* b0e7c9c4f Move `AnimatedIcons` example and fix typo in `cupertino/text_selection_toolbar.dart` (flutter/flutter#113937)

* 7f75d2416 Add Material 3 `ProgressIndicator` examples (flutter/flutter#113950)

* 593315ea3 Roll Flutter Engine from 9abb459368d5 to 2b70cba93123 (1 revision) (flutter/flutter#113997)

* 5304a2419 Roll Flutter Engine from 2b70cba93123 to c414b1d57b2b (1 revision) (flutter/flutter#114000)

* 3599b3a87 Add support for expression compilation when debugging integration tests (flutter/flutter#113481)

* 13cb46dd9 Roll Plugins from 84f5ec608be4 to fed91041ddd9 (2 revisions) (flutter/flutter#114019)

* b375b4ac8 Fix an issue that Dragging the iOS text selection handles is jumpy and iOS text selection update incorrectly. (flutter/flutter#109136)

* 563e0a4aa Page Up / Page Down in text fields (flutter/flutter#107602)

* 0fe29f585 Raise an exception when invalid subshard name (flutter/flutter#113222)

* dbbef15a5 Add Focus.parentNode to allow controlling the shape of the Focus tree. (flutter/flutter#113655)

* b373be889 Upgrade gradle for flutter tool to 7.3.0 (flutter/flutter#114023)

* 92d8b04ba [macOS] Flavors project throws `no flavor specified` for creating a project. (flutter/flutter#113979)

* ae143ad8d Hide debug logs from a MemoryAllocations test that intentionally throws an exception (flutter/flutter#113786)

* e13372188 Check for watch companion in build settings (flutter/flutter#113956)

* 9f5c6553b Revert "Check for watch companion in build settings (#113956)" (flutter/flutter#114035)

* df259c588 Add `clipBehavior`  and apply `borderRadius` to DataTable's Material (flutter/flutter#113205)

* e76f88316 Cache TextPainter plain text value to improve performance (flutter/flutter#109841)

* a30d816aa fix stretch effect with rtl support (flutter/flutter#113214)

* af34b1041 Roll Flutter Engine from c414b1d57b2b to 92500cfe7a65 (1 revision) (flutter/flutter#114001)

* 3ce88d381 Replace menu defaults with tokens (flutter/flutter#113963)

* 8c3806f81 Add parentNode to FocusScope widget (flutter/flutter#114034)

* a30c63a60 Roll Flutter Engine from 92500cfe7a65 to e9aba46a7bcb (13 revisions) (flutter/flutter#114038)

* 3ed14a05c Roll Flutter Engine from e9aba46a7bcb to b1f1ec2bae46 (4 revisions) (flutter/flutter#114043)

* b816801ab fix to add both flutter_test and integration_test (flutter/flutter#109650)

* e39fa7a83 Fix wasted memory caused by debug fields - 16 bytes per object (when adding that should-be-removed field crosses double-word alignment) (flutter/flutter#113927)

* d5c53b82e Fix text field label animation duration and curve (flutter/flutter#105966)

* dcf219ef1 Roll Flutter Engine from b1f1ec2bae46 to ce6649aeea6d (1 revision) (flutter/flutter#114044)

* afa9a70bf Roll Flutter Engine from ce6649aeea6d to a34d38ccb726 (1 revision) (flutter/flutter#114046)

* 8b2810421 Roll Flutter Engine from a34d38ccb726 to 71675e2de9f2 (2 revisions) (flutter/flutter#114049)

* dd9b7ac13 Roll Flutter Engine from 71675e2de9f2 to c814452fcb26 (1 revision) (flutter/flutter#114052)

* b00b2f1be Roll Flutter Engine from c814452fcb26 to ac95a3a4cc92 (1 revision) (flutter/flutter#114053)

* dcc6a4c0b Roll Flutter Engine from ac95a3a4cc92 to 199166f7c642 (1 revision) (flutter/flutter#114056)

* e739ad078 M3 Text field UI update (flutter/flutter#113776)

* 89e1fbd30 Roll Flutter Engine from 199166f7c642 to c00953e610fa (1 revision) (flutter/flutter#114059)

* 391f35635 Roll Flutter Engine from c00953e610fa to 3b086a0e98e3 (1 revision) (flutter/flutter#114062)

* 97970acfc Roll Flutter Engine from 3b086a0e98e3 to 56841d491f80 (1 revision) (flutter/flutter#114065)

* 8b3649794 Roll Flutter Engine from 56841d491f80 to 27269992caec (1 revision) (flutter/flutter#114068)

* 7d037f2c3 Roll Flutter Engine from 27269992caec to 25133f15440d (1 revision) (flutter/flutter#114075)

* cb534057e Don't specify libraries-spec argument if we are passing a platform dill. (flutter/flutter#114045)

* e6be9831e Roll Plugins from fed91041ddd9 to cd8bb0a20753 (9 revisions) (flutter/flutter#114077)

* d988c11a8 Expose `alwaysShowMiddle` in `CupertinoSliverNavigationBar` (flutter/flutter#113544)

* 5d9389426 [flutter_tools] Decouple fatal-warnings check from fatal-infos (flutter/flutter#113748)

* 609b8f321 Revert part of "Terminate simulator app on "q" (#113581)" (flutter/flutter#114083)

* 235a3252d Provide test API for accessibility announcements (flutter/flutter#109661)

* e4a80b48c Roll Flutter Engine from 25133f15440d to 2705bcb4e7d6 (1 revision) (flutter/flutter#114084)

* 51acda81f Update Cupertino text input padding (flutter/flutter#113958)

* 7bca82c3f Roll Flutter Engine from 2705bcb4e7d6 to 31d21cbed7b2 (1 revision) (flutter/flutter#114086)

* e334ac112 Revert "Update Cupertino text input padding (#113958)" (flutter/flutter#114102)

* 671c53201 107866: Add support for verifying SemanticsNode ordering in widget tests (flutter/flutter#113133)

* 23d258df5 Remove deprecated `updateSemantics` API usage. (flutter/flutter#113382)

* 156c31321 Dispose animation controller in platform view benchmarks (flutter/flutter#110413)

* 8cd79538c Roll Flutter Engine from 31d21cbed7b2 to a772d20c7550 (12 revisions) (flutter/flutter#114113)

* eaecf4687 Roll Flutter Engine from a772d20c7550 to 871de2f904a4 (6 revisions) (flutter/flutter#114119)

* 732d11701 Roll Flutter Engine from 871de2f904a4 to f0eb4f0bae67 (1 revision) (flutter/flutter#114122)

* d9a2229ba Roll Flutter Engine from f0eb4f0bae67 to 1ea6e5e2de95 (1 revision) (flutter/flutter#114125)
liangxianzhe pushed a commit to liangxianzhe/flutter that referenced this pull request Nov 1, 2022
@tgucio @liangxianzhe
Page Up / Page Down in text fields (flutter#107602)
cb48d8c
@tgucio tgucio deleted the page-up-down-v2 branch Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmc justinmc

@Piinks Piinks

Assignees
No one assigned
Labels
a: text input Entering text in a text field or keyboard related problems. autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tgucio @justinmc