updateEditingValueWithDeltas should fail loudly when TextRange is invalid#107426
updateEditingValueWithDeltas should fail loudly when TextRange is invalid#107426Renzo-Olivares merged 11 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
a: text input
| /// replacement string. | ||
| // 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) { |
There was a problem hiding this comment.
You probably don't need this function in first place? https://master-api.flutter.dev/flutter/dart-core/String/replaceRange.html
| } | ||
|
|
||
| // Verify that the given range is within the text. | ||
| bool _textRangeIsValid(TextRange range, String text) { |
There was a problem hiding this comment.
preferably the function name needs a debug prefix.
|
|
||
| // Verify that the given range is within the text. | ||
| bool _textRangeIsValid(TextRange range, String text) { | ||
| if (range.start == -1 && range.end == -1) { |
There was a problem hiding this comment.
nit: !range.isInvalid instead. This should be consistent with the isValid definition I think?
| return true; | ||
| } | ||
| assert(range.start >= 0 && range.start <= text.length, | ||
| 'Range start ${range.start} is out of text of length ${text.length}'); |
There was a problem hiding this comment.
nit: maybe print the entire range here to give more context? The end could be OOB too.
There was a problem hiding this comment.
nit: looking at the callers of this function, it's a bit difficult to tell what kind of TextRange is OOB, the only thing that can help developers trace that down is the line number in the stacktrace.
There was a problem hiding this comment.
I updated this a bit, to make it more obvious what range was failing (delta/selection/composing).
| // Verify the error message in parts because Web formats the message | ||
| // differently from others. | ||
| expect(record[0].exception.toString(), matches(RegExp(r'\brange.start >= 0 && range.start <= text.length\b'))); | ||
| expect(record[0].exception.toString(), matches(RegExp(r'\bRange start 3 is out of text of length 1\b'))); |
There was a problem hiding this comment.
out of curiosity, what's the difference?
There was a problem hiding this comment.
Slight formatting difference.
Command line:
Actual: '\'package:flutter/src/services/text_editing_delta.dart\': Failed assertion: line 140 pos 14: \'_debugTextRangeIsValid(newSelection, oldText)\': The selection range: TextSelection.collapsed(offset: 3, affinity: TextAffinity.downstream, isDirectional: false) is not within the bounds of text: 1 of length: 1'
Web:
Actual: 'Assertion failed: file:///Users/roliv/flutter/packages/flutter/lib/src/services/text_editing_delta.dart:140:14\n'
'_debugTextRangeIsValid(newSelection, oldText)\n'
'"The selection range: TextSelection.collapsed(offset: 3, affinity: TextAffinity.downstream, isDirectional: false) is not within the bounds of text: 1 of length: 1"'
prevents me from just using endsWith()
| if ((range.start >= 0 && range.start <= text.length) && (range.end >= 0 && range.end <= text.length)) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Nit: You could also directly return the expression in the if.
| ); | ||
|
|
||
| if (isNonTextUpdate) { | ||
| assert(_debugTextRangeIsValid(newSelection, oldText), 'TextEditingDelta.fromJSON failed, the selection range: $newSelection is not within the bounds of text: $oldText of length: ${oldText.length}'); |
There was a problem hiding this comment.
Nit: I think you could just start the error message at "the selection range..." if you want because the first part is redundant. The stacktrace will point to TextEditingDelta.fromJSON and "failed" is also obvious.
…alid (flutter#107426) * Make deltas fail loudly * analyzer fixes * empty * updates * Analyzer fixes * Make it more obvious what kind of TextRange is failing and where * update tests * Add tests for concrete TextEditinDelta apply method * trailing spaces * address nits * fix analyzer Co-authored-by: Renzo Olivares <[email protected]>
3 participants
This PR addresses a concern brought up in #107127 where the creation of a
TextEditingDeltathrough thefromJsonmethod will fail silently when given bad text bounds (delta range, selection, or composing region). A similar issue was addressed for non-delta text input in #104711 and this change adds the same text bound checks to delta text input.Pre-launch Checklist
///).