-
Couldn't load subscription status.
- Fork 6k
[web] autofocus in new routes #47727
Changes from all commits
149a99c
b564a5a
59b11d7
7b6952c
a182001
dd39594
c3f9ffd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,39 @@ class Dialog extends PrimaryRoleManager { | |
| // names its own route an `aria-label` is used instead of `aria-describedby`. | ||
| addFocusManagement(); | ||
| addLiveRegion(); | ||
|
|
||
| // When a route/dialog shows up it is expected that the screen reader will | ||
| // focus on something inside it. There could be two possibilities: | ||
| // | ||
| // 1. The framework explicitly marked a node inside the dialog as focused | ||
| // via the `isFocusable` and `isFocused` flags. In this case, the node | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It really depends on the type of dialog. For example for a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| // 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it has been implemented on ios and android now. |
||
| // expectation is to look through all nodes in traversal order and focus | ||
| // on the first one. | ||
| semanticsObject.owner.addOneTimePostUpdateCallback(() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does the debouncing work if there are multiple dialogs created in a update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not considered it. I hope apps do not create multiple dialogs in a single update. It seems like a messy UX implementation. I think the framework removes semantics from all background routes anyway, so the engine would only observe the top-most one, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if there are nested navigators or a pop up route like dropdownButtons that somehow open simultaneous , the former is probably more common than the latter. Another case is that I do know customer money use scopeRoute + namesRoute combo when they are animating page change effect within one page, but they are targeting mobile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Looking at the code, what will happen right now is that the last dialog to show up will "win" the focus because |
||
| if (semanticsObject.owner.hasNodeRequestingFocus) { | ||
| // Case 1: a node requested explicit focus. Nothing extra to do. | ||
| return; | ||
| } | ||
|
|
||
| // Case 2: nothing requested explicit focus. Focus on the first descendant. | ||
| _setDefaultFocus(); | ||
|
Comment on lines
+35
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may need to add a 3rd case:
This 3rd case will be common when Flutter is embedded inside another app, where the screen reader might be focusing somewhere else outside of the Flutter app. We don't want to steal that focus, do we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If something in the embedded flutter view triggered a route change inside, we should bring the focus here I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other platform we send screen change accessibilityEvent and let OS decide whether to move the accessibility focus. so we are in an uncharted territory here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we have focusability at the In the meantime, I agree with @chunhtai that if a route change happened, it is likely because the user took some action to push that route, and the most common outcome should be that a widget inside the new route takes focus. We can revisit when we have |
||
| }); | ||
| } | ||
|
|
||
| void _setDefaultFocus() { | ||
| semanticsObject.visitDepthFirstInTraversalOrder((SemanticsObject node) { | ||
| final PrimaryRoleManager? roleManager = node.primaryRole; | ||
| if (roleManager == null) { | ||
| return true; | ||
| } | ||
|
|
||
| // If the node does not take focus (e.g. focusing on it does not make | ||
| // sense at all). Despair not. Keep looking. | ||
| final bool didTakeFocus = roleManager.focusAsRouteDefault(); | ||
| return !didTakeFocus; | ||
| }); | ||
| } | ||
|
|
||
| @override | ||
|
|
@@ -57,6 +90,13 @@ class Dialog extends PrimaryRoleManager { | |
| routeName.semanticsObject.element.id, | ||
| ); | ||
| } | ||
|
|
||
| @override | ||
| bool focusAsRouteDefault() { | ||
| // Dialogs are the ones that look inside themselves to find elements to | ||
| // focus on. It doesn't make sense to focus on the dialog itself. | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /// Supplies a description for the nearest ancestor [Dialog]. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,24 @@ 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 did 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do inform the framework via a |
||
| /// | ||
| /// Returns `true` if the role manager took the focus. Returns `false` if | ||
| /// this role manager did not take the focus. The return value can be used to | ||
| /// decide whether to stop searching for a node that should take focus. | ||
| bool focusAsRouteDefault() { | ||
marcianx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| owner.element.focus(); | ||
| return true; | ||
| } | ||
|
|
||
| @override | ||
| void update() { | ||
| if (semanticsObject.isFocusable) { | ||
|
|
@@ -84,6 +102,14 @@ class AccessibilityFocusManager { | |
|
|
||
| _FocusTarget? _target; | ||
|
|
||
| // The last focus value set by this focus manager, used to prevent requesting | ||
| // focus on the same element repeatedly. Requesting focus on DOM elements is | ||
| // not an idempotent operation. If the element is already focused and focus is | ||
| // requested the browser will scroll to that element. However, scrolling is | ||
| // not this class' concern and so this class should avoid doing anything that | ||
| // would affect scrolling. | ||
| bool? _lastSetValue; | ||
|
|
||
| /// Whether this focus manager is managing a focusable target. | ||
| bool get isManaging => _target != null; | ||
|
|
||
|
|
@@ -136,6 +162,7 @@ class AccessibilityFocusManager { | |
| void stopManaging() { | ||
| final _FocusTarget? target = _target; | ||
| _target = null; | ||
| _lastSetValue = null; | ||
|
|
||
| if (target == null) { | ||
| /// Nothing is being managed. Just return. | ||
|
|
@@ -144,11 +171,6 @@ class AccessibilityFocusManager { | |
|
|
||
| target.element.removeEventListener('focus', target.domFocusListener); | ||
| target.element.removeEventListener('blur', target.domBlurListener); | ||
|
|
||
| // Blur the element after removing listeners. If this method is being called | ||
| // it indicates that the framework already knows that this node should not | ||
| // have focus, and there's no need to notify it. | ||
| target.element.blur(); | ||
| } | ||
|
|
||
| void _setFocusFromDom(bool acquireFocus) { | ||
|
|
@@ -174,6 +196,10 @@ class AccessibilityFocusManager { | |
| final _FocusTarget? target = _target; | ||
|
|
||
| if (target == null) { | ||
| // If this branch is being executed, there's a bug somewhere already, but | ||
| // it doesn't hurt to clean up old values anyway. | ||
| _lastSetValue = null; | ||
|
|
||
| // Nothing is being managed right now. | ||
| assert(() { | ||
| printWarning( | ||
|
|
@@ -185,6 +211,32 @@ class AccessibilityFocusManager { | |
| return; | ||
| } | ||
|
|
||
| if (value == _lastSetValue) { | ||
| // The focus is being changed to a value that's already been requested in | ||
| // the past. Do nothing. | ||
| return; | ||
| } | ||
| _lastSetValue = value; | ||
|
|
||
| if (value) { | ||
| _owner.willRequestFocus(); | ||
| } else { | ||
| // Do not blur elements. Instead let the element be blurred by requesting | ||
| // focus elsewhere. Blurring elements is a very error-prone thing to do, | ||
| // as it is subject to non-local effects. Let's say the framework decides | ||
| // that a semantics node is currently not focused. That would lead to | ||
| // changeFocus(false) to be called. However, what if this node is inside | ||
| // 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 | ||
| // 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 | ||
| // the dialog requesting focus on the same element. | ||
| return; | ||
| } | ||
|
|
||
| // Delay the focus request until the final DOM structure is established | ||
| // because the element may not yet be attached to the DOM, or it may be | ||
| // reparented and lose focus again. | ||
|
|
@@ -197,11 +249,7 @@ class AccessibilityFocusManager { | |
| return; | ||
| } | ||
|
|
||
| if (value) { | ||
| target.element.focus(); | ||
| } else { | ||
| target.element.blur(); | ||
| } | ||
| target.element.focus(); | ||
| }); | ||
| } | ||
| } | ||
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.
should this class be named Route instead? It took me a while to understand a dialog is corresponding to scopesRoute = true
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
PrimaryRoleManageris calledDialogbecause it sets the ARIArole="dialog", which is the unique role identifier. I can rename it, but I'd prefer doing it in a separate PR.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.
If every route is a role="dialog", wouldn't the screen reader announce every MaterialPageRoute a dialog when focused? Moving this to a separate pr sgtm
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.
Currently "dialog" is the only available ARIA role. A route expressed as a transition to a different screen is expressed on the web by changing the URL (which we already do via
UrlStrategy) and updating the title of the page (which is expressed via the Title widget). So I think for routes-that-are-not-dialogs we could improve the situation if the framework communicated the difference somehow. Happy to collaborate on improving this.