Scribble mixin

[justinmc]

This PR separates the handwriting text input feature from TextInputClient. The main point of the PR is to minimize breaking changes when working with platform APIs in the framework.

Design doc.
PR to make TextInputClient a mixin too: #104291

Depends on engine PR flutter/engine#36642.

Background

PR #75472 added scribble support directly to the abstract class TextInputClient, and this broke users (specifically, super_editor), which had implemented TextInputClient. This kicked off a discussion on how to add, change, and expand platform API support without causing these kinds of breaking changes.

How to minimize these breaking changes

The solution that I'm proposing with this PR is:

1. Use separate method channels as much as is reasonable.
2. Prefer mixins used with the with keyword instead of abstract classes.
3. Mix into widgets narrowly focused on bringing this platform functionality to the framework instead of making EditableTextState a catch-all.

1. Prefer separate method channels

Scribble is closely related to text input, but there are no direct dependencies, so I've created a separate MethodChannel for it in this PR (SystemChannels.scribble) instead of reusing SystemChannels.textInput. This comes along with a separate class to manage the channel (Scribble) similar to the existing TextInput class.

2. Prefer mixins and with

Similar to TextInputClient, I've also created a separate ScribbleClient, but it is a mixin instead of an abstract class. ScribbleClient is then mixed in using with.

3. Prefer new composable widgets to an EditableText catch-all

ScribbleClient is mixed into a new widget's state, _ScribbleFocusableState, not EditableTextState. This helps us separate concerns in our widgets as well by utilizing one of Flutter's core principles, composability.

The result

This approach would not have broken implementers of TextInputClient if it had been used originally. Also, any future changes to TextInputClient or to ScribbleClient will not affect users of the other class. Users can hack the platform text input and handwriting API support with less fear of breaking changes.

Migration guide

Users that were using TextInputClient for its own features and not for handwriting support likely had empty methods for showToolbar, insertTextPlaceholder, and removeTextPlaceholder. They may get warnings if they were using @override annotations, but shouldn't be broken by this change. Either way, they should remove the empty methods:

class MyEditorState extends State<MyEditor> implements TextInputClient {
  // If you had empty implementations of these methods just to prevent
  // compiler errors, you should remove them now. They are no longer
  // part of TextInputClient.
  /*
  @override
  void showToolbar() {}

  @override
  void insertTextPlaceholder(Size size) {}

  @override
  void removeTextPlaceholder() {}
  */

  // The rest of the class...
}

Users that had implemented their own handwriting feature using these methods will receive the @override warnings if they're using them, and their handwriting functionality will stop working. They should mix in ScribbleClient to regain the connection to the platform methods for scribble.

// If you had implemented your own custom handwriting functionality, mix
// in ScribbleClient like this to keep it working.
class MyEditorState extends State<MyEditor> with ScribbleClient implements TextInputClient {
  @override
  void showToolbar() {
    // Custom implementation...
  }

  @override
  void insertTextPlaceholder(Size size) {
    // Custom implementation...
  }

  @override
  void removeTextPlaceholder() 
    // Custom implementation...
  }

  // The rest of the class...
}

[nilsreichardt]

I don't know if this intended but the design doc is not public 😢

[justinmc]

@nilsreichardt Thanks for pointing that out! Updated.

[justinmc]

CC @fbcouch

[flutter-dashboard]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[LongCatIsLooong]

With scribble in a separated object you don't need to eagerly initialize TextInput I think?

[LongCatIsLooong]

This class seems to have more responsibilities than just receiving focus?

[LongCatIsLooong]

we probably want to explain what UIIndirectScribbleInteraction is and why it is special.

[LongCatIsLooong]

The documentation didn't explain offset?

[justinmc]

No, but I'll add something about it.

[LongCatIsLooong]

This is now specifically for scribble.

[LongCatIsLooong]

nit: specify coordinate space.

[LongCatIsLooong]

The change makes the line a bit different. Scribble could be in progress while there's no active connection.

[justinmc]

Is that handled by the part above that was added for autofill?

    if (!_shouldCreateInputConnection) {
      return;
    }

[LongCatIsLooong]

The change can still come from the keyboard when scribble is in progress (especially when there's a connected hardware keyboard). But since it's already there I'm fine with keeping this as is.

[justinmc]

Sounds good, let's keep an eye out if any issues are filed about typing with a keyboard while doing handwriting.

[LongCatIsLooong]

shouldn't the same check be performed in other methods as well?

[justinmc]

In _onPlaceholderLocationChanged and _onScribbleFocus, the _ScribbleFocusable widget prevents them from being called when scribble is disabled. Should I add the check (or an assert) just as a double check you think?

[LongCatIsLooong]

Is this right? The documentation states:

/// Set the given [ScribbleClient] as the single active client.
///
/// This is usually based on the [ScribbleClient] receiving focus.

[justinmc]

Good call, the docs are right, it should be set on focus. I'll fix that and add a test.

[justinmc]

@LongCatIsLooong Ready for re-review.

[LongCatIsLooong]

The change can still come from the keyboard when scribble is in progress (especially when there's a connected hardware keyboard). But since it's already there I'm fine with keeping this as is.

[LongCatIsLooong]

Is this part of the comment?

[justinmc]

Whoops, removed.

[LongCatIsLooong]

There's no guarantee that _client will be non-null I think?

[justinmc]

You're right. I'm going to return before this if it's not null, so if the engine somehow sends handwriting while no field is focused, it will just be ignored.

[LongCatIsLooong]

what if this client didn't have the focus?

[justinmc]

Great catch here and your next comment. I've updated it here and there to only set the client to null if this instance is currently the client.

[LongCatIsLooong]

Scribble.client could be pointing to a different client at this point.

[goderbauer]

(Triage): Are there still plans for this one?

[justinmc]

Planning to return to this after #107193!

[justinmc]

This PR must wait for flutter/engine#36642 to be rolled into the framework, which it is poised to do in #113951.