Fix web editable text composing range

[antholeole]

Flutter framework on web was not getting composingRange updates from the engine. This is the fix. Loosely based on this PR.

I attempted to pull most of the composing logic out into it's own mixin, so it would be easier to test + read.

before:

• note that composing range never updates - it stays -1, -1 regardless of composing.

after:

• composing range is -1, -1 when not composing.
• composing range is the correct range when composing, including in the middle of text.

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

• Fixes TextInputClient should receive correct composing range or ignore it on Flutter Web flutter#65357
• Fixes [Web] IME-based text input incorrectly fails to capture first character in composing region flutter#103645

Demo Code

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Material App',
      home: Scaffold(
        appBar: AppBar(
          title: Text('Material App Bar'),
        ),
        body: CustomInput(),
      ),
    );
  }
}

class CustomInput extends StatefulWidget {
  @override
  _CustomInputState createState() => _CustomInputState();
}

class _CustomInputState extends State<CustomInput>
    with AutomaticKeepAliveClientMixin {
  TextInputConnection? _textInputConnection;
  FocusNode? _focusNode;
  FocusAttachment? _focusAttachment;
  InputConnectionController? _input;

  int composingStart = -1;
  int composingEnd = -1;
  String text = '';

  void requestKeyboard() {
    if (_focusNode!.hasFocus) {
      _input!.openConnection(Brightness.dark);
    } else {
      FocusScope.of(context).requestFocus(_focusNode);
    }
  }

  void focusOrUnfocusIfNeeded() {
    if (_focusNode!.hasFocus) {
      _focusNode!.unfocus();
    }
  }

  @override
  bool get wantKeepAlive => _focusNode!.hasFocus;

  void _handleFocusChange() {
    if (_focusNode!.hasFocus && _focusNode!.consumeKeyboardToken()) {
      _input!.openConnection(Brightness.dark);
    } else if (!_focusNode!.hasFocus) {
      _input!.closeConnection();
    }
    updateKeepAlive();
  }

  bool get _hasFocus => _focusNode!.hasFocus;

  bool get _hasInputConnection =>
      _textInputConnection != null && _textInputConnection!.attached;

  @override
  void initState() {
    _focusNode = FocusNode();
    super.initState();
    _focusAttachment = _focusNode!.attach(context);
    _input = InputConnectionController(
        onUpdatedEditingValue: (start, end, inputtedText) {
      setState(() {
        composingEnd = end;
        composingStart = start;
        text = inputtedText;
      });
    });
    _focusNode!.addListener(_handleFocusChange);
  }

  @override
  void dispose() {
    _focusAttachment!.detach();
    _focusNode!.removeListener(_handleFocusChange);
    _input!.closeConnection();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    _focusAttachment!.reparent();
    super.build(context); // See AutomaticKeepAliveState.

    return GestureDetector(
      onTap: () => _focusNode!.requestFocus(),
      child: Column(
        children: [
          Container(
            width: 200,
            height: 200,
            color: Colors.red,
          ),
          Text('composing range: $composingStart, $composingEnd'),
          Text(text)
        ],
      ),
    );
  }
}

class InputConnectionController extends TextInputClient {
  void Function(int from, int to, String value) onUpdatedEditingValue;

  InputConnectionController({required this.onUpdatedEditingValue});

  TextInputConnection? _textInputConnection;

  bool get hasConnection =>
      _textInputConnection != null && _textInputConnection!.attached;

  void openOrCloseConnection(
      FocusNode focusNode, Brightness keyboardAppearance) {
    if (focusNode.hasFocus && focusNode.consumeKeyboardToken()) {
      openConnection(keyboardAppearance);
    } else if (!focusNode.hasFocus) {
      closeConnection();
    }
  }

  void openConnection(Brightness keyboardAppearance) {
    if (!hasConnection) {
      _textInputConnection = TextInput.attach(
        this,
        TextInputConfiguration(
          inputType: TextInputType.multiline,
          obscureText: false,
          autocorrect: false,
          inputAction: TextInputAction.newline,
          keyboardAppearance: keyboardAppearance,
          textCapitalization: TextCapitalization.sentences,
        ),
      );
    }
    _textInputConnection!.show();
  }

  void closeConnection() {
    if (hasConnection) {
      _textInputConnection!.close();
      _textInputConnection = null;
    }
  }

  @override
  void connectionClosed() {
    _textInputConnection!.connectionClosedReceived();
    _textInputConnection = null;
  }

  @override
  AutofillScope? get currentAutofillScope => null;

  @override
  TextEditingValue? get currentTextEditingValue => null;

  @override
  void performAction(TextInputAction action) {}

  @override
  void updateEditingValue(TextEditingValue value) {
    onUpdatedEditingValue(
        value.composing.start, value.composing.end, value.text);
  }

  @override
  void performPrivateCommand(String action, Map<String, dynamic> data) {}

  @override
  void insertTextPlaceholder(Size size) {
    // TODO: implement insertTextPlaceholder
  }

  @override
  void removeTextPlaceholder() {
    // TODO: implement removeTextPlaceholder
  }

  @override
  void showToolbar() {
    // TODO: implement showToolbar
  }

  @override
  void showAutocorrectionPromptRect(int start, int end) {}

  @override
  void updateFloatingCursor(RawFloatingCursorPoint point) {
    // TODO: implement updateFloatingCursor
  }
}

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.

[mdebbar]

Other than my concern about delta state, this PR looks good!

Thanks for taking the time to fix this!

[mdebbar]

I honestly don't know how delta state works with composing range. Your change here seems to break delta state because it won't be able to take composing range into account anymore:

engine/lib/web_ui/lib/src/engine/text_editing/text_editing.dart

Lines 506 to 512 in 593140b

	// If we are composing then set the delta range to the composing region we
	// captured in compositionupdate.
	final bool isCurrentlyComposing = newTextEditingDeltaState.composingOffset != null && newTextEditingDeltaState.composingOffset != newTextEditingDeltaState.composingExtent;
	if (newTextEditingDeltaState.deltaText.isNotEmpty && previousSelectionWasCollapsed && isCurrentlyComposing) {
	newTextEditingDeltaState.deltaStart = newTextEditingDeltaState.composingOffset!;
	newTextEditingDeltaState.deltaEnd = newTextEditingDeltaState.composingExtent!;
	}

I'd love to get @Renzo-Olivares's opinion on this one.

[Renzo-Olivares]

It seems to be causing some issues, but i'm still looking into it. Ideally we could just pull the composingOffset and composingExtent from the EditingState after determineCompositionState() is called, but I'm getting some weird behavior doing that. I'll keep investigating, and post an update asap. I'll also update the documentation since this should be more clearly explained.

[Renzo-Olivares]

We were previously setting the deltaStart and deltaEnd to the composing region captured in compositionupdate. I previously derived the composing region by grabbing the editing state from the DOM at the time when the compositionupdate event was fired and using the selection as the composing region. At the time of firing the compositionupdate event, the editing states selection contains the range that was replaced during composition. Previously this worked fine because we were replacing the replaced range with the composed text. However, this was a bit inaccurate since it is not the composing region after the edit was made. When making the composing region accurate through this change, a failure occurs in the _replace method because the range being replaced did not exist yet. For example when composing:

Type: ’n’
oldText = ‘’
deltaText = ’n’
deltaStart = ‘0’ (from composingOffset)
deltaEnd = ‘1’ (from composingExtent)

_replace tries to replace (0,1) with ’n’ in oldText which is an empty string. This fails and a delta is not produced. The reason for this being, by deriving the composing region from compositionupdate using CompositionAwareMixin we are actually deriving the composing region after that composing step has finished, i.e. after ’n’ has already been added to the EditingState the composing region is (0,1).

To fix this the deltaStart and deltaEnd should be the range replaced by the composing region, and not the composing region after the compositionupdate event has updated the editing state. We can do this by setting the deltaStart to the composingOffset captured by the CompositionAwareMixin and leaving the deltaEnd as the one captured by beforeInput. This will be the position of the caret before the compositionupdate event is fired. In the case of our example above it would be (0). So coming back to our previous example we would have:

Type: ’n’
oldText = ‘’
deltaText = ’n’
deltaStart = ‘0’ (from composingOffset)
deltaEnd = ‘0’ (from beforeInput extentOffset)

Which would not cause a failure on _replace and generate the correct delta.

The fix would look something like Renzo-Olivares@f459888 .

I'm unsure if we want to rely on beforeInput or if we want to rely on 'CompositionAwareMixin’ to save the selection range from the active DOM editing state in compositionupdate and set the composing region and deltaStart and deltaEnd through something like determineCompositionStateForDelta().

[justinmc]

Just small comments below, overall this looks good. +1 to having @Renzo-Olivares take a look as it pertains to deltas.

[antholeole]

@Renzo-Olivares This test group are the tests for the delta fix you had me implement - lmk your input

[Renzo-Olivares]

LGTM thanks for making these

[antholeole]

Unfortunately, the failing test is not a flake - I'm able to reproduce the behavior on Firefox. Here's a video of what is happening on Firefox:

delta_bug.mov

essentially, it looks like on composing using delta mode on Firefox, a second action is required to "flush" the composed region. Not sure why this is, but it's consistent with the tests: the test failing on Firefox passes if I send a dummy event after composition.

This is looking like a much more involved fix than originally planned because of deltas - I can continue trying to monkey patch this, but not sure if it will be beneficial, since Renzo said that deltas are getting refactored soon.

Thoughts?

EDIT: Spoke offline w/ Renzo and Mouad. This is probably safe to merge because the bug reproduces on master, meaning it doesn't introduce a regression. The test is failing because it was previously a blind spot in the testing suite.

[mdebbar]

@antholeole thanks again for being patient here.

When I spoke to Renzo last week, we agreed that the refactoring was a good idea. I suggest waiting for that refactor. It should make this PR a lot simpler. What do you think @Renzo-Olivares ?

[Renzo-Olivares]

@antholeole thanks again for being patient here.

When I spoke to Renzo last week, we agreed that the refactoring was a good idea. I suggest waiting for that refactor. It should make this PR a lot simpler. What do you think @Renzo-Olivares ?

We talked offline, but I think this can be merged since it is reproducible on master. The source of the issue is a difference in events given by Chrome and Firefox flutter/flutter#105244 (comment)

[mdebbar]

[justinmc]

LGTM 👍

Just a bunch of small comments below.

[antholeole]

🤞