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
Remove mouse tap text drag selection throttling to improve responsiveness #123460
Conversation
cc @justinmc |
Thank you for the PR @loune! @mdebbar I think you originally added text selection throttling when using the mouse, can you comment on this idea to remove it? It does feel much better to me without the throttling after trying it on web and Linux. I never noticed it was laggy before but now I can't unsee it... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think the throttling was added when web's text layout was too slow. Things have changed since, so we can remove the throttling.
/// See also: | ||
/// * [TextSelectionGestureDetector], which uses this parameter to avoid excessive updates | ||
/// text layouts in text fields. | ||
Duration? dragUpdateThrottleFrequency; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is public API, which makes removing it a breaking change because some apps might be using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will need to be deprecated instead. You can start to ignore the value, but it needs to stay in the API for a while.
Check out other examples of deprecation in the code (search for "@deprecated"), and look at https://github.com/flutter/flutter/wiki/Tree-hygiene#deprecations
Be sure to update the documentation comment to make it obvious that the field is deprecated, and that it will be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware TapAndDragGestureRecognizer
could be used by other apps. If that's the case then perhaps we keep dragUpdateThrottleFrequency
and just make the text_selection.dart changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loune That sounds good to me. I think you can also remove the comments in tap_and_drag_gestures.dart related to this issue.
/// See also:
/// * [TextSelectionGestureDetector], which uses this parameter to avoid excessive updates
/// text layouts in text fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but Google testing is failing. I'll try to see what's happening there.
This is passing google testing (ran locally). Will merge once all the github tests are green. |
This needs to be synced up to master again due to some recent changes in text_selection.dart . |
…ness (flutter#123460) Remove mouse tap text drag selection throttling to improve responsiveness
Currently, drag selecting text fields with a mouse in Flutter desktop and web feels sluggish, especially compared to platform native applications. This is due to the throttling of text selection updates with the aim of reducing excessive layout. This PR removes the drag selection throttling.
Since there is already logic which stops recalculating layout if the text is unchanged, the throttling on
TextSelectionGestureDetector
seems unneccesary. Also, text selection via other means such as drag handles on Android and iOS platforms does not have the throttle logic.Here is a video of text selection before and after the PR:
text-select.mp4
Live Demo: https://flutter-apps-ll-public.s3.us-west-2.amazonaws.com/text-selection/index-frame.html
Related issue: #78908
Since this is removing code, I don't feel unit tests are neccessary but happy to be corrected.
Pre-launch Checklist
///
).