Skip to content

Adds support for floating cursor - #24761

Merged
jslavitz merged 28 commits into
flutter:masterfrom
jslavitz:force-cursor
Dec 14, 2018
Merged

jslavitz merged 28 commits into
flutter:masterfrom
jslavitz:force-cursor

Conversation

@jslavitz

@jslavitz jslavitz commented Nov 27, 2018 •

Copy link
Copy Markdown
Contributor

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

saurik commented Nov 27, 2018 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

saurik commented Nov 27, 2018 •

Copy link
Copy Markdown
Contributor

@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

jslavitz commented Nov 27, 2018 •

Copy link
Copy Markdown
Contributor Author

@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
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
@xster

xster commented Nov 27, 2018

Copy link
Copy Markdown
Member

@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
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
Comment thread packages/flutter/lib/src/services/text_input.dart Outdated
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

mikezs commented Nov 30, 2018

Copy link
Copy Markdown

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

jslavitz commented Nov 30, 2018 •

Copy link
Copy Markdown
Contributor Author

@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-team-archive/engine#6945) if you want to test this.

@mikezs

mikezs commented Nov 30, 2018 •

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

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

@jslavitz

Copy link
Copy Markdown
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!

@xster xster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
}

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

Copy link
Copy Markdown
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.

Comment thread packages/flutter/lib/src/services/text_input.dart
Comment thread packages/flutter/lib/src/services/text_input.dart Outdated
Comment thread packages/flutter/lib/src/services/text_input.dart Outdated
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
Comment thread packages/flutter/lib/src/rendering/editable.dart Outdated
@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