Desktop edge scrolling by justinmc · Pull Request #93170 · flutter/flutter

justinmc · 2021-11-05T23:50:43Z

When selection moves on desktop, the field will now scroll to view the scroll extent, like it already does on mobile.

TODO:

Make sure horizontal edge scrolling works.
Make sure this works on Windows.
Tests.

Fixes #32344
Fixes #59141

justinmc · 2021-11-09T00:40:57Z

@mdebbar Is this old TODO of yours actually fixed?

flutter/packages/flutter/lib/src/widgets/text_selection.dart

Lines 1530 to 1531 in 2a00eac

    
           // TODO(mdebbar): Support dragging in any direction (for multiline text). 
        
           // https://github.com/flutter/flutter/issues/28676

I ran into a strange problem where vertical mouse dragging wasn't being detected properly in a test. I'm wondering if there's anything unimplemented there that could be causing it, or if that TODO should be removed.

mdebbar · 2021-11-09T19:29:42Z

@justinmc if you look at that code, it's only using a HorizontalDragGestureRecognizer which doesn't detect vertical dragging. At that time, vertical dragging was used for scrolling (even on web, because there was no scroll bars yet). But now I think that's not the case anymore. Maybe we should use PanGestureRecognizer?

justinmc · 2021-11-10T18:19:31Z

Sounds good, thanks for the info. I'll make a quick attempt at using PanGestureRecognizer and see if that fixes the strange missing vertical drag component that I saw in the test. If it seems complicated, I'll save it for another PR.

…lsewhere

justinmc · 2021-11-10T22:55:04Z

  DragStartDetails? _lastDragStartDetails;
  DragUpdateDetails? _lastDragUpdateDetails;
  Timer? _dragUpdateThrottleTimer;
+  bool _isMouseDragging = false;


HorizontalDragGestureRecognizer allows you to specify the type of pointer to accept, but now that we've switched to PanGestureRecognizer, which doesn't support that, we have to manually make sure we only listen to mouse drags.

Would it be possible to add that to the existing PanGestureRecognizer or use a private subclass of PanGestureRecognizer here?

Great idea, looks like it's actually only a 2 line change to add it to PanGesureRecognizer because the super class already supports it.

justinmc · 2021-11-11T01:09:35Z

@mdebbar Switching to PanGestureRecognizer worked without too much tweaking, and it fixed my test! I've replaced it here in this PR and removed your TODO.

LongCatIsLooong · 2021-11-11T21:53:39Z

+    switch (defaultTargetPlatform) {
+      case TargetPlatform.iOS:
+      case TargetPlatform.macOS:
+        if (cause == SelectionChangedCause.longPress


Just curious what does a long-press do on iOS and macOS? Is it the force press floating cursor that makes them different?

Yup this handles the thing where you force press and then drag the cursor.

…o text selection

LongCatIsLooong · 2021-11-12T23:15:41Z

+    PointerDeviceKind? kind,
+  }) : super(
+         debugOwner: debugOwner,
+         kind: kind,


kind is deprecated I think?

Ah I missed that, thanks. I'll do the same thing but with supportedDevices as the deprecation message suggests.

LongCatIsLooong

LGTM. This should also close #28676 (the TODO issue) right?

justinmc · 2021-11-15T21:18:14Z

@LongCatIsLooong That one is already closed, but I guess the TODO was missed.

justinmc · 2021-11-18T23:56:15Z

Adding the cherry pick tag after discussing with @mariamhas, if we're still doing cherry picks for 2.8. Shouldn't be a high risk of regressions.

devoncarew · 2021-11-30T19:21:26Z

@justinmc and @HansMuller - when was this regression introduced? What's the impact if we don't take it for this hotfix? We should carefully weigh taking it now (the week right before the release) w/ delaying until a batch of fixes after the release.

justinmc · 2021-11-30T20:52:14Z

I'm ok with skipping this. The behavior without it is fairly bad, but it's actually not a regression. Also there were previously a few goldens issues with this. Probably best to let it be released normally if @mariamhas is alright with that.

mariamhas · 2021-11-30T21:02:35Z

Leaving it out is fine with me, I understand the concern of this being hotfixed so close to the release date.

…

On Tue, Nov 30, 2021 at 12:52 PM Justin McCandless ***@***.***> wrote: I'm ok with skipping this. The behavior without it is fairly bad, but it's actually not a regression. Also there were previously a few goldens issues with this. Probably best to let it be released normally if @mariamhas <https://github.com/mariamhas> is alright with that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93170 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANRXIQKGRH6DQRSBZDKTJSLUOU2QXANCNFSM5HO456BA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

justinmc added 4 commits November 5, 2021 13:47

Works for Mac using same mechanism as iOS

90c18dc

Also reveal extent for keyboard selection changes

9dcde63

Do the same for other desktop platforms

1f48158

Do the same for CupertinoTextField

9a682be

justinmc self-assigned this Nov 5, 2021

flutter-dashboard Bot added a: text input Entering text in a text field or keyboard related problems p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Nov 5, 2021

google-cla Bot added the cla: yes label Nov 5, 2021

Test and get platforms right in logic.

e309a02

justinmc force-pushed the edge-scrolling-desktop branch from b47b649 to e309a02 Compare November 8, 2021 20:50

Vertical tests

91b9551

justinmc added 2 commits November 8, 2021 16:44

Update Cupertino logic to match Material

bd9d3bd

Cupertino test platforms correction

bb1abaf

justinmc added 3 commits November 9, 2021 12:47

Imprecise tests

b3667bf

Merge branch 'master' into edge-scrolling-desktop

815e1aa

More imprecise tests

6e7e3fc

Scrolling due to keyboard is already handled elsewhere

904d164

justinmc force-pushed the edge-scrolling-desktop branch from 0515541 to 904d164 Compare November 10, 2021 19:27

Keyboard tests don't work on web

ccad131

justinmc force-pushed the edge-scrolling-desktop branch from aeb96e8 to ccad131 Compare November 10, 2021 21:55

justinmc added 3 commits November 10, 2021 13:55

Try switching to PanGestureRecognizer

9b69c11

Mouse vertical test now that PanGestureRecognizer allows it

7782a3f

Cupertino also shouldn't handle keyboard selection changes, handled e…

e8c2236

…lsewhere

justinmc commented Nov 10, 2021

View reviewed changes

More test precision

a947944

justinmc marked this pull request as ready for review November 11, 2021 01:07

justinmc requested a review from LongCatIsLooong November 11, 2021 01:08

LongCatIsLooong reviewed Nov 11, 2021

View reviewed changes

Add 'kind' support for PanGestureRecognizer instead of hacking it int…

9e91162

…o text selection

LongCatIsLooong reviewed Nov 12, 2021

View reviewed changes

justinmc added 2 commits November 12, 2021 15:34

Double check and standardize the test precision stuff.

14be3c9

Use the new supportedDevices instead of deprecated

1ec20f0

LongCatIsLooong approved these changes Nov 14, 2021

View reviewed changes

justinmc added the waiting for tree to go green label Nov 15, 2021

fluttergithubbot merged commit 297a7c7 into flutter:master Nov 16, 2021

justinmc deleted the edge-scrolling-desktop branch November 16, 2021 23:05

justinmc added the cp: 2.8 label Nov 18, 2021

WizzXu pushed a commit to WizzXu/flutter that referenced this pull request Nov 19, 2021

Desktop edge scrolling (flutter#93170)

d8b48cc

justinmc mentioned this pull request Dec 3, 2021

☂️ Text editing for desktop - stable #90563

Closed

6 of 7 tasks

devoncarew removed the cp: 2.8 label Dec 14, 2021

jiyuujin mentioned this pull request Feb 4, 2022

2022/02/04 で喋ったこと Vol.183 jiyuujin/ohayo-developers#78

Open

kevmoo mentioned this pull request Nov 22, 2022

[Selection] How To Avoid '!_selectionStartsInScrollable': is not true. assertion when using SelectionArea with scrollable widget. #115787

Closed

Conversation

justinmc commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Nov 9, 2021

Uh oh!

mdebbar commented Nov 9, 2021

Uh oh!

justinmc commented Nov 10, 2021

Uh oh!

justinmc Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

justinmc commented Nov 11, 2021

Uh oh!

LongCatIsLooong Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

justinmc Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

justinmc Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Nov 15, 2021

Uh oh!

justinmc commented Nov 18, 2021

Uh oh!

devoncarew commented Nov 30, 2021

Uh oh!

justinmc commented Nov 30, 2021

Uh oh!

mariamhas commented Nov 30, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

justinmc commented Nov 5, 2021 •

edited

Loading

LongCatIsLooong Nov 11, 2021 •

edited

Loading

LongCatIsLooong left a comment •

edited

Loading