[ios][iOS 17] fix auto-correction highlight on top left corner

[hellohuanlin]

Fix native auto-correction highlight region on top left corner

I did some github archaeology - some code changes in firstRectForRange are related to this PR (CC: @LongCatIsLooong). I have verified that it still works.

Some investigation:

1. In iOS 17, the size of the highlight region matches the size of the prefix of words to be auto-corrected, except for the last character. For example, in the following 2 screenshot, the width of letter i and w are different, but this observation still holds. This is likely due to internal implementation of UIKit, and it's hard to guess why exactly Apple does this.

2. The highlight region is drawn with respect to the coordinate of our FlutterTextInputView. However, it's not a subview, so likely a draw call on its CALayer.

3. As discussed in this PR, Apple does not provide any API to tell us when a word should be auto-corrected or not. This means under current API, it is unlikely possible to leverage the system highlight to replace the our custom highlight. We have to find other solutions. As a workaround, I put the FlutterTextInputView outside of the screen.

4. This happens on previous iOS as well, but just not as noticeable (much smaller, see screenshot below). So we should have this workaround for both iOS 16 and iOS 17.

5. The same workaround also fixes another issue that shows the native auto-correction context menu (bug(ios): Misaligned Spelling Suggestion When Predictive Is Off In Settings flutter#130170 and [engine][spell_check] iOS 17 shows native text editing UI flutter#130818). But it only happens on iOS 17, and not iOS 16. In the future it's worth investigating whether we can leverage this native menu (tho at that time we still have to solve the highlight region problem).

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Fixes flutter/flutter#131695
Fixes flutter/flutter#130818

[hellohuanlin]

@LongCatIsLooong I'm a bit iffy on this one and could use another pair of eyes:

My understanding of your previous PR is that this should return rects in FlutterTextInputView's coord space - not only valid rects, but also this dummy invalid rect (because otherwise an invalid rect could very well become a valid rect with some transforms of FlutterTextInputView).

Let me know if you wanna discuss offline.

[LongCatIsLooong]

Yeah I think kInvalidFirstRect is probably specified in the window's coordinate system so it needs to be transformed. The input view used to be placed at the top left corner of the window iirc.

[hellohuanlin]

Wanna highlight the changes to this test case. Let me know if it makes sense @LongCatIsLooong

[stuartmorgan-g]

I'm still unclear on why we can't make this work correctly. If the size is exactly correct, it's clearly based on rects that we are providing; why can't we ensure that we are providing correct location in addition to size so that the highlight actually works?

[hellohuanlin]

I'm still unclear on why we can't make this work correctly. If the size is exactly correct, it's clearly based on rects that we are providing; why can't we ensure that we are providing correct location in addition to size so that the highlight actually works?

@stuartmorgan Because the size is 1 character shorter, and there's no reliable API to tell whether the highlight is shown or not (If there were such API, we would be able to maybe manually highlight the last character).

[stuartmorgan-g]

Leaving aside the issue of it missing the last character: do we know why the rect is in completely the wrong place, and can we fix that? Are we providing bogus offsets in our implementation of the query method?

Then for the last-character issue, have we made a reduced test case and filled an issue with Apple?

[hellohuanlin]

Leaving aside the issue of it missing the last character: do we know why the rect is in completely the wrong place, and can we fix that? Are we providing bogus offsets in our implementation of the query method?

To clarify, our implementation of the query method (firstRectForRange) is providing the correct rects - in iOS 17 it queries letter by letter (including the last letter), and I have manually measured the frame of every letter in a screenshot and they are all correct. The reason it is placed at the wrong place is because we put the FlutterTextInputView on top left corner.

The reason I chose to hide it in this PR is because the size doesn't match (missing 1 char). Other than that, we can easily position the highlight at the right place (pretty much the same logic as positioning it outside screen in this PR).

Then for the last-character issue, have we made a reduced test case and filled an issue with Apple?

I have not filed a radar with Apple. It's a good idea.

[hellohuanlin]

The reason I chose to hide it in this PR is because the size doesn't match (missing 1 char). Other than that, we can easily position the highlight at the right place (pretty much the same logic as positioning it outside screen in this PR).

@stuartmorgan Actually, while I was writing this, it came to my mind that it might be better UX to show a slightly smaller highlight region, than not showing it at all. Let me try and see how it looks.

[stuartmorgan-g]

To clarify, our implementation of the query method (firstRectForRange) is providing the correct rects [...] The reason it is placed at the wrong place is because we put the FlutterTextInputView on top left corner.

If the view that we are telling UIKit the words are relative to is in the top left corner, and we're returning rects that don't have the correct offset for the word relative to the top left corner of the Flutter view, then we aren't returning the correct rects. Am I misunderstanding here?

[hellohuanlin]

If the view that we are telling UIKit the words are relative to is in the top left corner, and we're returning rects that don't have the correct offset for the word relative to the top left corner of the Flutter view, then we aren't returning the correct rects. Am I misunderstanding here?

Oh the API should return the rects wrt the coordinate space of UITextInput itself, so I think we did it right.

Actually, while I was writing this, it came to my mind that it might be better UX to show a slightly smaller highlight region, than not showing it at all. Let me try and see how it looks.

I take back my word - Just tried it, looks pretty bad:

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-11.at.09.54.50.mp4

[stuartmorgan-g]

If the view that we are telling UIKit the words are relative to is in the top left corner, and we're returning rects that don't have the correct offset for the word relative to the top left corner of the Flutter view, then we aren't returning the correct rects. Am I misunderstanding here?

Oh the API should return the rects wrt the coordinate space of UITextInput itself, so I think we did it right.

I'm not following. You're saying that we are returning the actual offsets of the queried text, relative to the upper-left corner of the Flutter view (where the UITextInput is positioned), and yet UIKit is drawing them in a random location instead?

[hellohuanlin]

I'm not following. You're saying that we are returning the actual offsets of the queried text, relative to the upper-left corner of the Flutter view (where the UITextInput is positioned), and yet UIKit is drawing them in a random location instead?

We are returning the offset of queried text, relative to the upper-left corner of UITextInput, which happens to be positioned at the origin of FlutterView.

UIKit is drawing the highlight relative to UITextInput correctly (not randomly). So in this PR, I moved this UITextInput outside screen.

[stuartmorgan-g]

We are returning the offset of queried text, relative to the upper-left corner of UITextInput, which happens to be positioned at the origin of FlutterView.

UIKit is drawing the highlight relative to UITextInput (not randomly).

I do not understand how both of these things can possibly be true.

If we were returning the actual offset of the words relative to the origin of the Flutter view (where the UITextInput is), and UIKit were drawing the rect using that same offset and origin, then the rectangle would be in the correct location. It's not, so either one of those things is not true, or something else is wrong that I still don't understand.

[stuartmorgan-g]

Looking at the code here more closely, I see that the beginning of firstRectForRange: is assertions about the class types of the inputs being our own classes. That means that the values UIKit is passing us to query the rect are values we provided from some other part of the text input protocol. Given that, do we actually know that the missing last character is an iOS bug, and not a bug in our own implementation of some other part of the protocol, returning the wrong position (and thus leading to incorrect queries to firstRectForRange: using our own bad data)?

[hellohuanlin]

If we were returning the actual offset of the words relative to the origin of the Flutter view (where the UITextInput is), and UIKit were drawing the rect using that same offset and origin, then the rectangle would be in the correct location. It's not, so either one of those things is not true, or something else is wrong that I still don't understand.

Sorry I think I misread your previous message - we were not returning the offset of the actual rendered words (drawn by flutter), because UITextInput is at the wrong place. In my mental model, UITextInput should have been placed at the same position as the actual rendered words, that's why I said "UITextInput itself is at the wrong position, but it's returning the right rects in query APIs" (which was indeed confusing, my bad).

If we were to keep UITextInput to stay at the origin of FlutterView, then we would need to update all offsets to make it right. Though based on my understanding of existing code, iPad scribble likely requires UITextInput to overlap with the actual rendered text (which matches my mental model).

Looking at the code here more closely, I see that the beginning of firstRectForRange: is assertions about the class types of the inputs being our own classes. That means that the values UIKit is passing us to query the rect are values we provided from some other part of the text input protocol. Given that, do we actually know that the missing last character is an iOS bug, and not a bug in our own implementation of some other part of the protocol, returning the wrong position (and thus leading to incorrect queries to firstRectForRange: using our own bad data)?

We are querying the correct range (every letter of the last word, including the last letter). For example, in the following screenshot, the input value is a texx, and the range of misspelt word is [2, 6), and as you can see in the logging below, UIKit queried the range for t, e, x, x one by one.

[justinmc]

I'm interested in understanding more about why the highlight lags by 1 character if you know any more details @hellohuanlin. It would be great if we could fix that rather than hiding it off screen. If that's not possible then this approach looks good to me, though.

I wonder if some state is out of date (_markedRect, text?) at the time that firstRectsForRange is called.

I do not understand how both of these things can possibly be true.

My understanding is (before this PR) we returned the position of the text relative to the UITextInput correctly, but the UITextInput was just stuck naively at 0,0 rather than at the location of the TextField. We could easily put the UITextInput at the correct location of the TextField, but then the highlight would show up, and it looks wrong since it lags by 1 character. This PR moves UITextInput offscreen to avoid this.

So the real problem we have here seems to be the 1 character lag in the autocorrect highlight, if I'm understanding correctly.

[justinmc]

If scribble is enabled, do we still have the autocorrect highlight bug then?

[justinmc]

(I posted my review without seeing @hellohuanlin's reply above)

So firstRectsForRange is called one by one for each of the characters that should be highlighted, and we return the correct rects for all of those characters, but mysteriously the last character is not highlighted?

[stuartmorgan-g]

Okay, so to make sure I'm understanding correctly: when you said:

We are returning the offset of queried text, relative to the upper-left corner of UITextInput

what you actually meant is that we are returning the offset relative to the Flutter text input widget, not relative to the UITextInput?

(That was my assumption based on the visual behavior, but I was confused by your assertions to the contrary.)

For example, in the following screenshot, the input value is a texx, and the range of misspelt word is [2, 6), and as you can see in the logging below, UIKit queried the range for t, e, x, x one by one.

And you've validated that we are in fact returning the correctly sized and positioned rect for the final x in that case? We're not, e.g., ending up in an invalid-rect-return codepath for that?

Also, is this specific to the very last character of the text field? I.e., if you type a text field, then go back and delete text then re-type it as texx, does it have the same behavior, or not?

[hellohuanlin]

I wonder if some state is out of date (_markedRect, text?) at the time that firstRectsForRange is called.

They look right to me. Note that _markedRect is actually for CJK languages' intermediate state (you can see example in this PR's description).

So firstRectsForRange is called one by one for each of the characters that should be highlighted, and we return the correct rects for all of those characters, but mysteriously the last character is not highlighted?

Yes, we return the correct rects (if UITextInput is overlapping the flutter rendered text, rather than at the origin of FlutterView).

[stuartmorgan-g]

If we need to keep the off-screen hack, please use a dynamic value that is definitively off-screen based on iterating all UIScreen objects and uninioning their bounds.

[hellohuanlin]

Updated to use the current UIScreen that it is on (I think current screen is good enough unless im misunderstanding)

[stuartmorgan-g]

What effect does this have on IME display? Or do we do all of that via Flutter UI?

[hellohuanlin]

I can confirm that IME still works. Below is IME with software and hardware keyboards:

[hellohuanlin]

Ah I tested it again, and found iOS 16 and 17 have different behaviors for IME. Without the fix (main branch), iPhone shows IME on top (issue here). The current behavior is bad, but this PR will hide IME, which is even worse.

Even after putting the UIInputView to overlap with text doesn't 100% resolve the problem - the IME is still slightly off. I'd argue that it's somehow worse, because it covers the text:

Let me dig more into it, but I think we have to halt this workaround.

[LongCatIsLooong]

It sounds like what's happening could be, when the user types (btw are you using the software keyboard or the hardware keyboard?):

1. the key event is sent to the framework and the framework processes the platform channel message during the idle phase, sending back the updated text editing state without waiting for the text layout process to take place.
2. the ios text input plugin receives the updated text editing state and that triggers the software keyboard implementation to query the "firstRectsForRange" using the updated text editing state.
3. the framework starts to layout the text in TextPainter, sending back the bounding boxes for visible characters in composite phase, or in a post frame callback. In either case it will be too late since the software keyboard has already called the "firstRectsForRange" method and got the outdated bounding boxes

[hellohuanlin]

what you actually meant is that we are returning the offset relative to the Flutter text input widget, not relative to the UITextInput?

@stuartmorgan Well, I was actually thinking about UITextInput, not Flutter widget.

In my mental model, UITextInput is an "invisible" native text on top left corner, of exactly the same size as the text drawn on screen. If we were to implement the UITextInput in native iOS, we would draw that invisible native text out, like we are implementing our own version of UITextField or UITextView. But since Flutter handles the text drawing, we do not need to draw it on native side.

Previously, we did not need this "invisible native text" to overlap with the Flutter drawing, so we put it anywhere (conveniently, top left corner). But later, when adding iPad scribble support, if scribble is enabled, we have to put this "invisible native text" to overlap with the Flutter drawing on screen. In either scribble or non-scribble case, the range query API returns the same offset, relative to UITextInput (just the position of UITextInput itself is different in these 2 cases).

And you've validated that we are in fact returning the correctly sized and positioned rect for the final x in that case? We're not, e.g., ending up in an invalid-rect-return codepath for that?

Yeah I have manually checked all rects. But let me try again (and post the result here in case I made terrible mistakes)

Also, is this specific to the very last character of the text field? I.e., if you type a text field, then go back and delete text then re-type it as texx, does it have the same behavior, or not?

Let me try this too.

[stuartmorgan-g]

If we were to implement the UITextInput in native iOS, we would draw that invisible native text out, like we are implementing our own version of UITextField or UITextView. But since Flutter handles the text drawing, we do not need to draw it on native side.

Yes, which means the actual text only has one position on the screen: the position where Flutter draws it. I think it would help if any discussion of the position of text referred to the position of the actual text that has a position, not to the theoretical position of theoretical text that does not actually exist.

In either scribble or non-scribble case, the range query API returns the same offset, relative to UITextInput (just the position of UITextInput itself is different in these 2 cases).

I understand that what we return needs to be the offset relative to the UITextInput. My point above was that referring to the position we are returning as "correct" in the case where what we are returning is not the position of the the actual text on screen relative to the actual UITextInput location is extremely confusing.

[hellohuanlin]

My point above was that referring to the position we are returning as "correct" in the case where what we are returning is not the position of the the actual text on screen relative to the actual UITextInput location is extremely confusing.

Yeah agree it was confusing. I missed a big "if" - it's correct if the UITextInput overlaps with the text drawn on screen (which is the iPad scribble case). Sorry I think i paid most attention on the size part and somehow overlooked the subtle offset part.

[hellohuanlin]

Closing this PR in favor of #44779