Skip to content

Conversation

jslavitz
Copy link
Contributor

@jslavitz jslavitz commented Nov 27, 2018

Adds framework support for the floating cursor for text editing on iOS. It can be triggered by either a force press on the keyboard or (on iOS 12) by long pressing the spacebar. Fixes #17030 (#5445).

@saurik
Copy link
Contributor

saurik commented Nov 27, 2018

You mentioned #20693, which to me is a fundamentally more serious issue than #17030, but your various descriptions only seem to fix the latter; does this in fact fix the issue in the former as well? (The latter issue your description states you have fixed involves a relatively rarely-used feature involving force touch on a keyboard that was only introduced in recent versions of iOS, while the former issue that you mention is entirely different: it involves the primary method of moving the cursor which has existed forever, I believe back well into when iOS was called iPhoneOS ;P--where a tap and hold on text lets you move the cursor--which supposedly is causing the text to be selected in Flutter.) (cc: @tvolkert)

@jonahwilliams
Copy link
Contributor

@saurik If you have concerns or more details for the bugs please note them in the respective issues. This isn't the correct place for a discussion.

@saurik
Copy link
Contributor

saurik commented Nov 27, 2018

@jonahwilliams This patch says it "Fixes #17030 (#20693, #5445)". I am asking a question about this patch (as I am asking if it actually fixes #20693, which is a very different issue from #17030; I thereby explain the two issues to make it clearer).

@jslavitz
Copy link
Contributor Author

jslavitz commented Nov 27, 2018

@saurik With this patch, you can move the cursor while dragging in a text field while in the force touch mode, but upon closer inspection, it looks like that bug is for non force touch based dragging? If that is the case it doesn't fix that bug.

@tvolkert tvolkert requested review from goderbauer and xster November 27, 2018 16:39
@xster
Copy link
Member

xster commented Nov 27, 2018

@saurik: no, it's unrelated to #20693 (and #9507). We'll fix those in a few weeks.

@zoechi zoechi added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Nov 28, 2018
@@ -701,6 +706,23 @@ class RenderEditable extends RenderBox {
markNeedsPaint();
}

/// The padding applied to text field. Used to determine the bounds when
/// moving the floating cursor.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this is necessary from reading this. Is it not used for non-floating cursor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So on iOS, the floating cursor can be rendered right up the edge of the text field on the top and bottom, and half way to the edge on the right and left. This is not used for a non floating cursor. I'd by happy to show you where it takes function tomorrow!

Copy link
Member

Choose a reason for hiding this comment

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

I see now. Maybe it's just a name tweak. Maybe floatingCursorAddedMargin or floatingCursorRectExpansion or some such.

}

void _paintFloatingCaret(Canvas canvas, Offset effectiveOffset) {
assert(_textLayoutLastWidth == constraints.maxWidth);
Copy link
Member

Choose a reason for hiding this comment

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

assert the state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what state you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. Perhaps check _floatingCursorOn then. In general, the more instance variables there are that needs manual resetting, better it is to assert all assumptions and make things fail as early as possible if things aren't in the right state.

@mikezs
Copy link

mikezs commented Nov 30, 2018

I've just checked this out (the engine requires some typos fixing to compile/run) and given it a good test and it's not quite working properly, the drawing of the "floating" cursor and caret is working find until the "end" event.

Expected:
The "floating" cursor should animate to the current caret position and replace it, continuing to blink and act as normal

Actual:
The "floating" cursor stays in its last position (looks like it's stuck), the caret also draws in its last position, even if the caret is manually changed by tapping. It does however move when characters are inserted before it.

@jslavitz
Copy link
Contributor Author

jslavitz commented Nov 30, 2018

@mikezs . You need to check out the engine commit as well, and run with --local-engine to make this work. Also, I changed some of the names around in the most recent commit and have not yet tested, so check out #e0b973b in flutter/flutter and #954e796 in flutter/engine (flutter/engine#6945) if you want to test this.

@mikezs
Copy link

mikezs commented Nov 30, 2018

I have to compile the engine to test it because of the message channels passing the state. I assume it wouldn't work at all without doing that.

I've checked out the other commits and recompiled for iOS, the host and flutter library and the only difference is the animating back to the original position. Looking pretty good!

@jslavitz
Copy link
Contributor Author

@mikezs Glad you got it working, and you're right about the animation! Thanks for catching that!

@jslavitz
Copy link
Contributor Author

@mikezs Things are working again in the most recent commit, and I just pushed a change which has the animation if you want that as well!

Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -701,6 +706,23 @@ class RenderEditable extends RenderBox {
markNeedsPaint();
}

/// The padding applied to text field. Used to determine the bounds when
/// moving the floating cursor.
Copy link
Member

Choose a reason for hiding this comment

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

I see now. Maybe it's just a name tweak. Maybe floatingCursorAddedMargin or floatingCursorRectExpansion or some such.

@jslavitz jslavitz merged commit 46878d8 into flutter:master Dec 14, 2018
jonahwilliams pushed a commit that referenced this pull request Dec 14, 2018
jonahwilliams pushed a commit that referenced this pull request Dec 14, 2018
jonahwilliams pushed a commit that referenced this pull request Dec 14, 2018
jonahwilliams pushed a commit that referenced this pull request Dec 14, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support keyboard cursor move on iOS
8 participants