-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TextEditingDelta Support for the Web #28527
Changes from 10 commits
86885dd
9ef2c43
b93b043
d3201a1
9f99a66
8d69317
3ba145c
728055a
efbdf5b
1c330f3
0dd249a
37b757e
fd1396f
cb6ef96
4f610e0
f772bf1
f6e4d32
704d4f9
577aa59
0dcf1f2
a2e5525
95fe97b
5921215
a2a8b9d
1fbab86
928f186
03d63c3
51eabc8
125580e
26668cb
5a33dbf
e47a3ed
04020c2
13ca351
73c1411
be578c3
2df95b0
5e2b294
ac3407b
fa1d886
6ced401
15868f6
059fe74
26e676a
9196226
38bc244
cc0bb16
b7a91da
a600e9e
98fad47
786a528
3f260ce
b9db35a
41765f8
8ff4f2d
f4a858a
cbb6b6b
b596600
5a8a5f7
6645a0b
da90e99
6217ea9
b5e0b55
7a5aff9
c4c9cf6
9d97c88
0fc279d
fc823e9
6bf2ca7
acbc0de
7722380
0bb7f30
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 |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
import 'dart:async'; | ||
import 'dart:html' as html; | ||
import 'dart:js_util' as js_util; | ||
import 'dart:math' as math; | ||
import 'dart:typed_data'; | ||
|
||
|
@@ -439,6 +440,142 @@ class AutofillInfo { | |
} | ||
} | ||
|
||
/// Replaces a range of text in the original string with the text given in the | ||
/// replacement string. | ||
String _replace(String originalText, String replacementText, int start, int end) { | ||
final String textStart = originalText.substring(0, start); | ||
final String textEnd = originalText.substring(end, originalText.length); | ||
final String newText = textStart + replacementText + textEnd; | ||
return newText; | ||
} | ||
|
||
class TextEditingDeltaState { | ||
const TextEditingDeltaState({ | ||
this.oldText = '', | ||
this.deltaText = '', | ||
this.deltaStart = -1, | ||
this.deltaEnd = -1, | ||
this.baseOffset = -1, | ||
this.extentOffset = -1, | ||
this.composingOffset = -1, | ||
this.composingExtent = -1, | ||
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. Why are the names slightly different than the frameworks' TextDelta, is it to match how the browser's events present the information? 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. I just tried to match the 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. Are the empty strings and -1 meaningful values, or are they here only to make the fields non-null? Would it make sense to mark the parameters with 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.
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. On the framework side |
||
}); | ||
|
||
static TextEditingDeltaState inferDeltaState(EditingState newEditingState, EditingState? lastEditingState, TextEditingDeltaState? lastTextEditingDeltaState) { | ||
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. Would it make this any simpler if you used inputType? Or is that not possible... 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. I attempted to use For example, when trying to use |
||
TextEditingDeltaState newTextEditingDeltaState = lastTextEditingDeltaState ?? TextEditingDeltaState(oldText: lastEditingState!.text!); | ||
|
||
final bool previousSelectionWasCollapsed = lastEditingState?.baseOffset == lastEditingState?.extentOffset; | ||
|
||
if (newTextEditingDeltaState.deltaText.isEmpty && newTextEditingDeltaState.deltaEnd != -1) { | ||
// We are removing text. | ||
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. nit: consider extracting complex conditions into named variables. Less needs to be explained this way. E.g.: final bool isTextBeingRemoved = newTextEditingDeltaState.deltaText.isEmpty && newTextEditingDeltaState.deltaEnd != -1;
if (isTextBeingRemoved) {
... |
||
final int deletedLength = newTextEditingDeltaState.oldText.length - newEditingState.text!.length; | ||
newTextEditingDeltaState = newTextEditingDeltaState.copyWith(deltaStart: newTextEditingDeltaState.deltaEnd - deletedLength); | ||
} else if (newTextEditingDeltaState.deltaText.isNotEmpty && !previousSelectionWasCollapsed) { | ||
// We are replacing text at a selection. | ||
newTextEditingDeltaState = newTextEditingDeltaState.copyWith(deltaStart: lastEditingState!.baseOffset); | ||
} | ||
|
||
// If we are composing then set the delta range to the composing region we captured in compositionupdate. | ||
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. Nit: Line length. |
||
final bool isCurrentlyComposing = newTextEditingDeltaState.composingOffset != -1 && newTextEditingDeltaState.composingOffset != newTextEditingDeltaState.composingExtent; | ||
if (newTextEditingDeltaState.deltaText.isNotEmpty && previousSelectionWasCollapsed && isCurrentlyComposing) { | ||
newTextEditingDeltaState = newTextEditingDeltaState.copyWith(deltaStart: newTextEditingDeltaState.composingOffset, deltaEnd: newTextEditingDeltaState.composingExtent); | ||
} | ||
|
||
final bool isDeltaRangeEmpty = newTextEditingDeltaState.deltaStart == -1 && newTextEditingDeltaState.deltaStart == newTextEditingDeltaState.deltaEnd; | ||
if (!isDeltaRangeEmpty) { | ||
// To verify the range of our delta we should compare the newEditingState's | ||
// text with the delta applied to the oldText. If they differ then capture | ||
// the correct delta range from the newEditingState's text value. | ||
// | ||
// We can assume the deltaText for additions and replacements to the text value | ||
// are accurate. What may not be accurate is the range of the delta. | ||
// | ||
// We can think of the newEditingState as our source of truth. | ||
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. Is this whole comment explaining the situation of double space inserting a period? Maybe add that as an example in the comment somewhere. And/or, be sure that this is thoroughly tested so that future developers can understand your intention (and not break it). 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. Add a test for this case and the accent menu case. |
||
final String textAfterDelta = _replace( | ||
newTextEditingDeltaState.oldText, newTextEditingDeltaState.deltaText, | ||
newTextEditingDeltaState.deltaStart, | ||
newTextEditingDeltaState.deltaEnd); | ||
final bool isDeltaVerified = textAfterDelta == newEditingState.text!; | ||
|
||
if (!isDeltaVerified) { | ||
// 1. Find all matches for deltaText. | ||
// 2. Apply matches/replacement to oldText until oldText matches the | ||
// new editing state's text value. | ||
final RegExp deltaTextPattern = RegExp(r'' + newTextEditingDeltaState.deltaText + r''); | ||
for (final Match match in deltaTextPattern.allMatches(newEditingState.text!)) { | ||
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. So in the example of double space inserting a period, the matches here will be all periods in the text, and you're trying to find the one that is new? 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. in the case of double space inserting a period the When we have the matched ranges we iterate through them and apply the |
||
String textAfterMatch; | ||
int actualEnd; | ||
final bool isMatchWithinOldTextBounds = match.start >= 0 && match.end <= newTextEditingDeltaState.oldText.length; | ||
if (!isMatchWithinOldTextBounds) { | ||
actualEnd = match.start + newTextEditingDeltaState.deltaText.length - 1; | ||
textAfterMatch = _replace( | ||
newTextEditingDeltaState.oldText, | ||
newTextEditingDeltaState.deltaText, | ||
match.start, | ||
actualEnd, | ||
); | ||
} else { | ||
actualEnd = match.end; | ||
textAfterMatch = _replace( | ||
newTextEditingDeltaState.oldText, | ||
newTextEditingDeltaState.deltaText, | ||
match.start, | ||
actualEnd, | ||
); | ||
} | ||
|
||
if (textAfterMatch == newEditingState.text!) { | ||
newTextEditingDeltaState = newTextEditingDeltaState.copyWith(deltaStart: match.start, deltaEnd: actualEnd); | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
|
||
return newTextEditingDeltaState; | ||
} | ||
|
||
final String oldText; | ||
final String deltaText; | ||
final int deltaStart; | ||
final int deltaEnd; | ||
final int baseOffset; | ||
final int extentOffset; | ||
final int composingOffset; | ||
final int composingExtent; | ||
|
||
Map<String, dynamic> toFlutter() => <String, dynamic>{ | ||
'oldText': oldText, | ||
'deltaText': deltaText, | ||
'deltaStart': deltaStart, | ||
'deltaEnd': deltaEnd, | ||
'selectionBase': baseOffset, | ||
'selectionExtent': extentOffset, | ||
}; | ||
|
||
TextEditingDeltaState copyWith({ | ||
String? oldText, | ||
String? deltaText, | ||
int? deltaStart, | ||
int? deltaEnd, | ||
int? baseOffset, | ||
int? extentOffset, | ||
int? composingOffset, | ||
int? composingExtent, | ||
}) { | ||
return TextEditingDeltaState( | ||
oldText: oldText ?? this.oldText, | ||
deltaText: deltaText ?? this.deltaText, | ||
deltaStart: deltaStart ?? this.deltaStart, | ||
deltaEnd: deltaEnd ?? this.deltaEnd, | ||
baseOffset: baseOffset ?? this.baseOffset, | ||
extentOffset: extentOffset ?? this.extentOffset, | ||
composingOffset: composingOffset ?? this.composingOffset, | ||
composingExtent: composingExtent ?? this.composingExtent, | ||
); | ||
} | ||
} | ||
|
||
/// The current text and selection state of a text field. | ||
class EditingState { | ||
EditingState({this.text, int? baseOffset, int? extentOffset}) : | ||
|
@@ -610,6 +747,7 @@ class InputConfiguration { | |
const TextCapitalizationConfig.defaultCapitalization(), | ||
this.autofill, | ||
this.autofillGroup, | ||
this.enableDeltaModel = false, | ||
}); | ||
|
||
InputConfiguration.fromFrameworkMessage( | ||
|
@@ -633,7 +771,8 @@ class InputConfiguration { | |
autofillGroup = EngineAutofillForm.fromFrameworkMessage( | ||
flutterInputConfiguration.tryJson('autofill'), | ||
flutterInputConfiguration.tryList('fields'), | ||
); | ||
), | ||
enableDeltaModel = flutterInputConfiguration.tryBool('enableDeltaModel') ?? false; | ||
|
||
/// The type of information being edited in the input control. | ||
final EngineInputType inputType; | ||
|
@@ -658,14 +797,16 @@ class InputConfiguration { | |
/// supported by Safari. | ||
final bool autocorrect; | ||
|
||
final bool enableDeltaModel; | ||
|
||
final AutofillInfo? autofill; | ||
|
||
final EngineAutofillForm? autofillGroup; | ||
|
||
final TextCapitalizationConfig textCapitalization; | ||
} | ||
|
||
typedef OnChangeCallback = void Function(EditingState? editingState); | ||
typedef OnChangeCallback = void Function(EditingState? editingState, TextEditingDeltaState? editingDeltaState); | ||
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. I think it makes sense to add this parameter rather than creating a new type as long as nothing is broken by it. |
||
typedef OnActionCallback = void Function(String? inputAction); | ||
|
||
/// Provides HTML DOM functionality for editable text. | ||
|
@@ -849,6 +990,8 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
late InputConfiguration inputConfiguration; | ||
EditingState? lastEditingState; | ||
|
||
TextEditingDeltaState? lastTextEditingDeltaState; | ||
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. If I understand correctly, you are using
TextEditingDeltaState? _editingDelta;
TextEditingDeltaState get editingDelta {
if (_editingDelta == null) {
_editingDelta = TextEditingDeltaState(oldText: lastEditingState!.text!);
}
return _editingDelta!;
} Then you can simply do: editingDelta.deltaStart = ...
editingDelta.deltaText = ... And when the change has been flushed to the framework, just set the delta to null: _editingDelta = null; 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. Hey @mdebbar thank you for the reviews! I wasn't able to get to this in the past few days but I will come back to it and address all these comments. Thanks again! |
||
|
||
/// Styles associated with the editable text. | ||
EditableTextStyle? style; | ||
|
||
|
@@ -947,6 +1090,10 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
|
||
subscriptions.add(html.document.onSelectionChange.listen(handleChange)); | ||
|
||
activeDomElement.addEventListener('beforeinput', handleBeforeInput); | ||
|
||
activeDomElement.addEventListener('compositionupdate', handleCompositionUpdate); | ||
|
||
// Refocus on the activeDomElement after blur, so that user can keep editing the | ||
// text field. | ||
subscriptions.add(activeDomElement.onBlur.listen((_) { | ||
|
@@ -978,6 +1125,7 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
|
||
isEnabled = false; | ||
lastEditingState = null; | ||
lastTextEditingDeltaState = null; | ||
style = null; | ||
geometry = null; | ||
|
||
|
@@ -1022,13 +1170,40 @@ abstract class DefaultTextEditingStrategy implements TextEditingStrategy { | |
assert(isEnabled); | ||
|
||
final EditingState newEditingState = EditingState.fromDomElement(activeDomElement); | ||
final TextEditingDeltaState newTextEditingDeltaState = TextEditingDeltaState.inferDeltaState(newEditingState, lastEditingState, lastTextEditingDeltaState); | ||
|
||
if (newEditingState != lastEditingState) { | ||
lastEditingState = newEditingState; | ||
onChange!(lastEditingState); | ||
lastTextEditingDeltaState = newTextEditingDeltaState; | ||
onChange!(lastEditingState, lastTextEditingDeltaState); | ||
// Flush delta after it has been sent to framework. | ||
lastTextEditingDeltaState = TextEditingDeltaState(oldText: lastEditingState!.text!); | ||
} | ||
} | ||
|
||
void handleBeforeInput(html.Event event) { | ||
String eventData = js_util.getProperty(event, 'data').toString(); | ||
final TextEditingDeltaState newDeltaState = lastTextEditingDeltaState ?? TextEditingDeltaState(oldText: lastEditingState!.text!); | ||
|
||
if (eventData == 'null') { | ||
// When event.data is 'null' we have a deletion. | ||
// The deltaStart is set in handleChange because there is where we get access | ||
// to the new selection baseOffset which is our new deltaStart. | ||
lastTextEditingDeltaState = newDeltaState.copyWith(oldText: lastEditingState!.text, deltaText: '', deltaEnd: lastEditingState!.extentOffset); | ||
} else { | ||
// When event.data is not 'null' we we will begin by considering this delta as an insertion | ||
// at the selection extentOffset. This may change due to logic in handleChange to handle | ||
// composition and other IME behaviors. | ||
lastTextEditingDeltaState = newDeltaState.copyWith(oldText: lastEditingState!.text, deltaText: eventData, deltaStart: lastEditingState!.extentOffset, deltaEnd: lastEditingState!.extentOffset); | ||
} | ||
} | ||
|
||
void handleCompositionUpdate(html.Event event) { | ||
final EditingState newEditingState = EditingState.fromDomElement(activeDomElement); | ||
final TextEditingDeltaState newDeltaState = lastTextEditingDeltaState ?? TextEditingDeltaState(oldText: lastEditingState!.text!); | ||
lastTextEditingDeltaState = newDeltaState.copyWith(composingOffset: newEditingState.baseOffset, composingExtent: newEditingState.extentOffset); | ||
} | ||
|
||
void maybeSendAction(html.Event event) { | ||
if (event is html.KeyboardEvent) { | ||
if (inputConfiguration.inputType.submitActionOnEnter && | ||
|
@@ -1169,6 +1344,10 @@ class IOSTextEditingStrategy extends GloballyPositionedTextEditingStrategy { | |
|
||
subscriptions.add(html.document.onSelectionChange.listen(handleChange)); | ||
|
||
activeDomElement.addEventListener('beforeinput', handleBeforeInput); | ||
|
||
activeDomElement.addEventListener('compositionupdate', handleCompositionUpdate); | ||
|
||
// Position the DOM element after it is focused. | ||
subscriptions.add(activeDomElement.onFocus.listen((_) { | ||
// Cancel previous timer if exists. | ||
|
@@ -1300,6 +1479,10 @@ class AndroidTextEditingStrategy extends GloballyPositionedTextEditingStrategy { | |
|
||
subscriptions.add(html.document.onSelectionChange.listen(handleChange)); | ||
|
||
activeDomElement.addEventListener('beforeinput', handleBeforeInput); | ||
|
||
activeDomElement.addEventListener('compositionupdate', handleCompositionUpdate); | ||
|
||
subscriptions.add(activeDomElement.onBlur.listen((_) { | ||
if (domRenderer.windowHasFocus) { | ||
// Chrome on Android will hide the onscreen keyboard when you tap outside | ||
|
@@ -1352,6 +1535,10 @@ class FirefoxTextEditingStrategy extends GloballyPositionedTextEditingStrategy { | |
|
||
subscriptions.add(activeDomElement.onKeyDown.listen(maybeSendAction)); | ||
|
||
activeDomElement.addEventListener('beforeinput', handleBeforeInput); | ||
|
||
activeDomElement.addEventListener('compositionupdate', handleCompositionUpdate); | ||
|
||
// Detects changes in text selection. | ||
// | ||
// In Firefox, when cursor moves, neither selectionChange nor onInput | ||
|
@@ -1731,6 +1918,20 @@ class TextEditingChannel { | |
); | ||
} | ||
|
||
/// Sends the 'TextInputClient.updateEditingStateWithDeltas' message to the framework. | ||
void updateEditingStateWithDelta(int? clientId, TextEditingDeltaState? editingDeltaState) { | ||
EnginePlatformDispatcher.instance.invokeOnPlatformMessage( | ||
'flutter/textinput', | ||
const JSONMethodCodec().encodeMethodCall( | ||
MethodCall('TextInputClient.updateEditingStateWithDeltas', <dynamic>[ | ||
clientId, | ||
editingDeltaState!.toFlutter(), | ||
]), | ||
), | ||
_emptyCallback, | ||
); | ||
} | ||
|
||
/// Sends the 'TextInputClient.performAction' message to the framework. | ||
void performAction(int? clientId, String? inputAction) { | ||
EnginePlatformDispatcher.instance.invokeOnPlatformMessage( | ||
|
@@ -1824,8 +2025,13 @@ class HybridTextEditing { | |
isEditing = true; | ||
strategy.enable( | ||
configuration!, | ||
onChange: (EditingState? editingState) { | ||
channel.updateEditingState(_clientId, editingState); | ||
onChange: (EditingState? editingState, TextEditingDeltaState? editingDeltaState) { | ||
if (configuration!.enableDeltaModel) { | ||
editingDeltaState = editingDeltaState!.copyWith(baseOffset: editingState!.baseOffset, extentOffset: editingState.extentOffset); | ||
channel.updateEditingStateWithDelta(_clientId, editingDeltaState); | ||
} else { | ||
channel.updateEditingState(_clientId, editingState); | ||
} | ||
}, | ||
onAction: (String? inputAction) { | ||
channel.performAction(_clientId, inputAction); | ||
|
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.
Can you add a comment explaining this class, and also a comment below explaining inferDeltaState?
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.
It seems like the point of this class is to represent the json that will be sent to the framework, so maybe mention that in the comment.