Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@JsouLiang
Copy link
Contributor

Fix Issue: 98352

In current case is not cursor selector but current logic number input is will check the Selection range

When the TextInputType is Number, and you input text, the keyboard will sendKeyEvent, and next entry the handleKeyEvent method

public boolean handleKeyEvent(KeyEvent event) {
   ...
        // Enter a character.
        final int selStart = Selection.getSelectionStart(mEditable);
        final int selEnd = Selection.getSelectionEnd(mEditable);
        final int character = event.getUnicodeChar();
        if (selStart < 0 || selEnd < 0 || character == 0) {
          return false;
        }
...
  }

When first number entry this method the selStart and selEnd will be -1, because there is no cursor and thus the Selection.start is not attach. The getSelectionStart will return -1

We need to support logic that without a cursor, and need support delete action.

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.

@JsouLiang JsouLiang force-pushed the Android-TextInputType-Phone-Number-Input-Failed branch from 6412d4e to 8c77cbc Compare February 28, 2022 02:28
final int selStart = Selection.getSelectionStart(mEditable);
final int selEnd = Selection.getSelectionEnd(mEditable);
final int character = event.getUnicodeChar();
if (selStart < 0 || selEnd < 0 || character == 0) {
Copy link

Choose a reason for hiding this comment

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

is this case valid now, or just not possible because of the KEYCODE_DEL case above?


final int selMin = Math.min(selStart, selEnd);
final int selMax = Math.max(selStart, selEnd);
final int selMin = Math.max(Math.min(selStart, selEnd), 0);
Copy link

Choose a reason for hiding this comment

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

related to the question above, when is selStart or selEnd < 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this issue case, the user use an TextInputConfiguration to connect the keyboard, but not use TextFiled, when the keyboard is number it will go to the handleKeyEvent method and cause is input a number then in to the else branch in this case the selStart and selEnd will < 0 that cause the bug

final int selMin = Math.max(Math.min(selStart, selEnd), 0);
final int selMax = Math.max(Math.max(selStart, selEnd), 0);
beginBatchEdit();
if (selMin != selMax) mEditable.delete(selMin, selMax);
Copy link

Choose a reason for hiding this comment

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

is this case still relevant?

return handleVerticalMovement(false, event.isShiftPressed());
// When the enter key is pressed on a non-multiline field, consider it a
// submit instead of a newline.
} else if (event.getKeyCode() == KeyEvent.KEYCODE_DEL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the delete key not working? I think it's currently handled by the framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the delete key not working in this case flutter/flutter#98352, you can use the simple code to verify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use this demo, the framework doesn't handle the case of use input delete event. And in the engine doesn't handle it also.

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 meant to be handled by the framework code here: https://github.com/flutter/flutter/blob/ffe64c6336bfb0ad68436af904e6e1f28724dd5f/packages/flutter/lib/src/widgets/default_text_editing_shortcuts.dart#L165-L174

Could you elaborate a bit on why the framework isn't handling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we use TextFiled widget that will make overridable in here https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/editable_text.dart#L3120 and when we input Delete that will handle in method handleKaypress: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/shortcuts.dart#L717, and the action https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/shortcuts.dart#L723 will be a instance of _OverridableContextAction<Intent>

But in this case, customer doesn't use TextFiled, only use a TextInputConfigure, when you input delete, the handleKaypress action will be null.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Mar 14, 2022

Choose a reason for hiding this comment

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

Ah I see what you're saying. The sample code provided doesn't make use of EditableText and implements TextInputClient instead. However this is not recommended as most text editing key bindings should be handled by the framework (text layout isn't available to the engine so text input plugin can't reliably handle deleting to the beginning of the line reliably, for example), and I'm hoping that we can remove/deprecate this handleKeyEvent method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, do you mean that the problem is caused by the way the example is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, ideally the delete action needs to be handled by dart code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If delete the handleKey of Delete the user can't delete character. And do you have some suggestion to hold the delete character behavior?

InputConnectionAdaptor adaptor = sampleInputConnectionAdaptor(editable);

KeyEvent downKeyDown = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL);
KeyEvent downKeyUP = new KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL);
Copy link

Choose a reason for hiding this comment

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

This test was added by @gspencergoog in #22626.
Is this change correct, Greg?

@chinmaygarde
Copy link
Member

I think there is an open question for @LongCatIsLooong

@zanderso
Copy link
Member

From PR review triage: There are open questions for @JsouLiang and @gspencergoog

@zanderso
Copy link
Member

zanderso commented Apr 7, 2022

@JsouLiang Hi there again from PR triage. Are you interested in continuing with this PR? If so, it looks like there are some open questions for you. Thanks!

@JsouLiang
Copy link
Contributor Author

@zanderso hey, yes i am interested in continuing with this PR, but i have some problems need to ask @LongCatIsLooong. Thanks.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Apr 14, 2022
@chinmaygarde
Copy link
Member

cc @LongCatIsLooong Are you able to reply to @JsouLiang query?

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Aug 2, 2022

Sorry for the delayed reply. @JsouLiang looks like the handleDelete implementation in the pull request doesn't handle surrogate pairs or combining characters. The logic that handles that currently lives in the framework since that's much easier to maintain than implementing/maintaining the same thing for each platform. There's ongoing work to expose more things to help developers build their own TextInputClients from scratch but for now should they choose to implement TextInputClient themselves, they'll have to handle delete themselves.

@JsouLiang JsouLiang closed this Aug 16, 2022
@nhanvhn
Copy link

nhanvhn commented Jul 6, 2023

Is this any update? I still met this issue on android with Flutter v3.7.12

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants