Flutter breaking change: didChangeInputControl added to TextInputClient

[justinmc]

@matthew-carroll Another breaking change has been made to TextInputClient on the master branch of Flutter (flutter/flutter#76072). The didChangeInputControl method was added, so the classes in super editor that implement it will be broken.

The point of the breaking change is to add support for virtual keyboards to Flutter, which are keyboards written in Flutter itself as a part of the app (useful for embedded environments with no system keyboard). I left the implementations empty in this PR, but maybe you'd want super editor to support this, in which case you can probably just do something similar to what EditableText does.

I tried running the super editor example on this branch with the Flutter master channel that contains the breaking change, but it looks like there's an unrelated problem preventing super editor from running on master and beta, if it's not just something weird with my setup. This PR did seem to fix the errors related to didChangeInputControl, though.

[matthew-carroll]

@justinmc thanks for the PR. Are there docs about how this method works? Multiple keyboards have always been usable, right? Just not Flutter keyboards? I'm surprised that there's any signal for this at all. Is there a justification somewhere for why Flutter would want to create a special recognition of Flutter keyboards?

[justinmc]

The best docs are on TextInputControl. We're in the process of reverting and relanding that feature so I'll just link to the docs in the original PR instead of the docs site, but notice there is an example there too: https://github.com/flutter/flutter/pull/76072/files#diff-701a97b414026a5c87a06b546bb38ca70be2aa0ce44cbf170f4b43e8defcc91aR2136

That PR lets you send text input changes to a TextInputClient from the framework, similar to how changes are communicated to the framework from physical keyboards and IMEs via the engine. That means that you can create buttons at the app level that act as keyboard keys and send their changes to EditableText.

See this issue for how this project came about: flutter/flutter#68988

[matthew-carroll]

@justinmc I see that the motivation is embedded systems, because those systems don't have an OS keyboard like iOS and Android.

I'm still curious why the IME client cares about this? Isn't the job of the IME client to handle any given IME state or delta, regardless of source? What is the IME client expected to do with this information? Up to this point, apps might use the standard OS keyboard, GBoard, or Samsung keyboards, and I assume those could change while running an app, but we haven't needed to notify the text input client. Is there a reason that this particular use of, or change of the keyboard is relevant for IME clients?

[dnfield]

@justinmc I see that the motivation is embedded systems, because those systems don't have an OS keyboard like iOS and Android.

I'm still curious why the IME client cares about this? Isn't the job of the IME client to handle any given IME state or delta, regardless of source? What is the IME client expected to do with this information? Up to this point, apps might use the standard OS keyboard, GBoard, or Samsung keyboards, and I assume those could change while running an app, but we haven't needed to notify the text input client. Is there a reason that this particular use of, or change of the keyboard is relevant for IME clients?

My understanding is that in the examples you're giving, the OS specific embedding is abstracting this away from Dart based code - so if an Android devices goes from GBoard to Samsung keyboard, the Java code updates its reference internally but does not require that Dart code do the same.

OTOH, the framework patch allows someone to write an IME in Flutter, in which case the control object has a different role and is not getting preserved.

Regardless of whether this is a good design, it would be great to get this patch to a landable state and accept it so we can get the SuperEditor tests back into the customer testing repository and avoid further breakage/divergence. That way we could still iterate on the API design as necessary without as much risk.

[matthew-carroll]

Let's not merge bad API decisions

Regardless of whether this is a good design,

I appreciate the desire to get our tests running in the registry again, but I couldn't disagree with this more. We're getting more and more bad design decisions in the critical area of Flutter's text input system. I've mentioned many times before, there's no escape hatch for this part of Flutter. Every Flutter app in the world is stuck dealing with whatever the Flutter team does here. Let's please not ignore the implications and future consequences of bad API design.

That way we could still iterate on the API design as necessary without as much risk

Getting tests to pass doesn't mean "low risk", nor does it mean "low damage". Each time Flutter releases a breaking change, it really does break people. So if Flutter messes with this API once a quarter, Flutter is going to break every earlier release of Super Editor every quarter. That means all of our customers are forced to upgrade every quarter, for no other reason than the Flutter team didn't want to think these things through. What's the point of proposals and design docs if this is where we end up?

Revisiting the technical details

@dnfield you may not be the right person to answer these questions, but despite my best attempts, I can't seem to find anybody who is qualified to answer these questions. Frankly, it's astounding to me that it could be this difficult to get a clear answer as to why the Flutter team approved and merged a new method on a core interface...

so if an Android devices goes from GBoard to Samsung keyboard, the Java code updates its reference internally but does not require that Dart code do the same.

What control object are we talking about? Is this technical issue described in the proposal? If it is, I missed it. I've been trying my best to understand the specific technical requirement for this method, but haven't heard anything concrete. I've gotten high-level wishy-washy responses, and I've been referred to an issue ticket, PR, and proposal, none of which seem to make clear why this method exists, or what it's expected to do.

the framework patch allows someone to write an IME in Flutter

I still don't understand where my initial analysis went wrong. The TextInputClient currently doesn't know, and doesn't care what IME implementation it's working with. It doesn't know the difference between an iOS IME, an Android IME, a web IME, or a desktop IME. It doesn't know when the IME implementation changes. Why would writing an IME in Dart require any additional knowledge in the TextInputClient, whose only job is to process the current state of content editing, and update that state based on user input?

My theory is that this change takes a responsibility that belongs in the framework and pushes that responsibility into every implementation of TextInputClient.

Here's a diagram that shows the information flow that I would expect for a Dart IME:

Here's a diagram that shows the information flow for this particular implementation of Dart IME support:

It seems to me that the implied control flow for a Dart IME is plugging in at the wrong place. The framework boundary should be the place where all input signals plug in. Even the signals that originate in Dart code.

Think about how most of the testing signals work. When you fake touch events, Flutter's test system doesn't call a method directly on a gesture detector that says "Hey, FYI, you're now connected to a Dart gesture signal". No. Instead, the test system injects fake events through the real reporting system, because the point of gesture detectors is to respond to gestures, not to control the source of the gestures, nor to implement different behaviors based on the event origination. In fact, this exact same issue exists in tests for text. What about sending fake IME events in Flutter tests? Is Flutter's test system going to bypass the standard framework reporting mechanism? That sounds like a horror show waiting to happen...

If Flutter creates this watershed division of responsibilities, it's going to lead to a much bigger Frankenstein's monster. Let's not do that, please!

I'll repeat here the questions that I asked on Discord that never got a response:

What, exactly (I mean precisely, and technically, with an example), doesn't work in the absence of this new method?

What other reasonable response could there be to this method than to call show and hide?

Why is this the responsibility of the TextInputClient? We don't receive signals for any other kind of keyboard, so why do we seem to be creating a special snowflake situation for a virtual Flutter keyboard?

[dnfield]

For better or worse, the Flutter team has already accepted this API. I'm sympathetic to your concerns, and could imagine some improvements could be made that would eventually back this API out.

@justinmc and @jpnurmi would be the right people to talk to about this API. @cbracken also has worked on some of this more recently. For what it's worth, I would be supportive of an update to Flutter that obscured this responsibility from the TextInputClient as you are suggesting, but I did not work on this change and may be missing something - the people I mentioned have more recently been working on text input much more extensively. It's entirely possible that such a change will be effected but will just take some time to hash out.

In the mean time, there's a separate issue here too: to be part of the customer testing registry, one of the requirements is explicitly that

The upstream repository that hosts the tests must be able to receive patches to support the master channel of Flutter. This means that CI on the upstream repository should use the master channel Flutter SDK.

If you are not able to do this, this repo won't be able to be part of the customer testing registry. That means that additional changes that impact you may take longer for you to become aware of, and it will likely make it that much harder to give feedback before changes like this become more entrenched/part of stable releases.

AFAIK, both Justin and JP are available on Discord - it may be worth trying to reach out to them there if you're not getting responses here.

[matthew-carroll]

AFAIK, both Justin and JP are available on Discord - it may be worth trying to reach out to them there if you're not getting responses here.

I've spoken with Justin at great length. He doesn't have the answers to the above questions.

I posted those questions to JP. Haven't heard anything back yet.

Maybe Chris can finally clarify why this method should exist.

If you are not able to do this, this repo won't be able to be part of the customer testing registry.

You're welcome to quote the policy. You're welcome to kick us out of the registry. I don't see how that's in Flutter's interest. I don't see how that's in the community's interest. Therefore, to the extent that it's difficult or impossible for us to comply, you might look inward to see what Flutter is doing that makes it so difficult to comply. You might compare the number of tests in the registry against the number of projects in the ecosystem. It's probably worth asking why there's such an immense disparity.

That means that additional changes that impact you may take longer for you to become aware of, and it will likely make it that much harder to give feedback before changes like this become more entrenched/part of stable releases.

We were in the registry. Justin came over here to put up this PR because we were in the registry. How's the "feedback" part going? I can't get clear answers to basic questions. I don't know how or if we're supposed to even implement this method. I have yet to see my concerns taken seriously by anyone in a position to act on them.

The irony is that this thread right here, right now, is "feedback". But instead of processing the feedback, the message is "adapt to the bad API or get out of our registry".

[Hixie]

@matthew-carroll, unfortunately, when the tests were in the registry they didn't cause a test failure when they failed. This is regretful, and was an oversight we all missed, but it does mean that effectively the tests weren't in the registry in the sense that would let us know when a test failed. I urge you to enable the tests again so that we avoid this very specific kind of problem in the future again.

The point of the registry is for us to avoid breaking you by literally fixing your code for you (I'm impressed that we're doing this here even though there are no tests in the registry currently!). Up until https://github.com/superlistapp/super_editor/pull/837#issuecomment-1299388133 this thread seemed to be a healthy discussion of the design, but then the discussion took a weird turn where you started describing the API as "bad", which doesn't seem very friendly or productive. If you want to influence a design, you'll be more successful by leveraging empathy and presenting technical arguments in a friendly way rather than insulting the design. :-)

In any case, the goal of this PR as I understand it is to get your repository back into our test registry ASAP so that we can make sure you are in the loop of future changes. This is orthogonal to whether the API itself is what you would prefer, since it can continue to be changed (at least until it hits stable, after that we want to slow our roll, obviously) even after your tests are back in.