[SuperTextField][iOS] Ignore auto-corrections after handling input actions (Resolves #2004)

[angelosilvestre]

[SuperTextField][iOS] Ignore auto-corrections after handling input actions. Resolves #2004

On iOS, auto-corrections can happen after pressing the action button. For example, the user might edit some content, press "done", and after the "done" action is handled the IME reports the text replacement.

This PR prevents that by ignoring text replacements in the same frame that input actions are reported, except for TextInputAction.newLine. TextInputAction.newLine can't prevent replacements from being applied, otherwise the user won't be able to replace the selected content with a new line.

[angelosilvestre]

@miguelcmedeiros Can you please try this PR and see if it work for you? It will be required that super.performAction(action) is called.

[miguelcmedeiros]

It will be required that super.performAction(action) is called.

@angelosilvestre if the user presses "done" and then do not call super.performAction() then I would expect that the replacement is not applied and the user can keep typing.

Is that possible?

[angelosilvestre]

@miguelcmedeiros If you do not intend to call super.performAction() then we would need to have some method like preventReplacementsUntilNextFrame() for subclasses to call and signal us that we should do that.

Would that work for you?

[miguelcmedeiros]

@angelosilvestre I guess that would work.

The goal is to be able to handle the "done" key press and only apply a custom behaviour from our side.

[matthew-carroll]

The goal is to be able to handle the "done" key press and only apply a custom behaviour from our side.

@miguelcmedeiros can you please elaborate on what that means in practice? What custom behavior would you want to add, and what default behavior are you trying to avoid?

We can't make any guarantees about completely stopping all default behavior, because it seems that some default behavior isn't in our control.

Also, do you feel that you understand the nature of the problem that we've discovered, and what we're doing to solve it?

I want to make sure we're on the same page because I'm not a fan of the option to add a very obscure public method that no implementer will understand.

[miguelcmedeiros]

@matthew-carroll similar to keyboard handlers, it would be great that as a developer I can intercept the pressing of the return button in the soft keyboard so that I can define what happens, e.g. perform a custom action defined on the app side, and then block or allow it to execute the default action.

[matthew-carroll]

I understand the abstract desire. Can you please provide some examples of what's needed, concretely? As I mentioned, we do not have the level of control to provide the abstract goal. Instead, we'll need to look at general areas of control that you definitely want, and then see what we need to do to make those possible.

[miguelcmedeiros]

A concrete can be what is shown in the screen recording in the ticket description. The user typed some text and then presses return. In our case, we are presenting the user with a due date suggestion, which the user can accept by tapping on the suggestion overlay or press return.

Currently, if IME suggests a change to the text, then it will be applied, which is not desired for our use case.

[matthew-carroll]

So does Angelo's change fix that problem (excluding the idea of a new public method)? If so, what new problems does Angelo's approach create for you? Or what other needs are not being met by that change?

[matthew-carroll]

@angelosilvestre let's automatically apply this delta-ignoring behavior outside of the performAction() call. No configuration, or opting in or opting out, we'll just always ignore deltas that come in the frame after the action.

Then, the purpose of the performAction() override will go back to what it was before. If you want the default behavior, don't override it. If you want totally custom responses to actions, then override it.

Make sense?

[angelosilvestre]

@matthew-carroll This is the current implementation of this PR.

[miguelcmedeiros]

@angelosilvestre I tested out the changes with the example code and it still performs the text replacement when pressing return.

[angelosilvestre]

@miguelcmedeiros Did you call super.performAction?

[miguelcmedeiros]

Nope. My understanding was that if super.performAction is not called, then nothing else happens to the text field.

[angelosilvestre]

@miguelcmedeiros The original issue isn't caused by the default performAction implementation. The issue is that the iOS IME is reporting a replacement due to autocorrection after reporting the input action.

The fix in this PR consists in discarding replacement deltas reported in the same frame that an input action is performed. For that, the default performAction implementation needs to be called, so we can keep track that an action was performed.

[matthew-carroll]

For that, the default performAction implementation needs to be called, so we can keep track that an action was performed.

@angelosilvestre my point in my earlier comment is that we need this statement to not be true. We do not want to require any particular use, or non-use, of performAction to ignore the auto-correction.

[angelosilvestre]

@matthew-carroll But then how do we know we need to ignore the replacement?

Should we provided a method to signal that we should ignore the deltas as I suggested in this comment? #2012 (comment)

[matthew-carroll]

We should always ignore the replacement on the frame after an action. That's the same thing we're doing in super editor, right?

[angelosilvestre]

@matthew-carroll But if the IME controller is extended to avoid calling the default performAction implementation we cannot know that an action was performed.

[matthew-carroll]

Why can't we install our own private callback with the IME, implement ignoring deltas after actions, and then forward from our private callback to the regular callback in the controller?

[angelosilvestre]

@matthew-carroll I think that, for SuperTextField, the IME controller IS the delta input client, isn't it? The performAction is called directly on the IME controller.

[matthew-carroll]

So what are our options to control that callback?

[angelosilvestre]

@matthew-carroll Maybe introduce a handleAction and then tell people to override that instead of performAction?

Then, our performAction implementation will be just setting the flag to ignore the deltas and calling the new handleAction method.

[matthew-carroll]

Ok, let's see how that looks in code.

[angelosilvestre]

@matthew-carroll Updated. With this change, the controller subclass will look like the following:

class _TextController extends ImeAttributedTextEditingController {
  @override
  void handleTextInputAction(TextInputAction action) {
    // Do not perform actions, while still letting the base controller to keep track of internal flags.
  }
}

[matthew-carroll]

Please include why we're tracking this. Also "differente" -> "different"

[matthew-carroll]

This method call is directly forwarding to another method call. Why do we need the handleTextInputAction()?

I know what you mentioned in the PR description, but it seems to me like the situation here isn't exactly what was described.

As you can see, we already have a _onPerformActionPressed callback. That callback is publicly settable. It's called every time an action is pressed.

Why was there ever a need for anyone to try to override performAction()? If a client wants to customize the action behavior, why wouldn't that client just set their own value on onPerformActionPressed?

[miguelcmedeiros]

@angelosilvestre I tried again (by setting onPerformActionPressed) and it works as expected.

I'm guessing that yesterday when I tested, I picked the wrong commit and so, was hitting the same issue continuously since your fix was not included. Sorry about the confusion.

[angelosilvestre]

@matthew-carroll So I guess we can just revert the last commit, right?

[matthew-carroll]

@angelosilvestre I don't know. I can't tell exactly what the situation is. If you want to revert the commit then I can take a look at what that looks like. I still think we want the deltas to automatically be ignored in the next frame, no matter what the app chooses to override. If that was already accomplished in the previous commit, then maybe that commit works.

[angelosilvestre]

@matthew-carroll Yeah, I believe the previous commit will work.

[matthew-carroll]

Looks like we're back to the situation where if this method is overridden by a subclass without calling super(), we're not going to get the expected behavior to ignore the deltas.

Either this method should be annotated with "must call super", or this behavior needs to be moved to a place where its guaranteed to run.

[angelosilvestre]

@matthew-carroll Added @mustCallSuper.

[matthew-carroll]

LGTM