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

[web] Don't overwrite editing state with semantic updates #38271

Merged
merged 8 commits into from
Dec 19, 2022
Merged

[web] Don't overwrite editing state with semantic updates #38271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
717fcc0
Remove editing state overwrite on semantic updates
htoor3 Dec 12, 2022
19d72e6
Add tests
htoor3 Dec 13, 2022
acabdc0
Fix test name
htoor3 Dec 13, 2022
85bb97e
Assert that framework messages will update semantic editing state
htoor3 Dec 14, 2022
3bf4a84
Merge remote-tracking branch 'upstream/main' into remove-edit-state-o…
htoor3 Dec 14, 2022
2e79071
Change test name
htoor3 Dec 14, 2022
df642d4
Merge branch 'main' into remove-edit-state-overwrite-semantic
htoor3 Dec 15, 2022
0040d98
Whitespace
htoor3 Dec 15, 2022
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
11 changes: 1 addition & 10 deletions lib/web_ui/lib/src/engine/semantics/text_field.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,11 +364,7 @@ class TextField extends RoleManager {
// element, so that both the framework and the browser agree on what's
// currently focused.
bool needsDomFocusRequest = false;
final EditingState editingState = EditingState(
text: semanticsObject.value,
baseOffset: semanticsObject.textSelectionBase,
extentOffset: semanticsObject.textSelectionExtent,
);

if (semanticsObject.hasFocus) {
if (!_hasFocused) {
_hasFocused = true;
Expand All @@ -378,14 +374,9 @@ class TextField extends RoleManager {
if (domDocument.activeElement != editableElement) {
needsDomFocusRequest = true;
}
// Focused elements should have full text editing state applied.
SemanticsTextEditingStrategy.instance.setEditingState(editingState);
} else if (_hasFocused) {
SemanticsTextEditingStrategy.instance.deactivate(this);

// Only apply text, because this node is not focused.
editingState.applyTextToDomElement(editableElement);

if (_hasFocused && domDocument.activeElement == editableElement) {
// Unlike `editableElement.focus()` we don't need to schedule `blur`
// post-update because `document.activeElement` implies that the
Expand Down
101 changes: 98 additions & 3 deletions lib/web_ui/test/engine/semantics/text_field_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

@TestOn('chrome || safari || firefox')

import 'dart:typed_data';

import 'package:test/bootstrap/browser.dart';
import 'package:test/test.dart';

Expand Down Expand Up @@ -48,6 +50,11 @@ void testMain() {
testTextEditing.configuration = singlelineConfig;
});

/// Emulates sending of a message by the framework to the engine.
void sendFrameworkMessage(ByteData? message) {
testTextEditing.channel.handleTextInput(message, (ByteData? data) {});
}

test('renders a text field', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
Expand Down Expand Up @@ -127,7 +134,7 @@ void testMain() {
// TODO(yjbanov): https://github.com/flutter/flutter/issues/50754
skip: browserEngine != BrowserEngine.blink);

test('Syncs editing state from framework', () async {
test('Syncs semantic state from framework', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;
Expand Down Expand Up @@ -159,7 +166,6 @@ void testMain() {
expect(domDocument.activeElement, flutterViewEmbedder.glassPaneElement);
expect(appHostNode.activeElement, strategy.domElement);
expect(textField.editableElement, strategy.domElement);
expect((textField.editableElement as dynamic).value, 'hello');
Copy link
Contributor

Choose a reason for hiding this comment

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

This expectation used to verify that the semantics text value gets updated when the framework begins editing a text field. I think to cover that with the new approach we need a new test (or maybe just add it to this test) that sends the text editing state through the text input channel and checks that the editing state is applied to textField.editableElement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. I wrote it this way because we're delegating the responsibility of editing state updates to text_editing.dart, so it seems like the best place to test framework -> engine editing state updates would be in text_editing_test.dart which has existing tests that verify that behavior.

But is the idea here that we want to explicitly test whether that behavior applies specifically to semantic nodes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to test the behavior of text_editing.dart when using a semantics editing strategy to make sure that the element's value is still being updated correctly.

expect(textField.editableElement.getAttribute('aria-label'), 'greeting');
expect(textField.editableElement.style.width, '10px');
expect(textField.editableElement.style.height, '15px');
Expand All @@ -174,7 +180,6 @@ void testMain() {
expect(domDocument.activeElement, domDocument.body);
expect(appHostNode.activeElement, null);
expect(strategy.domElement, null);
expect((textField.editableElement as dynamic).value, 'bye');
expect(textField.editableElement.getAttribute('aria-label'), 'farewell');
expect(textField.editableElement.style.width, '12px');
expect(textField.editableElement.style.height, '17px');
Expand All @@ -188,6 +193,92 @@ void testMain() {
expect(actionCount, 0);
});

test(
'Does not overwrite text value and selection editing state on semantic updates',
() async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;

strategy.enable(
singlelineConfig,
onChange: (_, __) {},
onAction: (_) {},
);

final SemanticsObject textFieldSemantics = createTextFieldSemantics(
value: 'hello',
textSelectionBase: 1,
textSelectionExtent: 3,
isFocused: true,
rect: const ui.Rect.fromLTWH(0, 0, 10, 15));

final TextField textField =
textFieldSemantics.debugRoleManagerFor(Role.textField)! as TextField;
final DomHTMLInputElement editableElement =
textField.editableElement as DomHTMLInputElement;

expect(editableElement, strategy.domElement);
expect(editableElement.value, '');
expect(editableElement.selectionStart, 0);
expect(editableElement.selectionEnd, 0);

strategy.disable();
semantics().semanticsEnabled = false;
});

test(
'Updates editing state when receiving framework messages from the text input channel',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @yjbanov @mdebbar

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect! Thanks.

() async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;

expect(domDocument.activeElement, domDocument.body);
expect(appHostNode.activeElement, null);

strategy.enable(
singlelineConfig,
onChange: (_, __) {},
onAction: (_) {},
);

final SemanticsObject textFieldSemantics = createTextFieldSemantics(
value: 'hello',
textSelectionBase: 1,
textSelectionExtent: 3,
isFocused: true,
rect: const ui.Rect.fromLTWH(0, 0, 10, 15));

final TextField textField =
textFieldSemantics.debugRoleManagerFor(Role.textField)! as TextField;
final DomHTMLInputElement editableElement =
textField.editableElement as DomHTMLInputElement;

// No updates expected on semantic updates
expect(editableElement, strategy.domElement);
expect(editableElement.value, '');
expect(editableElement.selectionStart, 0);
expect(editableElement.selectionEnd, 0);

// Update from framework
const MethodCall setEditingState =
MethodCall('TextInput.setEditingState', <String, dynamic>{
'text': 'updated',
'selectionBase': 2,
'selectionExtent': 3,
});
sendFrameworkMessage(codec.encodeMethodCall(setEditingState));

// Editing state should now be updated
expect(editableElement.value, 'updated');
expect(editableElement.selectionStart, 2);
expect(editableElement.selectionEnd, 3);

strategy.disable();
semantics().semanticsEnabled = false;
});

test('Gives up focus after DOM blur', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
Expand Down Expand Up @@ -446,6 +537,8 @@ SemanticsObject createTextFieldSemantics({
bool isFocused = false,
bool isMultiline = false,
ui.Rect rect = const ui.Rect.fromLTRB(0, 0, 100, 50),
int textSelectionBase = 0,
int textSelectionExtent = 0,
}) {
final SemanticsTester tester = SemanticsTester(semantics());
tester.updateNode(
Expand All @@ -458,6 +551,8 @@ SemanticsObject createTextFieldSemantics({
hasTap: true,
rect: rect,
textDirection: ui.TextDirection.ltr,
textSelectionBase: textSelectionBase,
textSelectionExtent: textSelectionExtent
);
tester.apply();
return tester.getSemanticsObject(0);
Expand Down