-
Notifications
You must be signed in to change notification settings - Fork 6k
[iOSTextInputPlugin] bypass UIKit floating cursor coordinates clamping #26486
Changes from all commits
761aebc
d19cb61
39e8eaa
b13caac
fa1e498
659e74a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,18 @@ | |
// returns kInvalidFirstRect, iOS will not show the IME candidates view. | ||
const CGRect kInvalidFirstRect = {{-1, -1}, {9999, 9999}}; | ||
|
||
// The `bounds` value a FlutterTextInputView returns when the floating cursor | ||
// is activated in that view. | ||
// | ||
// DO NOT use extremely large values (such as CGFloat_MAX) in this rect, for that | ||
// will significantly reduce the precision of the floating cursor's coordinates. | ||
// | ||
// It is recommended for this CGRect to be roughly centered at caretRectForPosition | ||
// (which currently always return CGRectZero), so the initial floating cursor will | ||
// be placed at (0, 0). | ||
// See the comments in beginFloatingCursorAtPoint and caretRectForPosition. | ||
const CGRect kSpacePanBounds = {{-2500, -2500}, {5000, 5000}}; | ||
|
||
#pragma mark - TextInputConfiguration Field Names | ||
static NSString* const kSecureTextEntry = @"obscureText"; | ||
static NSString* const kKeyboardType = @"inputType"; | ||
|
@@ -505,6 +517,7 @@ @implementation FlutterTextInputView { | |
const char* _selectionAffinity; | ||
FlutterTextRange* _selectedTextRange; | ||
CGRect _cachedFirstRect; | ||
bool _isFloatingCursorActive; | ||
// The view has reached end of life, and is no longer | ||
// allowed to access its textInputDelegate. | ||
BOOL _decommissioned; | ||
|
@@ -527,6 +540,7 @@ - (instancetype)init { | |
// Initialize with the zero matrix which is not | ||
// an affine transform. | ||
_editableTransform = CATransform3D(); | ||
_isFloatingCursorActive = false; | ||
|
||
// UITextInputTraits | ||
_autocapitalizationType = UITextAutocapitalizationTypeSentences; | ||
|
@@ -543,6 +557,16 @@ - (instancetype)init { | |
_smartQuotesType = UITextSmartQuotesTypeYes; | ||
_smartDashesType = UITextSmartDashesTypeYes; | ||
} | ||
|
||
// This makes sure UITextSelectionView.interactionAssistant is not nil so | ||
// UITextSelectionView has access to this view (and its bounds). Otherwise | ||
// floating cursor breaks: https://github.com/flutter/flutter/issues/70267. | ||
if (@available(iOS 13.0, *)) { | ||
UITextInteraction* interaction = | ||
[UITextInteraction textInteractionForMode:UITextInteractionModeEditable]; | ||
interaction.textInput = self; | ||
[self addInteraction:interaction]; | ||
} | ||
} | ||
|
||
return self; | ||
|
@@ -612,9 +636,9 @@ - (UITextContentType)textContentType { | |
// from the view hierarchy) so that it may outlive the plugin/engine, | ||
// in which case _textInputDelegate will become a dangling pointer. | ||
|
||
// The text input plugin needs to call decommision when it should | ||
// The text input plugin needs to call decommission when it should | ||
// not have access to its FlutterTextInputDelegate any more. | ||
- (void)decommision { | ||
- (void)decommission { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😁 |
||
_decommissioned = YES; | ||
} | ||
|
||
|
@@ -1094,9 +1118,23 @@ - (CGRect)firstRectForRange:(UITextRange*)range { | |
|
||
- (CGRect)caretRectForPosition:(UITextPosition*)position { | ||
// TODO(cbracken) Implement. | ||
|
||
// As of iOS 14.4, this call is used by iOS's | ||
// _UIKeyboardTextSelectionController to determine the position | ||
// of the floating cursor when the user force touches the space | ||
// bar to initiate floating cursor. | ||
// | ||
// It is recommended to return a value that's roughly the | ||
// center of kSpacePanBounds to make sure the floating cursor | ||
// has ample space in all directions and does not hit kSpacePanBounds. | ||
// See the comments in beginFloatingCursorAtPoint. | ||
return CGRectZero; | ||
} | ||
|
||
- (CGRect)bounds { | ||
return _isFloatingCursorActive ? kSpacePanBounds : super.bounds; | ||
} | ||
|
||
- (UITextPosition*)closestPositionToPoint:(CGPoint)point { | ||
// TODO(cbracken) Implement. | ||
NSUInteger currentIndex = ((FlutterTextPosition*)_selectedTextRange.start).index; | ||
|
@@ -1120,18 +1158,47 @@ - (UITextRange*)characterRangeAtPoint:(CGPoint)point { | |
} | ||
|
||
- (void)beginFloatingCursorAtPoint:(CGPoint)point { | ||
// For "beginFloatingCursorAtPoint" and "updateFloatingCursorAtPoint", "point" is roughly: | ||
// | ||
// CGPoint( | ||
// width >= 0 ? point.x.clamp(boundingBox.left, boundingBox.right) : point.x, | ||
// height >= 0 ? point.y.clamp(boundingBox.top, boundingBox.bottom) : point.y, | ||
// ) | ||
// where | ||
// point = keyboardPanGestureRecognizer.translationInView(textInputView) + | ||
// caretRectForPosition boundingBox = self.convertRect(bounds, fromView:textInputView) bounds | ||
// = self._selectionClipRect ?? self.bounds | ||
// | ||
// It's tricky to provide accurate "bounds" and "caretRectForPosition" so it's preferred to bypass | ||
// the clamping and implement the same clamping logic in the framework where we have easy access | ||
// to the bounding box of the input field and the caret location. | ||
// | ||
// The current implementation returns kSpacePanBounds for "bounds" when "_isFloatingCursorActive" | ||
// is true. kSpacePanBounds centers "caretRectForPosition" so the floating cursor has enough | ||
// clearance in all directions to move around. | ||
// | ||
// It seems impossible to use a negative "width" or "height", as the "convertRect" | ||
// call always turns a CGRect's negative dimensions into non-negative values, e.g., | ||
// (1, 2, -3, -4) would become (-2, -2, 3, 4). | ||
NSAssert(!_isFloatingCursorActive, @"Another floating cursor is currently active."); | ||
_isFloatingCursorActive = true; | ||
[self.textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateStart | ||
withClient:_textInputClient | ||
withPosition:@{@"X" : @(point.x), @"Y" : @(point.y)}]; | ||
} | ||
|
||
- (void)updateFloatingCursorAtPoint:(CGPoint)point { | ||
NSAssert(_isFloatingCursorActive, | ||
@"updateFloatingCursorAtPoint is called without an active floating cursor."); | ||
[self.textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateUpdate | ||
withClient:_textInputClient | ||
withPosition:@{@"X" : @(point.x), @"Y" : @(point.y)}]; | ||
} | ||
|
||
- (void)endFloatingCursor { | ||
NSAssert(_isFloatingCursorActive, | ||
@"endFloatingCursor is called without an active floating cursor."); | ||
_isFloatingCursorActive = false; | ||
[self.textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateEnd | ||
withClient:_textInputClient | ||
withPosition:@{@"X" : @(0), @"Y" : @(0)}]; | ||
|
@@ -1410,6 +1477,7 @@ - (void)hideTextInput { | |
[self removeEnableFlutterTextInputViewAccessibilityTimer]; | ||
_activeView.accessibilityEnabled = NO; | ||
[_activeView resignFirstResponder]; | ||
[_activeView decommission]; | ||
[_activeView removeFromSuperview]; | ||
[_inputHider removeFromSuperview]; | ||
} | ||
|
@@ -1572,10 +1640,10 @@ - (UIView*)keyWindow { | |
return _inputHider.subviews; | ||
} | ||
|
||
// Decommisions (See the "decommision" method on FlutterTextInputView) and removes | ||
// Decommissions (See the "decommission" method on FlutterTextInputView) and removes | ||
// every installed input field, unless it's in the current autofill context. | ||
// | ||
// The active view will be decommisioned and removed from its superview too, if | ||
// The active view will be decommissioned and removed from its superview too, if | ||
// includeActiveView is YES. | ||
// When clearText is YES, the text on the input fields will be set to empty before | ||
// they are removed from the view hierarchy, to avoid triggering autofill save. | ||
|
@@ -1595,7 +1663,7 @@ - (void)cleanUpViewHierarchy:(BOOL)includeActiveView | |
if (clearText) { | ||
[inputView replaceRangeLocal:NSMakeRange(0, inputView.text.length) withText:@""]; | ||
} | ||
[inputView decommision]; | ||
[inputView decommission]; | ||
if (delayRemoval) { | ||
[inputView performSelector:@selector(removeFromSuperview) withObject:nil afterDelay:0.1]; | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ - (void)setEditableTransform:(NSArray*)matrix; | |
- (void)setTextInputState:(NSDictionary*)state; | ||
- (void)setMarkedRect:(CGRect)markedRect; | ||
- (void)updateEditingState; | ||
- (void)decommisson; | ||
- (BOOL)isVisibleToAutofill; | ||
|
||
@end | ||
|
@@ -52,7 +51,6 @@ @interface FlutterSecureTextInputView : FlutterTextInputView | |
@end | ||
|
||
@interface FlutterTextInputPlugin () | ||
@property(nonatomic, strong) FlutterTextInputView* reusableInputView; | ||
@property(nonatomic, assign) FlutterTextInputView* activeView; | ||
@property(nonatomic, readonly) | ||
NSMutableDictionary<NSString*, FlutterTextInputView*>* autofillContext; | ||
|
@@ -82,10 +80,11 @@ - (void)setUp { | |
} | ||
|
||
- (void)tearDown { | ||
[engine stopMocking]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line caused all the tests in the file to fail. Did we upgrade ocmock or something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope we didn't. You shouldn't remove this line. The tests are running as part of CI so we know this works before the changes here. Let me know what errors you are getting and I can try to help. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it back it starts failing again: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8845604182114825824/+/u/Host_Tests_for_ios_debug_sim/stdout
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means you are leaking an object beyond your test that is talking to the mock engine, probably a view controller. Let me look at your test code to see if I can spot it. (this can cause other tests to fail beyond yours) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like your test is fine. One of 2 things are happening:
Here's an example where I had to fix similar issues: I'd try running that test / writing one similar to make sure that the view controller is being deleted. If you run that one test and it fails, you'll know you introduced a leak. Otherwise tracking down the timing issue will take a bit more work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out UIKit is keeping some of the input views alive and modifying the content of these views even they're removed from the view hierarchy. #26547 |
||
[textInputPlugin.autofillContext removeAllObjects]; | ||
[textInputPlugin cleanUpViewHierarchy:YES clearText:YES delayRemoval:NO]; | ||
[[[[textInputPlugin textInputView] superview] subviews] | ||
makeObjectsPerformSelector:@selector(removeFromSuperview)]; | ||
|
||
[engine stopMocking]; | ||
[super tearDown]; | ||
} | ||
|
||
|
@@ -529,6 +528,53 @@ - (void)testUpdateFirstRectForRange { | |
XCTAssertTrue(CGRectEqualToRect(kInvalidFirstRect, [inputView firstRectForRange:range])); | ||
} | ||
|
||
#pragma mark - Floating Cursor - Tests | ||
|
||
- (void)testInputViewsHaveUIInteractions { | ||
if (@available(iOS 13.0, *)) { | ||
FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; | ||
XCTAssertGreaterThan(inputView.interactions.count, 0); | ||
} | ||
} | ||
|
||
- (void)testBoundsForFloatingCursor { | ||
FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; | ||
|
||
CGRect initialBounds = inputView.bounds; | ||
// Make sure the initial bounds.size is not as large. | ||
XCTAssertLessThan(inputView.bounds.size.width, 100); | ||
XCTAssertLessThan(inputView.bounds.size.height, 100); | ||
|
||
[inputView beginFloatingCursorAtPoint:CGPointMake(123, 321)]; | ||
CGRect bounds = inputView.bounds; | ||
XCTAssertGreaterThan(bounds.size.width, 1000); | ||
XCTAssertGreaterThan(bounds.size.height, 1000); | ||
|
||
// Verify the caret is centered. | ||
XCTAssertEqual( | ||
CGRectGetMidX(bounds), | ||
CGRectGetMidX([inputView caretRectForPosition:[FlutterTextPosition positionWithIndex:1235]])); | ||
XCTAssertEqual( | ||
CGRectGetMidY(bounds), | ||
CGRectGetMidY([inputView caretRectForPosition:[FlutterTextPosition positionWithIndex:4567]])); | ||
|
||
[inputView updateFloatingCursorAtPoint:CGPointMake(456, 654)]; | ||
bounds = inputView.bounds; | ||
XCTAssertGreaterThan(bounds.size.width, 1000); | ||
XCTAssertGreaterThan(bounds.size.height, 1000); | ||
|
||
// Verify the caret is centered. | ||
XCTAssertEqual( | ||
CGRectGetMidX(bounds), | ||
CGRectGetMidX([inputView caretRectForPosition:[FlutterTextPosition positionWithIndex:21]])); | ||
XCTAssertEqual( | ||
CGRectGetMidY(bounds), | ||
CGRectGetMidY([inputView caretRectForPosition:[FlutterTextPosition positionWithIndex:42]])); | ||
|
||
[inputView endFloatingCursor]; | ||
XCTAssertTrue(CGRectEqualToRect(initialBounds, inputView.bounds)); | ||
} | ||
|
||
#pragma mark - Autofill - Utilities | ||
|
||
- (NSMutableDictionary*)mutablePasswordTemplateCopy { | ||
|
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 tested on an iOS 12 simulator and couldn't reproduce the issue.