The problem with the AttributedTextEditingController API

[matthew-carroll]

There's a problem with the API for AttributedTextEditingController. Here's an example of the problem.

Imagine a text field with the following content, and caret position:

Welcome |@John to the team

In this example "@John" is a user name token. It's a string that's treated as a single character, from the perspective of text selection. As such, if the caret is ever placed within a token, the caret needs to be immediately moved outside the token. Imagine that we've implemented a sub-class of AttributedTextEditingController that enforces these selection rules. What should that subclass do when the caret appears as follows?

Welcome @|John to the team

The caret sits between "@" and "John", which is illegal. The controller needs to move that caret somewhere. Where should it move? Well, it depends...

If the user tapped at that location, then the caret needs to move back to the beginning of the token:

Welcome |@John to the team

However, if the user pressed the right arrow key, then the caret should "jump over" the token, placing the caret on the trailing end of the token:

Welcome @John| to the team

How do we know which action to take?

A problem with AttributedTextEditingController is that it combines use-case oriented behaviors, like moveCaret, with direct property manipulation, like set selection. If a client changes a selection with controller.selection = mySelection, then the controller can't possibly know which use-case rules to enforce.

Proposal

I think we should remove the setters for text and selection and force all controller changes to include some useful amount of context. Here's a possible API surface for the controller:

AttributedText get text;

TextSelection get selection;

TextRange get composingRegion;

/// Place the caret at a given [position], regardless of where it was previously, like when
/// a user taps at a position.
void placeCaret(TextPosition position, {bool expands = false, bool inheritAttributions = true});

/// Move the caret one character in the given [direction], like when a user presses an
/// arrow key.
///
/// The selection keeps its current `base` and expands the `extent`, if [expands] is `true`.
///
/// If the selection is expanded, and [expands] is `false`, the selection collapses at the extent.
void moveCaret(TextAffinity direction, {bool expands = false});

/// Selects all the text.
void selectAll();

/// If the selection is expanded, the selection is collapsed at the `extent`.
void collapseSelection();

/// If selection is collapsed, inserts the given [text] at the current caret position, and moves 
/// the caret to the end of the inserted text, if the selection is expanded, the selected text
/// is replaced with the given [text], and the caret is placed at the end of the inserted [text].
void insertAtCaret(AttributedText text);

/// Deletes text in the given [direction].
///
/// If the selection is collapsed, a single character, or piece of content, is deleted. If the selection
/// is expanded, then the selected text is deleted.
void delete(TextAffinity direction);

/// Replaces the current text and selection with the given [text] and [selection].
///
/// This method is the replacement for the "text" and "selection" setters. But this method makes it
/// clear that the client is making a dramatic change, and it requires that the client explicitly choose
/// the new text, selection, and composing region. The implementation of this method doesn't need
/// to make any sophisticated choices about how to maintain a selection when the text changes, or
/// how to enforce selection rules when the selection changes. This method should almost never be
/// called.
void replaceEverything(AttributedText text, TextSelection selection, [TextRange composingRegion = TextRange.empty]);

[matthew-carroll]

@venkatd @miguelcmedeiros - I'd like to get your thoughts on this proposal. Do you think a use-case-oriented text field controller makes sense? Or do you think the API surface might be too constraining, or otherwise create new problems?

[venkatd]

@matthew-carroll

Speaking for SuperTextField, but I think it can apply to all of the editors.

Taking an event-sourced approach with commands/edits may allow the kind of flexibility needed without introducing a large surface API. So I'd consider #49 and #67 related to this.

The summary would be:

• The controller could have a single method like controller.execute(moveCaretCommand). Everything would have to go through here.
• A command could make use of the primitives operations
• We could have a single object that represents the core state of the controller. So something like TextEditValue(text, selection, composingRegion)
• controller.execute would be atomic, so it would execute the command and replace the old state with the new one in a single go

Some implications of this:

• Developers edit can remap user interaction (keyboard shortcuts, taps) to their own commands. For example, on some platforms/apps triple click may do something different with the text selection.
• Implementing undo/redo will be easier because there will be no way to bypass state changes. And changes are atomic per command.

We've implemented this pattern in our own app and it has been straightforward for us to add a layer on top of SuperTextField that works as expected on multiple platforms (keyboard navigation, shortcuts, etc) and implement undo/redo. I can document link to it this month.

On a related note, are there any performance implications to making AttributedText immutable?

[matthew-carroll]

@venkatd it sounds like you're suggesting adding another layer of indirection on top of what's suggested. Can you describe what you're aiming to solve?

Also, the proposed API doesn't mention any specific key combos or gestures, so I think the proposal already allows you to map any input to the desired change.

[venkatd]

@matthew-carroll good point, I may be conflating a few use cases.

I can describe our own use case and if it's unrelated, I can create a new issue.

We've implemented our own subclass of the text editing controller and it uses the mapping as follows:
https://gist.github.com/venkatd/74bf669f824cf291b487011c64e587d6

To account for the nuances of each platform we ended up with a large map. So when we react to a keyboard combination, we lookup the command and call controller.execute(commandForKeyboardCombo).

To support undo/redo in a generic way, we centralize where we update the state. The only place we update state is in a call to updateTextAndSelection. And if we have multiple ways to to update the state, we'd have to override every one of those methods in our subclass to prevent bypassing the state. Or possibly throw an unimplemented error to force the usage of controller.execute.

It also helps that this state change for each command (including compound operations) is atomic. For example, if I want to delete the current word by running controller.someOperation() a few times, it would cause a bunch of notifyListeners() to get called and make it harder for our undo/redo implementation to recognize what operations are grouped together.

I'll write up some pseudocode for a proposal and clarify what we're aiming to solve this week or next week. (If you decide to go ahead with this proposal, it won't be a show-stopper for us since we can just override those handlers and prevent them from being used.)

[matthew-carroll]

I think the undo/redo behavior is probably unrelated. And we have a ticket and milestone for that, so no need to file anything there.

But it sounds like you're saying that on the road to undo/redo, you needed to force all updates through a single method, rather than multiple methods, so that you could capture the changes?

I can see why you did that, but I'm not sure that this proposal helps or hinders that situation.

The existing implement of AttributedTextEditingController allows arbitrary changes to its text and selection properties. You can change them independently (in either order), or change them together. It's convenient to say controller.text = AttributedText(text: "some new text"); but that statement doesn't provide any context about why the text was changed. For example, when you change that text value, what's supposed to happen to the existing selection?

What I described at the top of the proposal is an example of a situation where it becomes impossible to do the right thing when the client sets the text and selection properties. The client could inspect the text, find the tokens, and change the selection, but I'm sure the client would like the controller to enforce the token selection rules, instead of re-implementing those rules in every client. To enforce the token rules, the controller must understand some additional details about the action being taken. For example, the controller will enforce the rules differently depending on whether you placeCaret() into a token, or moveCaret() into a token.

I guess the outstanding questions are:

• Is it appropriate to have SuperTextField depend on a controller whose API attempts to account for every way that text field content might change, or should SuperTextField depend on a controller that essentially just has one method to set its value, without any intelligence at all?
• If we want to try to express all the ways that a text field's content might change, have we identified all of those ways? Is there anything obvious that's missing from the proposed behaviors?

[venkatd]

But it sounds like you're saying that on the road to undo/redo, you needed to force all updates through a single method, rather than multiple methods, so that you could capture the changes?

Yes that's right. In the overridden controller, setting the value+selection is a single operation and it's done through a single method.

We've got a class that represents the full state (text+selection) as defined here:

class TextEditValue {
  const TextEditValue({
    required this.text,
    required this.selection,
  });

  final AttributedText text;
  final TextSelection selection;

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) return true;
    if (other is! TextEditValue) return false;
    // @TODO, understand why we can't compare AttributedText for equality
    return selection == other.selection && text.text == other.text.text;
  }

  @override
  int get hashCode => text.hashCode;

  @override
  String toString() {
    return 'TextEditValue(text: $text, selection: $selection)';
  }

  TextEditValue copyWith({AttributedText? text, TextSelection? selection}) {
    return TextEditValue(
      text: text ?? this.text,
      selection: selection ?? this.selection,
    );
  }
}

• Is it appropriate to have SuperTextField depend on a controller whose API attempts to account for every way that text field content might change, or should SuperTextField depend on a controller that essentially just has one method to set its value, without any intelligence at all?

I think so. The controller could be responsible for ensuring the value is a valid state (i.e., selection is within bounds), but wouldn't require hardcoding the use cases inside the controller.

• If we want to try to express all the ways that a text field's content might change, have we identified all of those ways? Is there anything obvious that's missing from the proposed behaviors?

I think it makes a lot of sense to take a use-case based approach, but I don't know if placing those as methods on a controller will scale. It will be hard to anticipate all of the use cases. For example, during development, we would regularly get a bug report on some combination of OS+browser that a certain keyboard combination didn't accomplish some use case.

Some examples of these use cases:

• Delete to start of line
• Delete word
• Move back by a word
• Move forward by a word

Depending on the platform, we mix and match different keyboard combos to intents. For example lets consider arrowLeft which we map to a command as follows:

  LogicalKeySet(LogicalKeyboardKey.arrowLeft): const MoveCaretToSelectionBounds(
    CaretDirection.left,
    orElse: MoveCaretTo(previousCharacter),
  ),

This means: when we hit the left arrow (and the text is selected) move the cursor to the left boundary of the selection. Otherwise move the caret to the previousCharacter.

If I want to override this behavior in my app such as skipping over an entire token, I can implement my own MoveCaretTo command that has different logic.

Just like a controller method, I could manually call it:

controller.execute(MoveCaretToSelectionBounds(
    CaretDirection.left,
    orElse: MoveCaretTo(previousCharacter),
  ))

instead of an equivalent:

controller.moveCaretToSelectionBounds(
    CaretDirection.left,
    orElse: MoveCaretTo(previousCharacter),
  )

The additional validation and interpretation would be encapsulated in those classes. For example, here's our implementation of MoveCaretTo. (It's worth noting that this command does not actually update the state of the controller. It produces a new state which the controller updates itself with.)

/// Move the caret to the given position defined by [getPosition]
///
/// Can optionally expand the selection if [expandSelection] is set to true
class MoveCaretTo extends TextEditCommand {
  const MoveCaretTo(
    this.getPosition, {
    this.expandSelection = false,
    this.debugLabel,
  });

  /// The new position to move the caret to based on the current
  /// text, selection, and state of [SuperSelectableText]
  final GetRelativeTextPosition getPosition;

  /// Whether to expand the selection rather than simply moving the caret
  /// to the destination with a collapsed state
  ///
  /// For example, in most platforms, holding SHIFT then pressing UP/DOWN/LEFT/RIGHT
  /// would move the caret and expand the selection
  final bool expandSelection;
  final String? debugLabel;

  @override
  Edit<TextEditValue> execute({
    required TextEditValue value,
    SuperSelectableTextState? state,
  }) {
    final position = getPosition(
      selectableTextState: state,
      text: value.text.text,
      position: value.selection.extent,
    );
    return SetSelection(
      previous: value.selection,
      next: expandSelection
          // Expand the selection to the calculated offset using the baseOffset as a pivot point
          ? TextSelection(
              baseOffset: value.selection.baseOffset,
              extentOffset: position.offset,
            )
          : TextSelection.collapsed(offset: position.offset),
    );
  }
}

If I wanted a controller.replaceEverything, I could implement a command like this:

class ReplaceEverything extends TextEditCommand {
  ReplaceEverything(this.text, this.selection,
      [this.composingRegion = TextRange.empty]);

  final AttributedText text;
  final TextSelection selection;
  final TextRange composingRegion;

  @override
  Edit<TextEditValue> execute({
    required TextEditValue value,
    SuperSelectableTextState? state,
  }) {
    return ReplaceValueEdit(
      prev: value,
      next: TextEditValue(text: text, selection: selection),
    );
  }
}

Then call controller.execute(ReplaceEverything(...args)) rather than controller.replaceEverything(...args)