Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ Koutaro Mori <koutaro.mo@gmail.com>
TheOneWithTheBraid <the-one@with-the-braid.cf>
Twin Sun, LLC <google-contrib@twinsunsolutions.com>
Qixing Cao <qixing.cao.83@gmail.com>
LinXunFeng <linxunfeng@yeah.net>
LinXunFeng <linxunfeng@yeah.net>
Amir Panahandeh <amirpanahandeh@gmail.com>
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ mixin CompositionAwareMixin {
}

EditingState determineCompositionState(EditingState editingState) {
if (editingState.baseOffset == null || composingText == null || editingState.text == null) {
if (editingState.extentOffset == null || composingText == null || editingState.text == null) {
return editingState;
}

final int composingBase = editingState.baseOffset! - composingText!.length;
final int composingBase = editingState.extentOffset! - composingText!.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks reasonable to me. Using baseOffset fails when composition changes the selection to be non-collapsed, composingBase turns out being negative so (-1,-1) is reported to the framework. This doesn't fail for non-delta input because for non-delta input we always replace the old editing state with the new one, while with delta input we require the correct ranges to correctly infer the difference.


if (composingBase < 0) {
return editingState;
Expand Down
26 changes: 25 additions & 1 deletion lib/web_ui/test/engine/composition_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ Future<void> testMain() async {
});

group('determine composition state', () {
test('should return new composition state if valid new composition', () {
test('should return new composition state - compositing middle of text', () {
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 probably outside of the scope of this PR, but I noticed this test has the selection of the EditingState set to (100, 100), while the text it contains is only testing. The selection should always be in the bounds of the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const int baseOffset = 100;
const String composingText = 'composeMe';

Expand All @@ -128,6 +128,30 @@ Future<void> testMain() async {
composingBaseOffset: expectedComposingBase,
composingExtentOffset: expectedComposingBase + composingText.length));
});

test('should return new composition state - compositing from beginning of text', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great! There are some untested code paths in determineCompositionState that we should probably add tests for - mainly the null checks and if composingBase < 0 (will this still occur with this change?)

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 can't think of any scenario that this can happen but it's still possible. Do you think I can address adding tests along with #44139 (comment) in another PR? @htoor3

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me @Amir-P

const String composingText = '今日は';

final EditingState editingState = EditingState(
text: '今日は',
baseOffset: 0,
extentOffset: 3,
);

final _MockWithCompositionAwareMixin mockWithCompositionAwareMixin =
_MockWithCompositionAwareMixin();
mockWithCompositionAwareMixin.composingText = composingText;

const int expectedComposingBase = 0;

expect(
mockWithCompositionAwareMixin
.determineCompositionState(editingState),
editingState.copyWith(
composingBaseOffset: expectedComposingBase,
composingExtentOffset:
expectedComposingBase + composingText.length));
});
});
});

Expand Down