-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix a11y tab traversal #25797
Fix a11y tab traversal #25797
Conversation
2572346 to
afec643
Compare
afec643 to
13f0431
Compare
|
I'm going to debug the Mac Safari test but otherwise this is ready for review. I don't think the approach will change much. |
| /// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus | ||
| bool? get windowHasFocus => | ||
| bool get windowHasFocus => | ||
| js_util.callMethod(html.document, 'hasFocus', <dynamic>[]); |
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.
i would add ?? false here if there are browsers that don't implement and we have undefined return.
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.
Done.
| void update() { | ||
| final html.Element element = semanticsObject.element; | ||
|
|
||
| // "tab-index" HTML attribute is necessary to make the element can be |
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.
maybe reword: tab-index = 0 is used for allow keyboard traversal of non-form elements.
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.
Done.
| _stopListening(); | ||
| } | ||
| } | ||
| if (semanticsObject.isFlagsDirty && semanticsObject.hasFocus) { |
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.
Add comment on why we are setting focus here.
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.
Done.
| _subscriptions.clear(); | ||
| _lastEditingState = null; | ||
|
|
||
| // If focused element is a part of a form, it needs to stay on the DOM |
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.
Looks like this was added as a fix for form autofill, does disabling keep form submit intact?
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.
No, I removed form autofill from semantics. It was broken. The non-a11y version is still intact.
| // Element placement is done by [TextField]. | ||
| } | ||
|
|
||
| @override |
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.
remove empty override?
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.
The empty override makes these methods noop.
| testInputElement = null; | ||
| }); | ||
|
|
||
| test('autofill form lifecycle works', () async { |
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.
Is there an integration test for autofill that still covers this?
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.
I'm not changing tests for non-a11y form autofill. This is a11y autofill, which I'm removing due to broken design.
13f0431 to
6561387
Compare
6561387 to
461af70
Compare
461af70 to
322fe17
Compare
f5cf6eb to
6e942e6
Compare
* fix race with framework when using tab to traverse in a11y mode
Fix widget traversal when using the TAB key while a11y is enabled.
flutter/textinputchannel.tabindex=0onto tappable targets, otherwise the browser doesn't treat the element as focusable.This is an alternative approach to the previously proposed #25401. This approach does not require changes on the framework side.
Fixes flutter/flutter#52106