-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix web EditableText Widget composing range error #25673
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -373,11 +373,18 @@ class AutofillInfo { | |
| /// The current text and selection state of a text field. | ||
| @visibleForTesting | ||
| class EditingState { | ||
| EditingState({this.text, int? baseOffset, int? extentOffset}) : | ||
| // Don't allow negative numbers. Pick the smallest selection index for base. | ||
| baseOffset = math.max(0, math.min(baseOffset ?? 0, extentOffset ?? 0)), | ||
| // Don't allow negative numbers. Pick the greatest selection index for extent. | ||
| extentOffset = math.max(0, math.max(baseOffset ?? 0, extentOffset ?? 0)); | ||
| EditingState( | ||
| {this.text, | ||
| int? baseOffset, | ||
| int? extentOffset, | ||
| this.composingBase, | ||
| this.composingExtent}) | ||
| : | ||
| // Don't allow negative numbers. Pick the smallest selection index for base. | ||
| baseOffset = math.max(0, math.min(baseOffset ?? 0, extentOffset ?? 0)), | ||
| // Don't allow negative numbers. Pick the greatest selection index for extent. | ||
| extentOffset = | ||
| math.max(0, math.max(baseOffset ?? 0, extentOffset ?? 0)); | ||
|
|
||
| /// Creates an [EditingState] instance using values from an editing state Map | ||
| /// coming from Flutter. | ||
|
|
@@ -436,13 +443,31 @@ class EditingState { | |
| } | ||
| } | ||
|
|
||
| EditingState copyWith({ | ||
| String? text, | ||
| int? baseOffset, | ||
| int? extentOffset, | ||
| int? composingBase, | ||
| int? composingExtent, | ||
| }) { | ||
| return EditingState( | ||
| text: text ?? this.text, | ||
| baseOffset: baseOffset ?? this.baseOffset, | ||
| extentOffset: extentOffset ?? this.extentOffset, | ||
| composingBase: composingBase ?? this.composingBase, | ||
| composingExtent: composingExtent ?? this.composingExtent, | ||
| ); | ||
| } | ||
|
|
||
| /// The counterpart of [EditingState.fromFrameworkMessage]. It generates a Map that | ||
| /// can be sent to Flutter. | ||
| // TODO(mdebbar): Should we get `selectionAffinity` and other properties from flutter's editing state? | ||
| Map<String, dynamic> toFlutter() => <String, dynamic>{ | ||
| 'text': text, | ||
| 'selectionBase': baseOffset, | ||
| 'selectionExtent': extentOffset, | ||
| 'composingBase': composingBase ?? -1, | ||
| 'composingExtent': composingExtent ?? -1, | ||
| }; | ||
|
|
||
| /// The current text being edited. | ||
|
|
@@ -454,11 +479,18 @@ class EditingState { | |
| /// The offset at which the text selection terminates. | ||
| final int? extentOffset; | ||
|
|
||
| /// The start range of text that is still being composed. | ||
| final int? composingBase; | ||
|
|
||
| /// The end range of text that is still being composed. | ||
| final int? composingExtent; | ||
|
|
||
| /// Whether the current editing state is valid or not. | ||
| bool get isValid => baseOffset! >= 0 && extentOffset! >= 0; | ||
|
|
||
| @override | ||
| int get hashCode => ui.hashValues(text, baseOffset, extentOffset); | ||
| int get hashCode => ui.hashValues( | ||
| text, baseOffset, extentOffset, composingBase, composingExtent); | ||
|
|
||
| @override | ||
| bool operator ==(Object other) { | ||
|
|
@@ -471,7 +503,9 @@ class EditingState { | |
| return other is EditingState && | ||
| other.text == text && | ||
| other.baseOffset == baseOffset && | ||
| other.extentOffset == extentOffset; | ||
| other.extentOffset == extentOffset && | ||
| other.composingBase == composingBase && | ||
| other.composingExtent == composingExtent; | ||
| } | ||
|
|
||
| @override | ||
|
|
@@ -772,6 +806,8 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
| bool get appendedToForm => _appendedToForm; | ||
| bool _appendedToForm = false; | ||
|
|
||
| String? composingText; | ||
|
|
||
| html.FormElement? get focusedFormElement => | ||
| _inputConfiguration.autofillGroup?.formElement; | ||
|
|
||
|
|
@@ -849,6 +885,10 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
| domElement.focus(); | ||
| })); | ||
|
|
||
| domElement.addEventListener('compositionstart', _handleCompositionStart); | ||
| domElement.addEventListener('compositionupdate', _handleCompositionChange); | ||
| domElement.addEventListener('compositionend', _handleCompositionEnd); | ||
|
|
||
|
Comment on lines
+888
to
+891
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same needs to be repeated in a few places. Let's put it in a method: html.EventListener? compositionStart;
html.EventListener? compositionUpdate;
html.EventListener? compositionEnd;
void addCompositionHandlers() {
compositionStart = _handleCompositionStart;
compositionUpdate = _handleCompositionUpdate;
compositionEnd = _handleCompositionEnd;
domElement.addEventListener('compositionstart', compositionStart);
domElement.addEventListener('compositionupdate', compositionChange);
domElement.addEventListener('compositionend', compositionEnd);
}and call this method in all subclass of |
||
| preventDefaultForMouseEvents(); | ||
| } | ||
|
|
||
|
|
@@ -882,6 +922,12 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
| _subscriptions[i].cancel(); | ||
| } | ||
| _subscriptions.clear(); | ||
|
|
||
| domElement.removeEventListener('compositionstart', _handleCompositionStart); | ||
| domElement.removeEventListener( | ||
| 'compositionupdate', _handleCompositionChange); | ||
| domElement.removeEventListener('compositionend', _handleCompositionEnd); | ||
|
Comment on lines
+926
to
+929
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the suggestion above, let's put this into a method: void removeCompositionHandlers() {
domElement.removeEventListener('compositionstart', compositionStart);
domElement.removeEventListener(
'compositionupdate', compositionChange);
domElement.removeEventListener('compositionend', compositionEnd);
}and call from this |
||
|
|
||
| // If focused element is a part of a form, it needs to stay on the DOM | ||
| // until the autofill context of the form is finalized. | ||
| // More details on `TextInput.finishAutofillContext` call. | ||
|
|
@@ -921,13 +967,43 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
|
|
||
| EditingState newEditingState = EditingState.fromDomElement(domElement, | ||
| textCapitalization: _inputConfiguration.textCapitalization); | ||
| if (composingText != null && newEditingState.text != null) { | ||
| int composingBase = newEditingState.text!.lastIndexOf(composingText!); | ||
| if (composingBase >= 0) { | ||
| newEditingState = newEditingState.copyWith( | ||
| composingBase: composingBase, | ||
| composingExtent: composingBase + composingText!.length); | ||
| } | ||
| } | ||
|
Comment on lines
+970
to
+977
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall, the PR looks good, except this part. Doing a From a quick try, I noticed that Instead of doing a final int compositionExtent = newEditingState.baseOffset
final int compositionBase = compositionExtent - composingText.length; |
||
|
|
||
| if (newEditingState != _lastEditingState) { | ||
| _lastEditingState = newEditingState; | ||
| _onChange!(_lastEditingState); | ||
| } | ||
| } | ||
|
|
||
| void _handleCompositionStart(html.Event event) { | ||
| if (event is html.CompositionEvent) { | ||
| composingText = null; | ||
| } | ||
| } | ||
|
|
||
| void _handleCompositionChange(html.Event event) { | ||
| if (event is html.CompositionEvent) { | ||
| composingText = event.data; | ||
| } | ||
| } | ||
|
|
||
| void _handleCompositionEnd(html.Event event) { | ||
| if (event is html.CompositionEvent) { | ||
| composingText = null; | ||
| EditingState newEditingState = EditingState.fromDomElement(domElement, | ||
| textCapitalization: _inputConfiguration.textCapitalization); | ||
| _lastEditingState = newEditingState; | ||
| _onChange!(_lastEditingState); | ||
| } | ||
| } | ||
|
|
||
| void _maybeSendAction(html.Event event) { | ||
| if (event is html.KeyboardEvent) { | ||
| if (_inputConfiguration.inputType.submitActionOnEnter && | ||
|
|
@@ -1377,7 +1453,8 @@ class TextEditingChannel { | |
| break; | ||
|
|
||
| default: | ||
| EnginePlatformDispatcher.instance._replyToPlatformMessage(callback, null); | ||
| EnginePlatformDispatcher.instance | ||
| ._replyToPlatformMessage(callback, null); | ||
| return; | ||
| } | ||
| EnginePlatformDispatcher.instance | ||
|
|
@@ -1483,7 +1560,7 @@ class HybridTextEditing { | |
| } else if (browserEngine == BrowserEngine.webkit) { | ||
| this._defaultEditingElement = SafariDesktopTextEditingStrategy(this); | ||
| } else if ((browserEngine == BrowserEngine.blink || | ||
| browserEngine == BrowserEngine.samsung) && | ||
| browserEngine == BrowserEngine.samsung) && | ||
| operatingSystem == OperatingSystem.android) { | ||
| this._defaultEditingElement = AndroidTextEditingStrategy(this); | ||
| } else if (browserEngine == BrowserEngine.firefox) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1577,7 +1577,9 @@ void testMain() { | |
| <String, dynamic>{ | ||
| 'text': 'something', | ||
| 'selectionBase': 9, | ||
| 'selectionExtent': 9 | ||
| 'selectionExtent': 9, | ||
| 'composingBase': -1, | ||
| 'composingExtent': -1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a test (or more) that sends a few composition events and makes sure they are handled correctly. |
||
| } | ||
| ], | ||
| ); | ||
|
|
@@ -1601,7 +1603,9 @@ void testMain() { | |
| <String, dynamic>{ | ||
| 'text': 'something', | ||
| 'selectionBase': 2, | ||
| 'selectionExtent': 5 | ||
| 'selectionExtent': 5, | ||
| 'composingBase': -1, | ||
| 'composingExtent': -1 | ||
| } | ||
| ], | ||
| ); | ||
|
|
@@ -1674,7 +1678,9 @@ void testMain() { | |
| hintForFirstElement: <String, dynamic>{ | ||
| 'text': 'something', | ||
| 'selectionBase': 9, | ||
| 'selectionExtent': 9 | ||
| 'selectionExtent': 9, | ||
| 'composingBase': -1, | ||
| 'composingExtent': -1 | ||
| } | ||
| }, | ||
| ], | ||
|
|
@@ -1733,6 +1739,8 @@ void testMain() { | |
| 'text': 'something\nelse', | ||
| 'selectionBase': 14, | ||
| 'selectionExtent': 14, | ||
| 'composingBase': -1, | ||
| 'composingExtent': -1 | ||
| } | ||
| ], | ||
| ); | ||
|
|
@@ -1747,6 +1755,8 @@ void testMain() { | |
| 'text': 'something\nelse', | ||
| 'selectionBase': 2, | ||
| 'selectionExtent': 5, | ||
| 'composingBase': -1, | ||
| 'composingExtent': -1 | ||
| } | ||
| ], | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set a default value of
-1here?