Skip to content

Conversation

@xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented Sep 14, 2020

Description

See #65059 (comment) and #65059 (comment)

@justinmc @GaryQian Please review, thank you guys very much:)

Related Issues

Fixes #65059

Tests

See the change files.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 14, 2020

// Always attempt to send the value. If the value has changed, then it will send,
// otherwise, it will short-circuit.
_updateRemoteEditingValueIfNeeded();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be automatically called by the controller's notifier.

Copy link
Member Author

@xu-baolin xu-baolin Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be removed, see #67892

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating this issue! This was not a simple problem.

Just a small naming change and a request for an issue to be created.

log.clear();

// Currently `_receivedRemoteTextEditingValue` equals 'I will be modified by the formatter.',
// setEditingState will be called when set the [controller.value] to `_receivedRemoteTextEditingValue` by local.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky, good call testing this case.


bool _isSelectionOnlyChange(TextEditingValue value) {
return value.text == _value.text && value.composing == _value.composing;
bool _isSelectionOnlyChanged(TextEditingValue value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should stay as "Change", as I understand it. I think it meant like: "is selection the only change"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

bool _isSelectionOnlyChange(TextEditingValue value) {
return value.text == _value.text && value.composing == _value.composing;
bool _isSelectionOnlyChanged(TextEditingValue value) {
return value.text == _value.text && value.composing == _value.composing && value.selection != _value.selection;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the selection wasn't checked here in the first place. This seems to make more sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this, with the changes made in line 1650 through 1656 (i.e., the additional case handling when _value == value )?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LongCatIsLooong
Yes, you are correct. But the name of this function is ambiguous, or change the name instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears the method is only used here. You can get rid of it and use the expression instead (and maybe leave a comment that says if the change is selection only)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.
Done.

if (_isSelectionOnlyChange(value)) {
if (value == _value) {
// This is possible, for example, when the numeric keyboard is input,
// the engine will notify twice for the same value. Perhaps this is an issue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create an actual issue for this and link to it here? I just want to make sure this has the chance to get looked at, in case it is a problem. It doesn't make sense to me that the engine should notify twice for the numeric keyboard.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The track at #65811

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@override
TextEditingValue get currentTextEditingValue => _value;

bool _updateEditingValueInProgress = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR really reminds me of this issue, which also deals with the asynchronous communication between the engine and framework: #61282

My solution there wasn't anything like this PR, but I think using this flag _updateEditingValueInProgress could maybe help us handle problems like that.

@LongCatIsLooong
Copy link
Contributor

Thanks for the PR and the detailed explanation! This looks like an Android-specific issue, as I failed to reproduce this on iOS. The android input plugin seems to be not syncing back the editing state when there's a selection only change. Maybe this bug needs to be fixed from the engine side (i.e., the android input plugin)?

@xu-baolin
Copy link
Member Author

xu-baolin commented Sep 14, 2020

Thanks for the PR and the detailed explanation! This looks like an Android-specific issue, as I failed to reproduce this on iOS. The android input plugin seems to be not syncing back the editing state when there's a selection only change. Maybe this bug needs to be fixed from the engine side (i.e., the android input plugin)?

If we can ensure that all platforms(android, iOS, Windows, Web, etc.) will sync back to any local changes(including text, selection, and composing) synchronous, we can do that. But the platform channel is asynchronous communication, In theory, bugs can also be triggered within this time gap.

I also reproduce this issue at the Windows platform.
@LongCatIsLooong Hi, I don’t have an Apple device right now. Can you test if there is any problem with the iPhone? Thank you very much.

In summary, fixing this problem in the framework may be the best choice.
It is reasonable that always send changes to the engine when local changes.

Steps to Reproduce

1, Clean the textField and input 123
2, Tap set to 12
3, Tap set to 123
4, Input any key will trigger the issue, currently, the state is mismatch.
20200915_100509

Sample Code

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() => runApp(MyApp());

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Retrieve Text Input',
      home: MyCustomForm(),
    );
  }
}

// Define a custom Form widget.
class MyCustomForm extends StatefulWidget {
  @override
  _MyCustomFormState createState() => _MyCustomFormState();
}

// Define a corresponding State class.
// This class holds data related to the Form.
class _MyCustomFormState extends State<MyCustomForm> {
  // Create a text controller and use it to retrieve the current value
  // of the TextField.
  final myController = TextEditingController(text: 'I love flutter!');
  final newController = TextEditingController(text: 'I am a new controller.');
  TextEditingController _controller;

  @override
  void initState() {
    super.initState();
    _controller = myController;

    _controller.addListener(_printLatestValue);
  }

  @override
  void dispose() {
    // Clean up the controller when the widget is removed from the widget tree.
    // This also removes the _printLatestValue listener.
    myController.dispose();
    newController.dispose();
    super.dispose();
  }

  _printLatestValue() {
    print("Second text field: ${myController.text}");
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('Retrieve Text Input'),
      ),
      body: Padding(
        padding: const EdgeInsets.all(16.0),
        child: Column(
          children: <Widget>[
//            TextField(
//              onChanged: (text) {
//                print("First text field: $text");
//              },
//            ),
            TextField(
              controller: _controller,
              keyboardType: TextInputType.multiline,
              maxLines: 7,
//              inputFormatters: [LengthLimitingTextInputFormatter(3)],
            ),
            MaterialButton(
                child: Text("set to '123'"),
                onPressed: () {
                  setState(() {
                    _controller.value = TextEditingValue(
                        text: '123',
                        composing:TextRange(start: -1, end: -1),
                        selection: TextSelection(
                            baseOffset: 3,
                            extentOffset: 3,
                            affinity: TextAffinity.downstream,
                            isDirectional: false));
                  });
                }),
            MaterialButton(
                child: Text("set to '12'"),
                onPressed: () {
                  setState(() {
                    _controller.text = '12';
                  });
                }),
            MaterialButton(
                child: Text('change controller'),
                onPressed: () {
                  setState(() {
                    _controller = newController;
                  });
                }),
          ],
        ),
      ),
    );
  }
}

@xu-baolin
Copy link
Member Author

@justinmc @LongCatIsLooong I also create an issue to track #65059 (comment)

@xu-baolin xu-baolin requested a review from justinmc September 16, 2020 06:40
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I thought the text input platform plugin should send back the value until I unearthed flutter/engine@600567e. Yeah you're right we shouldn't enforce the platform text input plugin to send back the value. Thank you!

bool _isSelectionOnlyChange(TextEditingValue value) {
return value.text == _value.text && value.composing == _value.composing;
bool _isSelectionOnlyChanged(TextEditingValue value) {
return value.text == _value.text && value.composing == _value.composing && value.selection != _value.selection;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this, with the changes made in line 1650 through 1656 (i.e., the additional case handling when _value == value )?

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think this is ready to merge when the tree is green. Thanks so much for working on this!

@fluttergithubbot fluttergithubbot merged commit 0d945a1 into flutter:master Sep 17, 2020
goderbauer pushed a commit to goderbauer/flutter that referenced this pull request Sep 18, 2020
@zanderso zanderso linked an issue Sep 28, 2020 that may be closed by this pull request
christopherfujino pushed a commit to chris-forks/flutter that referenced this pull request Sep 29, 2020
christopherfujino added a commit that referenced this pull request Sep 29, 2020
* [Icons][iOS] Pointing to version of material icon font that includes more metadata in the xml. (#66684)
* apply engine cherrypicks
* Page-subclasses to take children instead of builder (#66694)
* Update pub dependencies to support dart 2.10.0
* cherry-pick 76ad864
* Fix the inconsistency between the local state of the input and the engine state (#65754)

Co-authored-by: Will Larche <larche@google.com>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: xubaolin <xubaolin@oppo.com>
@justinmc justinmc mentioned this pull request Sep 30, 2020
9 tasks
willlockwood pushed a commit to willlockwood/flutter that referenced this pull request Dec 25, 2020
* [Icons][iOS] Pointing to version of material icon font that includes more metadata in the xml. (flutter#66684)
* apply engine cherrypicks
* Page-subclasses to take children instead of builder (flutter#66694)
* Update pub dependencies to support dart 2.10.0
* cherry-pick 76ad864
* Fix the inconsistency between the local state of the input and the engine state (flutter#65754)

Co-authored-by: Will Larche <larche@google.com>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: xubaolin <xubaolin@oppo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

On some devices, native keyboard cannot be closed with back button TextField cursor misalignment when keyboardType is multiline

5 participants