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

[web] autofocus in new routes #47727

Merged
auto-submit merged 7 commits into from
Dec 21, 2023
Merged

[web] autofocus in new routes #47727

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
149a99c
[web] autofocus in new routes
yjbanov Nov 2, 2023
b564a5a
add test; clean-up
yjbanov Nov 13, 2023
59b11d7
address comments
yjbanov Dec 4, 2023
7b6952c
address comments
yjbanov Dec 5, 2023
a182001
fix "the this"
yjbanov Dec 18, 2023
dd39594
Update lib/web_ui/lib/src/engine/semantics/focusable.dart
yjbanov Dec 20, 2023
c3f9ffd
clean up visitDepthFirstInRenderOrder
yjbanov Dec 20, 2023
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
Prev Previous commit
Next Next commit
add test; clean-up
  • Loading branch information
@yjbanov
yjbanov committed Dec 18, 2023
commit b564a5a00c84fb02d300ab9d4f4b3bf46f323143
6 changes: 3 additions & 3 deletions lib/web_ui/lib/src/engine/semantics/dialog.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Dialog extends PrimaryRoleManager {
// focus on something inside it. There could be two possibilities:
//
// 1. The framework explicitly marked a node inside the dialog as focused
// via the `isFocusabe` and `isFocused` flags. In this case, the node
// via the `isFocusable` and `isFocused` flags. In this case, the node
Copy link
Contributor

@chunhtai chunhtai Nov 17, 2023

Choose a reason for hiding this comment

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

isFocusable/isFocused are refering to keyboard focus. For a sighted user it is ok if some element in the middle of the page request keyboard focus and the system bring focus to there. For a visually impaired user, this will make them confused.

Should we always do (2) if screen reader is turned on.

Copy link
Contributor

@marcianx marcianx Dec 5, 2023

Choose a reason for hiding this comment

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

It really depends on the type of dialog. For example for a role="alertdialog", it's very common to move focus into the Ok button since when the focus first moves into the alert dialog, the screen reader vocalizes the entire contents of the dialog. Of course, the choice to focus on the "Ok" dialog would ideally by done via an explicit autofocus rather than implicit focus. So maybe you may decide that that consideration might be less relevant for fallback behavior.

Copy link
Contributor Author

@yjbanov yjbanov Dec 18, 2023

Choose a reason for hiding this comment

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

On the web we do not know if the screen reader is turned on, so we have to pick one option. I'm not sure if focusing on the first keyboard-focusable will produce good result, as there's no guarantee that the widget will be correct one, and landing in the middle of the dialog, the user will have to traverse backwards then forwards to find the widget they are interested in.

I think keeping the logic simple (i.e. what it is in the PR right now) while leaving the option to use autofocus on the framework should be flexible enough.

// will request focus directly and there's nothing to do on top of that.
// 2. No node inside the route takes focus explicitly. In this case, the
Copy link
Contributor

@chunhtai chunhtai Dec 20, 2023

Choose a reason for hiding this comment

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

In case this route is reveal when the previous top-most routes are popped, you can listen to the send focus event if we start sending event for web in framework.

Copy link
Contributor Author

@yjbanov yjbanov Dec 20, 2023

Choose a reason for hiding this comment

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

Yep. I've been getting updates from @Hangyujin about it. I'm planning to implementing for web, if/when it's ready on the framework side.

Copy link
Member

@hannah-hyj hannah-hyj Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it has been implemented on ios and android now.
related PR: ios: #48252
framework: flutter/flutter#135771

// expectation is to look through all nodes in traversal order and focus
Expand All @@ -45,7 +45,7 @@ class Dialog extends PrimaryRoleManager {
}

// If the node does not take focus (e.g. focusing on it does not make
// sense at all), depair not. Keep looking.
// sense at all). Despair not. Keep looking.
final bool didTakeFocus = roleManager.focusAsRouteDefault();
return !didTakeFocus;
});
Expand Down Expand Up @@ -94,7 +94,7 @@ class Dialog extends PrimaryRoleManager {
@override
bool focusAsRouteDefault() {
// Dialogs are the ones that look inside themselves to find elements to
// focus one. It doesn't make sense to focus on the dialog itself.
// focus on. It doesn't make sense to focus on the dialog itself.
return false;
}
}
Expand Down
11 changes: 9 additions & 2 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 @@ -34,8 +34,16 @@ class Focusable extends RoleManager {

final AccessibilityFocusManager _focusManager;

/// Requests focus as a result of a route (e.g. dialog) deciding that the node
/// managed by this class should be focused by default when nothing requests
/// focus explicitly.
///
/// This method of taking focus is different from the regular method of using
/// the [SemanticsObject.hasFocus] flag, as in this case the framework is not
/// explicitly request focus. Instead, the DOM element is being focus directly
/// programmatically, simulating the screen reader choosing a default element
/// to focus on.
Comment on lines +44 to +45
Copy link
Contributor

@mdebbar mdebbar Dec 20, 2023

Choose a reason for hiding this comment

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

Should we inform the framework about the element we chose here? That way, when the user hits tab, the framework would continue the traversal correctly.

Copy link
Contributor Author

@yjbanov yjbanov Dec 20, 2023

Choose a reason for hiding this comment

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

We do inform the framework via a didGainAccessibilityFocus event, which happen by relevant nodes receiving the DOM focus event.

bool focusAsRouteDefault() {
print('>>> ${semanticsObject.id} is taking default route focus (label: ${semanticsObject.label})');
owner.element.focus();
return true;
}
Expand Down Expand Up @@ -237,7 +245,6 @@ class AccessibilityFocusManager {
return;
}

print('>>> calling focus on ${target.element} id="${target.element.id}"');
target.element.focus();
});
}
Expand Down
257 changes: 248 additions & 9 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 @@ -577,6 +577,11 @@ class MockRoleManager extends PrimaryRoleManager {
super.update();
_log('update');
}

@override
bool focusAsRouteDefault() {
throw UnimplementedError();
}
}

class MockSemanticsEnabler implements SemanticsEnabler {
Expand Down Expand Up @@ -1537,16 +1542,24 @@ void _testIncrementables() {
};

pumpSemantics(isFocused: false);
final DomElement element = semantics().debugSemanticsTree![0]!.element.querySelector('input')!;
expect(capturedActions, isEmpty);

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

pumpSemantics(isFocused: false);
expect(
reason: 'The engine never calls blur() explicitly.',
capturedActions,
isEmpty,
);

element.blur();
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didGainAccessibilityFocus, null),
(0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);

Expand Down Expand Up @@ -1897,16 +1910,20 @@ void _testCheckables() {
};

pumpSemantics(isFocused: false);
final DomElement element = semantics().debugSemanticsTree![0]!.element;
expect(capturedActions, isEmpty);

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

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

element.blur();
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didGainAccessibilityFocus, null),
(0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);

Expand Down Expand Up @@ -2069,16 +2086,20 @@ void _testTappable() {
};

pumpSemantics(isFocused: false);
final DomElement element = semantics().debugSemanticsTree![0]!.element;
expect(capturedActions, isEmpty);

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

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

element.blur();
expect(capturedActions, <CapturedAction>[
(0, ui.SemanticsAction.didGainAccessibilityFocus, null),
(0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);

Expand Down Expand Up @@ -2856,6 +2877,210 @@ void _testDialog() {

semantics().semanticsEnabled = false;
});

// Test the simple scenario of a dialog coming up and containing focusable
// descentants that are not initially focused. The expectation is that the
Copy link
Contributor

@marcianx marcianx Nov 13, 2023

Choose a reason for hiding this comment

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

typo: "descentants"

Copy link
Contributor Author

@yjbanov yjbanov Dec 4, 2023

Choose a reason for hiding this comment

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

Fixed.

// first descendant will be auto-focused.
test('focuses on the first unfocused Focusable', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;

final List<CapturedAction> capturedActions = <CapturedAction>[];
EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) {
capturedActions.add((event.nodeId, event.type, event.arguments));
};

final SemanticsTester tester = SemanticsTester(semantics());
tester.updateNode(
id: 0,
scopesRoute: true,
transform: Matrix4.identity().toFloat64(),
children: <SemanticsNodeUpdate>[
tester.updateNode(
id: 1,
children: <SemanticsNodeUpdate>[
tester.updateNode(
id: 2,
label: 'Button 1',
hasTap: true,
hasEnabledState: true,
isEnabled: true,
isButton: true,
isFocusable: true,
isFocused: false,
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
),
tester.updateNode(
id: 3,
label: 'Button 2',
hasTap: true,
hasEnabledState: true,
isEnabled: true,
isButton: true,
isFocusable: true,
isFocused: false,
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
),
],
),
],
);
tester.apply();

expect(
capturedActions,
<CapturedAction>[
(2, ui.SemanticsAction.didGainAccessibilityFocus, null),
],
);

semantics().semanticsEnabled = false;
});

// Test the scenario of a dialog coming up and containing focusable
// descentants with one of them explicitly requesting focus. The expectation
Copy link
Contributor

@marcianx marcianx Nov 13, 2023

Choose a reason for hiding this comment

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

typo: "descentants"

Copy link
Contributor Author

@yjbanov yjbanov Dec 4, 2023

Choose a reason for hiding this comment

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

Fixed.

// is that the dialog will not attempt to auto-focus on anything and let the
// respective descendant take focus.
test('does nothing if a descendant asks for focus explicitly', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;

final List<CapturedAction> capturedActions = <CapturedAction>[];
EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) {
capturedActions.add((event.nodeId, event.type, event.arguments));
};

final SemanticsTester tester = SemanticsTester(semantics());
tester.updateNode(
id: 0,
scopesRoute: true,
transform: Matrix4.identity().toFloat64(),
children: <SemanticsNodeUpdate>[
tester.updateNode(
id: 1,
children: <SemanticsNodeUpdate>[
tester.updateNode(
id: 2,
label: 'Button 1',
hasTap: true,
hasEnabledState: true,
isEnabled: true,
isButton: true,
isFocusable: true,
isFocused: false,
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
),
tester.updateNode(
id: 3,
label: 'Button 2',
hasTap: true,
hasEnabledState: true,
isEnabled: true,
isButton: true,
isFocusable: true,
isFocused: true,
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
),
],
),
],
);
tester.apply();

expect(
capturedActions,
<CapturedAction>[
(3, ui.SemanticsAction.didGainAccessibilityFocus, null),
],
);

semantics().semanticsEnabled = false;
});

// Test the scenario of a dialog coming up and containing non-focusable
// descentants that can have a11y focus. The expectation is that the first
Copy link
Contributor

@marcianx marcianx Nov 13, 2023

Choose a reason for hiding this comment

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

typo: "descentants"

Copy link
Contributor Author

@yjbanov yjbanov Dec 4, 2023

Choose a reason for hiding this comment

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

Fixed.

// descendant will be auto-focused.
Copy link
Contributor

@marcianx marcianx Nov 13, 2023

Choose a reason for hiding this comment

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

"...even if it's not focusable."
(surprising to me, as I expected it to go to the first focusable descendant!)

Copy link
Contributor Author

@yjbanov yjbanov Dec 4, 2023

Choose a reason for hiding this comment

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

Are there any guidelines around this? My logic to focus on the first accessibility-focusable element is as follows:

For a screen reader user, nearly everything is focusable. The screen reader can focus on things that keyboard and mouse cannot, such as text (headings, paragraph), images, and other read-only elements. What the framework considers "focusable" is widgets that have input focus. You can interact with those widgets using keyboard and/or mouse. So let's say we auto-focused on the first widget from that subset. Consider a dialog with some headings, text, and buttons. Your screen reader starts in some random widget in the middle. To figure out what's on the dialog, you have to go back and forth to find the boundaries of the dialog and read available information. But if we focus on the first thing that can have accessibility focus, then all you need is to traverse the dialog forward, read through the text, then find the button you are interested in pressing. Defaulting to the first button may not even be the correct button to focus on by default. If it is desired that focus goes to an input-focusable widget, the app can still focus on it explicitly. The logic in this PR only kicks in if the framework did not communicate explicit focus to any descendant.

But it's totally possible that I'm not thinking about this correctly.

test('focuses on the first non-focusable descedant', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;

final List<CapturedAction> capturedActions = <CapturedAction>[];
EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) {
capturedActions.add((event.nodeId, event.type, event.arguments));
};

final SemanticsTester tester = SemanticsTester(semantics());
tester.updateNode(
id: 0,
scopesRoute: true,
transform: Matrix4.identity().toFloat64(),
children: <SemanticsNodeUpdate>[
tester.updateNode(
id: 1,
children: <SemanticsNodeUpdate>[
tester.updateNode(
id: 2,
label: 'Heading',
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
),
tester.updateNode(
id: 3,
label: 'Click me!',
hasTap: true,
hasEnabledState: true,
isEnabled: true,
isButton: true,
isFocusable: true,
isFocused: false,
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
),
],
),
],
);
tester.apply();

// The focused node is not focusable, so no notification is sent to the
// framework.
expect(capturedActions, isEmpty);

// However, the element should have gotten the focus.
final DomElement element = semantics().debugSemanticsTree![2]!.element;
expect(element.tabIndex, -1);
expect(domDocument.activeElement, element);

semantics().semanticsEnabled = false;
});

// This mostly makes sure the engine doesn't crash if given a completely empty
// dialog trying to find something to focus on.
test('does nothing if nothing is focusable inside the dialog', () async {
semantics()
..debugOverrideTimestampFunction(() => _testTime)
..semanticsEnabled = true;

final List<CapturedAction> capturedActions = <CapturedAction>[];
EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) {
capturedActions.add((event.nodeId, event.type, event.arguments));
};

final SemanticsTester tester = SemanticsTester(semantics());
tester.updateNode(
id: 0,
scopesRoute: true,
transform: Matrix4.identity().toFloat64(),
);
tester.apply();

// The focused node is not focusable, so no notification is sent to the
// framework.
expect(capturedActions, isEmpty);

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

typedef CapturedAction = (int nodeId, ui.SemanticsAction action, Object? args);
Expand Down Expand Up @@ -2910,11 +3135,16 @@ void _testFocusable() {
// Give up focus
manager.changeFocus(false);
pumpSemantics(); // triggers post-update callbacks
expect(capturedActions, isEmpty);
expect(domDocument.activeElement, element);

// Browser blurs the element
element.blur();
expect(domDocument.activeElement, isNot(element));
expect(capturedActions, <CapturedAction>[
(1, ui.SemanticsAction.didLoseAccessibilityFocus, null),
]);
capturedActions.clear();
expect(domDocument.activeElement, isNot(element));

// Request focus again
manager.changeFocus(true);
Expand All @@ -2925,20 +3155,29 @@ void _testFocusable() {
]);
capturedActions.clear();

// Double-request focus
manager.changeFocus(true);
pumpSemantics(); // triggers post-update callbacks
expect(domDocument.activeElement, element);
expect(
reason: 'Nothing should be sent to the framework on focus re-request.',
capturedActions, isEmpty);
capturedActions.clear();

// Stop managing
manager.stopManaging();
pumpSemantics(); // triggers post-update callbacks
expect(
reason: 'Even though the element was blurred after stopManaging there '
'should be no notification to the framework because the framework '
'should already know. Otherwise, it would not have asked to stop '
'managing the node.',
reason: 'There should be no notification to the framework because the '
'framework should already know. Otherwise, it would not have '
'asked to stop managing the node.',
capturedActions,
isEmpty,
);
expect(domDocument.activeElement, isNot(element));
expect(domDocument.activeElement, element);

// Attempt to request focus when not managing an element.
element.blur();
manager.changeFocus(true);
pumpSemantics(); // triggers post-update callbacks
expect(
Expand Down