Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[web] switch from .didGain/LoseAccessibilityFocus to .focus #53134

Merged
merged 3 commits into from
Jun 11, 2024
Merged

[web] switch from .didGain/LoseAccessibilityFocus to .focus #53134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e27b18e
[web] switch from .didGain/LoseAccessibilityFocus to .focus
yjbanov May 30, 2024
74e39d1
also fix #143337 as that messes with focus too
yjbanov Jun 1, 2024
be5c032
fix one more test
yjbanov Jun 3, 2024
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
17 changes: 4 additions & 13 deletions lib/web_ui/lib/src/engine/semantics/focusable.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,6 @@ typedef _FocusTarget = ({

/// The listener for the "focus" DOM event.
DomEventListener domFocusListener,

/// The listener for the "blur" DOM event.
DomEventListener domBlurListener,
});

/// Implements accessibility focus management for arbitrary elements.
Expand Down Expand Up @@ -135,7 +132,6 @@ class AccessibilityFocusManager {
semanticsNodeId: semanticsNodeId,
element: previousTarget.element,
domFocusListener: previousTarget.domFocusListener,
domBlurListener: previousTarget.domBlurListener,
);
return;
}
Expand All @@ -148,14 +144,12 @@ class AccessibilityFocusManager {
final _FocusTarget newTarget = (
semanticsNodeId: semanticsNodeId,
element: element,
domFocusListener: createDomEventListener((_) => _setFocusFromDom(true)),
domBlurListener: createDomEventListener((_) => _setFocusFromDom(false)),
domFocusListener: createDomEventListener((_) => _didReceiveDomFocus()),
);
_target = newTarget;

element.tabIndex = 0;
element.addEventListener('focus', newTarget.domFocusListener);
element.addEventListener('blur', newTarget.domBlurListener);
}

/// Stops managing the focus of the current element, if any.
Expand All @@ -170,10 +164,9 @@ class AccessibilityFocusManager {
}

target.element.removeEventListener('focus', target.domFocusListener);
target.element.removeEventListener('blur', target.domBlurListener);
}

void _setFocusFromDom(bool acquireFocus) {
void _didReceiveDomFocus() {
final _FocusTarget? target = _target;

if (target == null) {
Expand All @@ -184,9 +177,7 @@ class AccessibilityFocusManager {

EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
target.semanticsNodeId,
acquireFocus
? ui.SemanticsAction.didGainAccessibilityFocus
: ui.SemanticsAction.didLoseAccessibilityFocus,
ui.SemanticsAction.focus,
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we also want to send a11yfocus as well?

Copy link
Contributor Author

@yjbanov yjbanov May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That would be a wrong thing to do. On the web, accessibility focus is not observable through any platform API. If the accessibility focus is indeed detached from the input focus, sending didGainAccessibilityFocus together with focus would make the framework believe a wrong widget has accessibility focus. I think instead, we want the framework to be resilient to the absence of accessibility focus signals (it seems to be the case already for our own widgets; according to flutter/flutter#83809 (comment) it's only used for highlighting, which can be achieved at least on desktop through input focus).

Copy link
Contributor

Choose a reason for hiding this comment

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

If the accessibility focus is indeed detached from the input focus, sending didGainAccessibilityFocus together with focus would make the framework believe a wrong widget has accessibility focus.

but on the web they are not detached, right? or at least we can't tell whether something receives only the a11y focus. but when something receives focus, it must also have received a11y focus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be detached on the web too. For example, you can uncheck the following checkbox in macOS VoiceOver utility:

Screenshot 2024-05-31 at 12 44 06 PM

With that checkbox off, you can move the accessibility focus around without moving the input focus. Here's an example of VoiceOver focus being on a text field inside the web page, but when typing your input goes into the address bar instead:

Screenshot 2024-05-31 at 12 46 29 PM

Here's another example, where the a11y focus is on the last name field, but I'm entering the first name field:

Screenshot 2024-05-31 at 12 57 05 PM

You need to take special action to activate the widget under VoiceOver cursor, e.g. Control + Option + Space.

However, there's no listeners we can hook up on the web page to tell where the VoiceOver cursor is headed. So it's not observable to us. That's not a real problem though. The browser already does whatever it needs and determines when input focus should move into the field, and send us a DOM "focus" event. The web engine will simply forward it to the framework as SemanticsAction.focus.

I must say, this detached mode feels confusing to me. I think when both kinds of focus are synchronized, it's easier to use the UI. VoiceOver itself seems to be giving incorrect instructions. For example, when you move focus to a text field, it says "To enter text in this field, type", which is incorrect in this mode as the text field does not yet have focus.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so the web just not going to use a11y focus. Now I think of it this is probably correct since native web can't tell which dom has the a11y focus either.

null,
);
}
Expand Down Expand Up @@ -229,7 +220,7 @@ class AccessibilityFocusManager {
// a dialog, and nothing else in the dialog is focused. The Flutter
// framework expects that the screen reader will focus on the first (in
// traversal order) focusable element inside the dialog and send a
// didGainAccessibilityFocus action. Screen readers on the web do not do
// SemanticsAction.focus action. Screen readers on the web do not do
// that, and so the web engine has to implement this behavior directly. So
// the dialog will look for a focusable element and request focus on it,
// but now there may be a race between this method unsetting the focus and
Expand Down
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 @@ -310,16 +310,7 @@ class TextField extends PrimaryRoleManager {
}

EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
semanticsObject.id, ui.SemanticsAction.didGainAccessibilityFocus, null);
}));
activeEditableElement.addEventListener('blur',
createDomEventListener((DomEvent event) {
if (EngineSemantics.instance.gestureMode != GestureMode.browserGestures) {
return;
}

EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
semanticsObject.id, ui.SemanticsAction.didLoseAccessibilityFocus, null);
semanticsObject.id, ui.SemanticsAction.focus, null);
}));
}

Expand Down
55 changes: 29 additions & 26 deletions lib/web_ui/test/engine/semantics/semantics_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1755,7 +1755,7 @@ void _testIncrementables() {

pumpSemantics(isFocused: true);
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didGainAccessibilityFocus, null),
(0, ui.SemanticsAction.focus, null),
]);
capturedActions.clear();

Expand All @@ -1766,10 +1766,12 @@ void _testIncrementables() {
isEmpty,
);

// The web doesn't send didLoseAccessibilityFocus as on the web,
// accessibility focus is not observable, only input focus is. As of this
// writing, there is no SemanticsAction.unfocus action, so the test simply
// asserts that no actions are being sent as a result of blur.
element.blur();
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);
expect(capturedActions, isEmpty);

semantics().semanticsEnabled = false;
});
Expand Down Expand Up @@ -1831,7 +1833,7 @@ void _testTextField() {
final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder();
updateNode(
builder,
actions: 0 | ui.SemanticsAction.didGainAccessibilityFocus.index,
actions: 0 | ui.SemanticsAction.focus.index,
flags: 0 | ui.SemanticsFlag.isTextField.index,
value: 'hello',
transform: Matrix4.identity().toFloat64(),
Expand All @@ -1849,7 +1851,7 @@ void _testTextField() {

expect(owner().semanticsHost.ownerDocument?.activeElement, textField);
expect(await logger.idLog.first, 0);
expect(await logger.actionLog.first, ui.SemanticsAction.didGainAccessibilityFocus);
expect(await logger.actionLog.first, ui.SemanticsAction.focus);

semantics().semanticsEnabled = false;
}, // TODO(yjbanov): https://github.com/flutter/flutter/issues/46638
Expand Down Expand Up @@ -2135,7 +2137,7 @@ void _testCheckables() {

pumpSemantics(isFocused: true);
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didGainAccessibilityFocus, null),
(0, ui.SemanticsAction.focus, null),
]);
capturedActions.clear();

Expand All @@ -2145,15 +2147,12 @@ void _testCheckables() {
pumpSemantics(isFocused: false);
expect(capturedActions, isEmpty);

// If the element is blurred by the browser, then we do want to notify the
// framework. This is because screen reader can be focused on something
// other than what the framework is focused on, and notifying the framework
// about the loss of focus on a node is information that the framework did
// not have before.
// The web doesn't send didLoseAccessibilityFocus as on the web,
// accessibility focus is not observable, only input focus is. As of this
// writing, there is no SemanticsAction.unfocus action, so the test simply
// asserts that no actions are being sent as a result of blur.
element.blur();
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);
expect(capturedActions, isEmpty);

semantics().semanticsEnabled = false;
});
Expand Down Expand Up @@ -2319,17 +2318,19 @@ void _testTappable() {

pumpSemantics(isFocused: true);
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didGainAccessibilityFocus, null),
(0, ui.SemanticsAction.focus, null),
]);
capturedActions.clear();

pumpSemantics(isFocused: false);
expect(capturedActions, isEmpty);

// The web doesn't send didLoseAccessibilityFocus as on the web,
// accessibility focus is not observable, only input focus is. As of this
// writing, there is no SemanticsAction.unfocus action, so the test simply
// asserts that no actions are being sent as a result of blur.
element.blur();
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);
expect(capturedActions, isEmpty);

semantics().semanticsEnabled = false;
});
Expand Down Expand Up @@ -3159,7 +3160,7 @@ void _testDialog() {
expect(
capturedActions,
<CapturedAction>[
(2, ui.SemanticsAction.didGainAccessibilityFocus, null),
(2, ui.SemanticsAction.focus, null),
],
);

Expand Down Expand Up @@ -3221,7 +3222,7 @@ void _testDialog() {
expect(
capturedActions,
<CapturedAction>[
(3, ui.SemanticsAction.didGainAccessibilityFocus, null),
(3, ui.SemanticsAction.focus, null),
],
);

Expand Down Expand Up @@ -3371,7 +3372,7 @@ void _testFocusable() {
pumpSemantics(); // triggers post-update callbacks
expect(domDocument.activeElement, element);
expect(capturedActions, <CapturedAction>[
(1, ui.SemanticsAction.didGainAccessibilityFocus, null),
(1, ui.SemanticsAction.focus, null),
]);
capturedActions.clear();

Expand All @@ -3384,17 +3385,19 @@ void _testFocusable() {
// Browser blurs the element
element.blur();
expect(domDocument.activeElement, isNot(element));
expect(capturedActions, <CapturedAction>[
(1, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);
// The web doesn't send didLoseAccessibilityFocus as on the web,
// accessibility focus is not observable, only input focus is. As of this
// writing, there is no SemanticsAction.unfocus action, so the test simply
// asserts that no actions are being sent as a result of blur.
expect(capturedActions, isEmpty);
capturedActions.clear();

// Request focus again
manager.changeFocus(true);
pumpSemantics(); // triggers post-update callbacks
expect(domDocument.activeElement, element);
expect(capturedActions, <CapturedAction>[
(1, ui.SemanticsAction.didGainAccessibilityFocus, null),
(1, ui.SemanticsAction.focus, null),
]);
capturedActions.clear();

Expand Down
4 changes: 4 additions & 0 deletions lib/web_ui/test/engine/semantics/semantics_tester.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class SemanticsTester {
bool? hasPaste,
bool? hasDidGainAccessibilityFocus,
bool? hasDidLoseAccessibilityFocus,
bool? hasFocus,
bool? hasCustomAction,
bool? hasDismiss,
bool? hasMoveCursorForwardByWord,
Expand Down Expand Up @@ -241,6 +242,9 @@ class SemanticsTester {
if (hasDidLoseAccessibilityFocus ?? false) {
actions |= ui.SemanticsAction.didLoseAccessibilityFocus.index;
}
if (hasFocus ?? false) {
actions |= ui.SemanticsAction.focus.index;
}
if (hasCustomAction ?? false) {
actions |= ui.SemanticsAction.customAction.index;
}
Expand Down
6 changes: 2 additions & 4 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 @@ -110,7 +110,7 @@ void testMain() {

// TODO(yjbanov): this test will need to be adjusted for Safari when we add
// Safari testing.
test('sends a didGainAccessibilityFocus/didLoseAccessibilityFocus action when browser requests focus/blur', () async {
test('sends a SemanticsAction.focus action when browser requests focus', () async {
final SemanticsActionLogger logger = SemanticsActionLogger();
createTextFieldSemantics(value: 'hello');

Expand All @@ -123,13 +123,11 @@ void testMain() {

expect(owner().semanticsHost.ownerDocument?.activeElement, textField);
expect(await logger.idLog.first, 0);
expect(await logger.actionLog.first, ui.SemanticsAction.didGainAccessibilityFocus);
expect(await logger.actionLog.first, ui.SemanticsAction.focus);

textField.blur();

expect(owner().semanticsHost.ownerDocument?.activeElement, isNot(textField));
expect(await logger.idLog.first, 0);
expect(await logger.actionLog.first, ui.SemanticsAction.didLoseAccessibilityFocus);
}, // TODO(yjbanov): https://github.com/flutter/flutter/issues/46638
// TODO(yjbanov): https://github.com/flutter/flutter/issues/50590
skip: ui_web.browser.browserEngine != ui_web.BrowserEngine.blink);
Expand Down