-
Notifications
You must be signed in to change notification settings - Fork 29.4k
Changes the regular cursor to a floating cursor when a long press occurs. #138479
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
Conversation
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #138479 at sha 03efdc22f780d3b9b66832332a1d7064d3a621b3 |
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 looks like a new feature to me. Could you add a bit more detail wrt how this fixes the flickering in #132825 ?
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.
nit: consider using https://main-api.flutter.dev/flutter/dart-ui/Offset/distanceSquared.html
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.
nit: maybe add a bit more context about the behavior? Something like in UITextField when floating cursor is activated the regular cursor won't be visible unless it's too far away from the regular one?
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.
nit: use ??
over if
statements?
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.
can this be incorporated into the base class instead of a separate mixin?
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, I found that it doesn't affect SelectableText
, so we can incorporate into the base class.
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.
Ideally we should avoid using the selection value from the controller. The value from the controller is potentially one frame ahead of the currently rendered selection range (and the selection range reported to the iOS text input plugin).
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.
Then we have to update the floating cursor after the frame
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.
Why does the code need to be scheduled to run as a post-frame callback?
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.
editable_text.dart#L3135C7-L3135C7 Need to get the correct selection after the frame.
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.
Shouldn't this go to line 2417? The floating cursor is iOS only right? And it only happens when you long press and drag the blinking cursor?
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.
(Same for the update/end events)
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.
A long press displays a floating cursor that does not require dragging. A long press may be further away from the regular cursor. For example, long press at the end of the text far away from the text
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.
Ah I see thanks for pointing this out. I tried this in a UITextView, my observations so far:
- When the text selection is collapsed, long press drag initiates floating cursor and when it ends the selection remains collapsed.
- When there is pre-selected text, long press drag does the same, but only when the initial long press starts outside of the selected text. And when it ends the selection becomes collapsed.
- When there is pre-selected text, and the initial long press starts inside of the selected text, the behavior has been somewhat inconsistent for me. Most of the time it enters "text drag and drop" mode, but sometimes one of the selection handlers enters the floating cursor mode and I was able to adjust the text selection in that mode.
- When the field doesn't have focus you can still long press to focus the field and start floating cursor.
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 guess I should only need to show the floating cursor when the selection is collapsed. Do we have any plans regarding "text drag and drop" mode? I'd like to contribute to this.
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.
One existing issue I found: #128388. @Renzo-Olivares have you looked into this?
I'd like to verify some floating cursor interactions in a UIKit app tomorrow and get back to this PR. Sorry for the delay! |
I also found that the cursor will be reset to the blinking state shortly after long pressing. This may also be incorrect behavior. I will look for this issue later and fix it as another PR. |
Are you still continuing the review? @LongCatIsLooong |
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.
Sorry for the delay.
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.
Shouldn't this go to line 2417? The floating cursor is iOS only right? And it only happens when you long press and drag the blinking cursor?
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.
(Same for the update/end events)
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.
It will probably be easier if we update the logic in RenderEditable.updateFloatingCursor
to update the selection immediately on FloatingCursorDragState.Start
(as all types of floating cursors seem to relocate the caret to where the floating cursor is on start?), so we don't have to do this? And we should be able to avoid the post frame callback with that.
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.
Also when I tried long pressing on ios/macos, whether the field was initially focused didn't seem to make any difference. cc @Renzo-Olivares.
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.
If we update the selection within updateFloatingCursor
, we would also need to bring SelectionChangedCause.longPress
into updateFloatingCursor
. This could potentially complicate things further.
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.
Good point. Then does it make sense to add the new text selection to the RawFloatingCursorPoint
class?
class RawFloatingCursorPoint {
// (local offset, new caret location)
// Only non-null when starting a floating cursor via long press.
final (Offset, TextPositon)? startLocation;
}
Then in editable_text.dart we can use that information in the FloatingCursorDragState.Start
clause if non-null. It's error-prone to use addPostframeCallback
in general since you don't know what is going to happen in that frame (the render object can be disposed, for example).
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.
Coooooool, we can avoid using the addPostframeCallback
method now. 🎉🎉🎉
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.
Could you add a few tests to verify the new feature works?
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.
remove late
?
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.
nit: I think it probably makes more sense to keep this in the switch
statement? If the render editable isn't focused, instead of selecting the word, we want to select the position and start the floating cursor right?
@LongCatIsLooong Hello, I noticed some issues when moving outside of the textfield, which have now been fixed. Please continue with the review. Additionally, I moved the code into a switch statement. I found that macOS does not trigger the long press gesture, so I didn't make any additional judgments. Finally, I added a golden file test. This is my first time using a golden file test, and I hope everything goes smoothly. |
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #138479 at sha ca6474ccef9ee2c31445db077a4f40d4b89af83a |
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.
Can this just be an Offset
and we can get the current value from within updateFloatingCursor
?
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 had a discussion with LongCatIsLooong, and because renderEditable.selection
updates one frame late, we can't use renderEditable.selection
for calculation.
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.
Since this is public API documentation, could you update it to make it more formal / descriptive?
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.
nit: is this empty line needed?
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.
nit: add a period at the end of this comment.
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.
nit: add a period at the end of this comment.
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.
nit: Wait for autofocus.
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.
nit: consider changing to Wait for autofocus.
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.
nit: Add a period at the end of this comment.
All nits have been fixed. |
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #138479 at sha 05a3344cf8252ba33120f6ff85c1801fe64a289e |
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
final EdgeInsets boundingRect = _calculateFloatingCursorBounds(); | ||
|
||
if (shouldResetOrigin != null) { | ||
_shouldResetOrigin = shouldResetOrigin; |
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.
nit: ??=
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.
We can't use ??=
here. When shouldResetOrigin
is not null
, the variable that needs to be assigned is _shouldResetOrigin
, they are not the same.
/// Returns the position within the text field closest to the raw cursor offset. | ||
Offset calculateBoundedFloatingCursorOffset(Offset rawCursorOffset, {bool? shouldResetOrigin}) { | ||
Offset deltaPosition = Offset.zero; | ||
final EdgeInsets boundingRect = _calculateFloatingCursorBounds(); |
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.
nit: inline the implementation?
also consider using clampDouble
.
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.
also nit: bounding rects are typically Rects.
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.
Fixed.
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.
nit: use Rect instead of EdgeInsets.
} | ||
|
||
if (!_shouldResetOrigin) { | ||
return _calculateAdjustedCursorOffset(rawCursorOffset, boundingRect); |
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.
If you adjust the cursor offset to be within boundingRect
, does a multiline text field still scroll down if you drag the cursor to the bottom of the text field?
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 see that they are working properly.
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
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 modulo nits. Thank you for contributing! The floating cursor seems to work pretty well when I tried out this patch.
/// Returns the position within the text field closest to the raw cursor offset. | ||
Offset calculateBoundedFloatingCursorOffset(Offset rawCursorOffset, {bool? shouldResetOrigin}) { | ||
Offset deltaPosition = Offset.zero; | ||
final EdgeInsets boundingRect = _calculateFloatingCursorBounds(); |
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.
nit: use Rect instead of EdgeInsets.
return EdgeInsets.fromLTRB(leftBound, topBound, rightBound, bottomBound); | ||
} | ||
|
||
Offset _calculateAdjustedCursorOffset(Offset offset, EdgeInsets boundingRect) { |
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.
nit: make this static?
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.
Since this is public API documentation, could you update it to make it more formal / descriptive?
from: details.globalPosition, | ||
cause: SelectionChangedCause.longPress, | ||
); | ||
// Show the floating cursor |
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.
nit: period at the end of sentence.
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 w/ small nit.
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
The nits have been fixed, I believe it's ready for merge. |
Manual roll Flutter from 11def8e to cc40425 (118 revisions) Manual roll requested by [email protected] flutter/flutter@11def8e...cc40425 2024-01-05 [email protected] Roll Flutter Engine from 0bbb4d61ce82 to f60d9a9a3395 (1 revision) (flutter/flutter#140993) 2024-01-04 [email protected] Roll Flutter Engine from b2a9ce88a19e to 0bbb4d61ce82 (3 revisions) (flutter/flutter#140990) 2024-01-04 [email protected] [web] Fix and unskip a few more CanvasKit tests (flutter/flutter#140821) 2024-01-04 [email protected] Roll Flutter Engine from bd175aa5e0b6 to b2a9ce88a19e (1 revision) (flutter/flutter#140986) 2024-01-04 [email protected] Pin package:vm_service (flutter/flutter#140972) 2024-01-04 [email protected] Add scrollbar for menus (flutter/flutter#140941) 2024-01-04 [email protected] manual pub roll to pick up dds fixes (flutter/flutter#140979) 2024-01-04 [email protected] Roll Flutter Engine from b81023eb71c9 to bd175aa5e0b6 (2 revisions) (flutter/flutter#140980) 2024-01-04 [email protected] Temporarily remove env variable for leak tracking bots. (flutter/flutter#140978) 2024-01-04 [email protected] Add Flutter CI status to README (flutter/flutter#140513) 2024-01-04 [email protected] Reland "integrate testWidgets with leak tracking" (#140521) (flutter/flutter#140928) 2024-01-04 [email protected] Run half of iOS devicelab tests with Xcode 15 (flutter/flutter#140927) 2024-01-04 [email protected] Fix `SegmentedButton` states update logic (flutter/flutter#140772) 2024-01-04 [email protected] Roll Flutter Engine from f539acfb8c5a to b81023eb71c9 (1 revision) (flutter/flutter#140973) 2024-01-04 [email protected] [Fix] Consistency in ButtonStyleButton related Tests (flutter/flutter#140610) 2024-01-04 [email protected] Roll Packages from bbb4134 to 31fc7b5 (6 revisions) (flutter/flutter#140967) 2024-01-04 [email protected] Fix local engine use in macOS plugins (flutter/flutter#140222) 2024-01-04 [email protected] Roll Flutter Engine from 7d5a120a601b to f539acfb8c5a (2 revisions) (flutter/flutter#140959) 2024-01-04 [email protected] Roll Flutter Engine from 1ff3cb885842 to 7d5a120a601b (1 revision) (flutter/flutter#140946) 2024-01-04 [email protected] Roll Flutter Engine from c8bf51f0d4cd to 1ff3cb885842 (1 revision) (flutter/flutter#140943) 2024-01-04 [email protected] Roll Flutter Engine from bfd2d8a100ec to c8bf51f0d4cd (2 revisions) (flutter/flutter#140939) 2024-01-04 [email protected] Roll Flutter Engine from 28ae9e35c331 to bfd2d8a100ec (1 revision) (flutter/flutter#140937) 2024-01-04 [email protected] Roll Flutter Engine from ab4098c742f8 to 28ae9e35c331 (1 revision) (flutter/flutter#140936) 2024-01-04 [email protected] Roll Flutter Engine from e169f3677008 to ab4098c742f8 (2 revisions) (flutter/flutter#140933) 2024-01-03 [email protected] Roll Flutter Engine from 7c2adb811059 to e169f3677008 (1 revision) (flutter/flutter#140929) 2024-01-03 [email protected] Remove deprecated bitcode stripping from tooling (flutter/flutter#140903) 2024-01-03 [email protected] Add Windows leak tracking targets (flutter/flutter#140423) 2024-01-03 [email protected] [github actions] refactor and fix cherry pick actions (flutter/flutter#140499) 2024-01-03 [email protected] Migrate Xcode projects last version checks to Xcode 15.1 (flutter/flutter#140256) 2024-01-03 [email protected] Roll Flutter Engine from bf232c4da241 to 7c2adb811059 (3 revisions) (flutter/flutter#140920) 2024-01-03 [email protected] fix typo and reflow (flutter/flutter#140925) 2024-01-03 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Re-land integrate testWidgets with leak tracking." (flutter/flutter#140926) 2024-01-03 [email protected] Roll Flutter Engine from bf979d220283 to bf232c4da241 (1 revision) (flutter/flutter#140915) 2024-01-03 [email protected] Changes the regular cursor to a floating cursor when a long press occurs. (flutter/flutter#138479) 2024-01-03 [email protected] Add `SegmentedButton.styleFrom` (flutter/flutter#137542) 2024-01-03 [email protected] Roll Flutter Engine from c62bcff5b809 to bf979d220283 (1 revision) (flutter/flutter#140910) 2024-01-03 [email protected] Re-land integrate testWidgets with leak tracking. (flutter/flutter#140521) 2024-01-03 [email protected] Roll Flutter Engine from 98b72c7ffe71 to c62bcff5b809 (2 revisions) (flutter/flutter#140905) 2024-01-03 [email protected] [flutter_tools] add support for --enable-impeller to test device. (flutter/flutter#140899) 2024-01-03 [email protected] Roll Flutter Engine from cf7536964a2f to 98b72c7ffe71 (1 revision) (flutter/flutter#140897) 2024-01-03 [email protected] Handle KEYCODE_DPAD_CENTER and KEYCODE_ENTER (flutter/flutter#140808) 2024-01-03 [email protected] Add Lucas Saudon to AUTHORS (flutter/flutter#139965) 2024-01-03 [email protected] Link to wiki page about updating dependencies in each `pubspec.yaml` file (flutter/flutter#140826) 2024-01-03 [email protected] Marks Linux_pixel_7pro native_assets_android to be unflaky (flutter/flutter#140866) ... --------- Co-authored-by: Tarrin Neal <[email protected]>
This PR changes the regular cursor to a floating cursor when a long press occurs.
This is a new feature. Fixes #89228
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.