Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix crash with CJK keyboard with emoji at end of text field #42540

Merged
merged 4 commits into from
Jun 12, 2023
Merged

Fix crash with CJK keyboard with emoji at end of text field #42540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52d4f25
Remove isRTLAtPosition
moffatman Jun 3, 2023
52d7edf
Add test for partial rects
moffatman Jun 3, 2023
17c1a8a
Fix comment merge
moffatman Jun 3, 2023
d6df36a
Fix formatting
moffatman Jun 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 6 additions & 22 deletions shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1658,25 +1658,6 @@ - (CGRect)firstRectForRange:(UITextRange*)range {
return CGRectZero;
}

- (BOOL)isRTLAtPosition:(NSUInteger)position {
// _selectionRects is sorted by position already.
// We can use binary search.
NSInteger min = 0;
NSInteger max = [_selectionRects count];
while (min <= max) {
const NSUInteger mid = min + (max - min) / 2;
FlutterTextSelectionRect* rect = _selectionRects[mid];
if (rect.position > position) {
max = mid - 1;
} else if (rect.position == position) {
return rect.isRTL;
} else {
min = mid + 1;
}
}
return NO;
}

- (CGRect)caretRectForPosition:(UITextPosition*)position {
NSInteger index = ((FlutterTextPosition*)position).index;
UITextStorageDirection affinity = ((FlutterTextPosition*)position).affinity;
Expand All @@ -1699,7 +1680,8 @@ - (CGRect)caretRectForPosition:(UITextPosition*)position {
CGRect characterAfterCaret = rects[0].rect;
// Return a zero-width rectangle along the upstream edge of the character after the caret
// position.
if ([self isRTLAtPosition:index]) {
if ([rects[0] isKindOfClass:[FlutterTextSelectionRect class]] &&
Copy link
Contributor

Choose a reason for hiding this comment

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

The old implementation doesn't seem to have this check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you might have to special case it when characterAfterCaret is a newline character, iirc SkParagraph reports it as an empty textbox at the end of the current line (instead of at the beginning of the next line).

Copy link
Contributor Author

@moffatman moffatman Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here we use selectionRectsForRange instead of directly looking at the array. I put a check, because I guess if someone subclasses and reimplemented selectionRectsForRange, this would crash if it didn't return specifically FlutterTextSelectionRect. Not sure if this is really a concern...

Not sure what you mean about the special case. But for end-of-line, it does work correctly because I have added affinity into the FlutterTextPosition. So it will go to the characterBeforeCaret section.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the document is "{newline}A" (so 2 characters), placing the cursor at (1, upstream) should point to the start of the second line. I think it would fall into the "remaining cases" category (around line 1718) and returns

    return CGRectMake(characterBeforeCaret.origin.x + characterBeforeCaret.size.width,
                      characterBeforeCaret.origin.y, 0, characterBeforeCaret.size.height);

which is going to be the right edge of the zero-width TextBox skparagraph returns for the newline character at the first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand upstream/downstream, it only comes into play where you have word wrap breaking lines. When you have a hard line-break, we have distinct indices at the end of line 1 and the beginning of line 2. And so most of the time the affinity is downstream, exccept at the end of a word-wrapped line.

I tried out the "\nA" scenario and it seems to work as expected in all 3 possible cursor locations. In the specific one you highlighted (cursor at index 1), it used the middle branch (rects.count == 2 && affinity == UITextStorageDirectionForward). I think this isn't a concern as you will never actually get that affinity (1, upstream) using either keyboard or touch?

Copy link
Contributor

Choose a reason for hiding this comment

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

you will never actually get that affinity (1, upstream) using either keyboard or touch?

Ah that's probably true.

But I think newlines are still special (sorry my example was bad). I tried getting the text boxes for "A\n":

    final TextPainter painter = TextPainter(
      textDirection: TextDirection.ltr,
      text: const TextSpan(text: 'A\n'),
    )..layout();
    for (int i = 0; i < 2; i++) {
      print(painter.getBoxesForSelection(TextSelection(baseOffset: i, extentOffset: i + 1)));
    }

the output:

[TextBox.fromLTRBD(0.0, 0.0, 14.0, 14.0, TextDirection.ltr)]
[TextBox.fromLTRBD(14.0, 0.0, 14.0, 14.0, TextDirection.ltr)]

when index == 1 and the affinity is UITextStorageDirectionForward this should place the cursor at the start of the second line. But if you look at the output both TextBoxes are on the first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I get it now. But I don't know if we can fix that here, if we don't even have any text box on the second line, we can't even make a guess of the correct position? It needs to be fixed upstream in skparagraph. And there isn't any regression in this PR. I hope we can ignore this, and merge it to avoid the hard crash.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. Probably not a SkParagraph issue I think, we should probably add a bit more information to rects the framework sends.

((FlutterTextSelectionRect*)rects[0]).isRTL) {
return CGRectMake(characterAfterCaret.origin.x + characterAfterCaret.size.width,
characterAfterCaret.origin.y, 0, characterAfterCaret.size.height);
} else {
Expand All @@ -1712,7 +1694,8 @@ - (CGRect)caretRectForPosition:(UITextPosition*)position {
CGRect characterAfterCaret = rects[1].rect;
// Return a zero-width rectangle along the upstream edge of the character after the caret
// position.
if ([self isRTLAtPosition:index]) {
if ([rects[1] isKindOfClass:[FlutterTextSelectionRect class]] &&
((FlutterTextSelectionRect*)rects[1]).isRTL) {
return CGRectMake(characterAfterCaret.origin.x + characterAfterCaret.size.width,
characterAfterCaret.origin.y, 0, characterAfterCaret.size.height);
} else {
Expand All @@ -1727,7 +1710,8 @@ - (CGRect)caretRectForPosition:(UITextPosition*)position {
// For both cases, return a zero-width rectangle along the downstream edge of the character
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we only send the rects for visible characters in a multiline text field, so technically this also includes the case when the selection reaches the end of the visible text? Would this prevent the user from scrolling the text field using force touch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This API is used by UIKit to figure out where to initially position its internal tracking of the selection gesture. So it would only be called on visible characters. And the logic still holds even if there are characters after current position that don't have a rect. As long as we have the 1 visible character before the cursor.
And yeah, to implement full scrolling we would need to send rects for all characters. A matter for another PR in framework.

// before the caret position.
CGRect characterBeforeCaret = rects[0].rect;
if ([self isRTLAtPosition:index - 1]) {
if ([rects[0] isKindOfClass:[FlutterTextSelectionRect class]] &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can assert the class type here?

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 you mean. I am checking the class type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like:

FlutterTextSelectionRect *rect = (FlutterTextSelectionRect *)rects[0];
if (rect...)

Because the rect must be the correct type right? There is no way isKindOfClass returns false, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically it's possible, this method signature is not FlutterTextSelectionRect*, but UITextSelectionRect*.

((FlutterTextSelectionRect*)rects[0]).isRTL) {
return CGRectMake(characterBeforeCaret.origin.x, characterBeforeCaret.origin.y, 0,
characterBeforeCaret.size.height);
} else {
Expand Down
23 changes: 23 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1535,6 +1535,29 @@ - (void)testClosestPositionToPointWithinRange {
((FlutterTextPosition*)[inputView closestPositionToPoint:point withinRange:range]).affinity);
}

- (void)testClosestPositionToPointWithPartialSelectionRects {
FlutterTextInputView* inputView = [[FlutterTextInputView alloc] initWithOwner:textInputPlugin];
[inputView setTextInputState:@{@"text" : @"COMPOSING"}];

[inputView setSelectionRects:@[ [FlutterTextSelectionRect
selectionRectWithRect:CGRectMake(0, 0, 100, 100)
position:0U] ]];
// Asking with a position at the end of selection rects should give you the trailing edge of
// the last rect.
XCTAssertTrue(CGRectEqualToRect(
[inputView caretRectForPosition:[FlutterTextPosition
positionWithIndex:1
affinity:UITextStorageDirectionForward]],
CGRectMake(100, 0, 0, 100)));
// Asking with a position beyond the end of selection rects should return CGRectZero without
// crashing.
XCTAssertTrue(CGRectEqualToRect(
[inputView caretRectForPosition:[FlutterTextPosition
positionWithIndex:2
affinity:UITextStorageDirectionForward]],
CGRectZero));
}

#pragma mark - Floating Cursor - Tests

- (void)testFloatingCursorDoesNotThrow {
Expand Down