From 149a99ca65a7487c5809dcdbec528abd28e722e0 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Thu, 2 Nov 2023 16:27:26 -0700 Subject: [PATCH 1/7] [web] autofocus in new routes --- .../lib/src/engine/semantics/checkable.dart | 3 + .../lib/src/engine/semantics/dialog.dart | 40 ++++++ .../lib/src/engine/semantics/focusable.dart | 57 +++++++-- .../lib/src/engine/semantics/image.dart | 3 + .../src/engine/semantics/incrementable.dart | 6 + lib/web_ui/lib/src/engine/semantics/link.dart | 3 + .../src/engine/semantics/platform_view.dart | 9 ++ .../lib/src/engine/semantics/scrollable.dart | 3 + .../lib/src/engine/semantics/semantics.dart | 121 +++++++++++++++++- .../lib/src/engine/semantics/tappable.dart | 3 + .../lib/src/engine/semantics/text_field.dart | 10 ++ 11 files changed, 242 insertions(+), 16 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/checkable.dart b/lib/web_ui/lib/src/engine/semantics/checkable.dart index d50f51c512c64..1840acc4aa6e1 100644 --- a/lib/web_ui/lib/src/engine/semantics/checkable.dart +++ b/lib/web_ui/lib/src/engine/semantics/checkable.dart @@ -102,4 +102,7 @@ class Checkable extends PrimaryRoleManager { removeAttribute('aria-disabled'); removeAttribute('disabled'); } + + @override + bool focusAsRouteDefault() => focusable?.focusAsRouteDefault() ?? false; } diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index 9f64e42e7acff..1221c6231006b 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -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 `isFocusabe` and `isFocused` flags. In this case, the node + // 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 + // expectation is to look through all nodes in traversal order and focus + // on the first one. + semanticsObject.owner.addOneTimePostUpdateCallback(() { + 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(); + }); + } + + 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), depair 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 one. It doesn't make sense to focus on the dialog itself. + return false; + } } /// Supplies a description for the nearest ancestor [Dialog]. diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 4caf56f3f3eac..ab89b492aab59 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -34,6 +34,12 @@ class Focusable extends RoleManager { final AccessibilityFocusManager _focusManager; + bool focusAsRouteDefault() { + print('>>> ${semanticsObject.id} is taking default route focus (label: ${semanticsObject.label})'); + owner.element.focus(); + return true; + } + @override void update() { if (semanticsObject.isFocusable) { @@ -84,6 +90,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 +150,7 @@ class AccessibilityFocusManager { void stopManaging() { final _FocusTarget? target = _target; _target = null; + _lastSetValue = null; if (target == null) { /// Nothing is being managed. Just return. @@ -144,11 +159,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 +184,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 +199,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 +237,8 @@ class AccessibilityFocusManager { return; } - if (value) { - target.element.focus(); - } else { - target.element.blur(); - } + print('>>> calling focus on ${target.element} id="${target.element.id}"'); + target.element.focus(); }); } } diff --git a/lib/web_ui/lib/src/engine/semantics/image.dart b/lib/web_ui/lib/src/engine/semantics/image.dart index efe1d7cdb414b..da36ad2a87b5d 100644 --- a/lib/web_ui/lib/src/engine/semantics/image.dart +++ b/lib/web_ui/lib/src/engine/semantics/image.dart @@ -23,6 +23,9 @@ class ImageRoleManager extends PrimaryRoleManager { addTappable(); } + @override + bool focusAsRouteDefault() => focusable?.focusAsRouteDefault() ?? false; + /// The element with role="img" and aria-label could block access to all /// children elements, therefore create an auxiliary element and describe the /// image in that if the semantic object have child nodes. diff --git a/lib/web_ui/lib/src/engine/semantics/incrementable.dart b/lib/web_ui/lib/src/engine/semantics/incrementable.dart index 5dceb9582c77c..6a8c846d0915c 100644 --- a/lib/web_ui/lib/src/engine/semantics/incrementable.dart +++ b/lib/web_ui/lib/src/engine/semantics/incrementable.dart @@ -59,6 +59,12 @@ class Incrementable extends PrimaryRoleManager { _focusManager.manage(semanticsObject.id, _element); } + @override + bool focusAsRouteDefault() { + _element.focus(); + return true; + } + /// The HTML element used to render semantics to the browser. final DomHTMLInputElement _element = createDomHTMLInputElement(); diff --git a/lib/web_ui/lib/src/engine/semantics/link.dart b/lib/web_ui/lib/src/engine/semantics/link.dart index 00dcdfcad54c5..168c93322c430 100644 --- a/lib/web_ui/lib/src/engine/semantics/link.dart +++ b/lib/web_ui/lib/src/engine/semantics/link.dart @@ -18,4 +18,7 @@ class Link extends PrimaryRoleManager { element.style.display = 'block'; return element; } + + @override + bool focusAsRouteDefault() => focusable?.focusAsRouteDefault() ?? false; } diff --git a/lib/web_ui/lib/src/engine/semantics/platform_view.dart b/lib/web_ui/lib/src/engine/semantics/platform_view.dart index 7502694390dc3..9426e524d04ea 100644 --- a/lib/web_ui/lib/src/engine/semantics/platform_view.dart +++ b/lib/web_ui/lib/src/engine/semantics/platform_view.dart @@ -38,4 +38,13 @@ class PlatformViewRoleManager extends PrimaryRoleManager { removeAttribute('aria-owns'); } } + + @override + bool focusAsRouteDefault() { + // It's unclear how it's possible to auto-focus on something inside a + // platform view without knowing what's in it. If the framework adds API for + // focusing on platform view internals, this method will be able to do more, + // but for now there's nothing to focus on. + return false; + } } diff --git a/lib/web_ui/lib/src/engine/semantics/scrollable.dart b/lib/web_ui/lib/src/engine/semantics/scrollable.dart index 61cb17f6ae611..dd73c57436ae3 100644 --- a/lib/web_ui/lib/src/engine/semantics/scrollable.dart +++ b/lib/web_ui/lib/src/engine/semantics/scrollable.dart @@ -239,4 +239,7 @@ class Scrollable extends PrimaryRoleManager { _gestureModeListener = null; } } + + @override + bool focusAsRouteDefault() => false; } diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index c88543b2d0331..22198549cd852 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -519,9 +519,13 @@ abstract class PrimaryRoleManager { void removeEventListener(String type, DomEventListener? listener, [bool? useCapture]) => element.removeEventListener(type, listener, useCapture); + /// Convenience getter for the [Focusable] role manager, if any. + Focusable? get focusable => _focusable; + Focusable? _focusable; + /// Adds generic focus management features. void addFocusManagement() { - addSecondaryRole(Focusable(semanticsObject, this)); + addSecondaryRole(_focusable = Focusable(semanticsObject, this)); } /// Adds generic live region features. @@ -594,6 +598,18 @@ abstract class PrimaryRoleManager { removeAttribute('role'); _isDisposed = true; } + + /// Transfers the accessibility focus to the [element] managed by this role + /// manager as a result of this node taking focus by default. + /// + /// For example, when a dialog pops up it is expected that one of its child + /// nodes takes accessibility focus. + /// + /// Transferring accessibility focus is different from transferring input + /// focus. Not all elements that can take accessibility focus can also take + /// input focus. For example, a plain text node cannot take input focus, but + /// it can take accessibility focus. + bool focusAsRouteDefault(); } /// A role used when a more specific role couldn't be assigned to the node. @@ -639,6 +655,38 @@ final class GenericRole extends PrimaryRoleManager { setAriaRole('text'); } } + + @override + bool focusAsRouteDefault() { + // Case 1: current node has input focus. Let the input focus system decide + // default focusability. + if (semanticsObject.isFocusable) { + final Focusable? focusable = this.focusable; + if (focusable != null) { + return focusable.focusAsRouteDefault(); + } + } + + // Case 2: current node is not focusable, but just a container of other + // nodes or lacks a label. Do not focus on it and let the search continue. + if (semanticsObject.hasChildren || !semanticsObject.hasLabel) { + return false; + } + + // Case 3: current node is visual/informational. Move just the + // accessibility focus. + + // Plain text nodes should not be focusable via keyboard or mouse. They are + // only focusable for the purposes of focusing the screen reader. To achieve + // this the -1 value is used. + // + // See also: + // + // https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex + element.tabIndex = -1; + element.focus(); + return true; + } } /// Provides a piece of functionality to a [SemanticsObject]. @@ -1719,16 +1767,59 @@ class SemanticsObject { } } - /// Recursively visits the tree rooted at `this` node in depth-first fashion. + /// Recursively visits the tree rooted at `this` node in depth-first fashion + /// in render order. /// /// Calls the [callback] for `this` node, then for all of its descendants. - void visitDepthFirst(void Function(SemanticsObject) callback) { + void visitDepthFirstInRenderOrder(void Function(SemanticsObject) callback) { callback(this); _currentChildrenInRenderOrder?.forEach((SemanticsObject child) { - child.visitDepthFirst(callback); + child.visitDepthFirstInRenderOrder(callback); }); } + /// Recursively visits the tree rooted at `this` node in depth-first fashion + /// in traversal order. + /// + /// Calls the [callback] for `this` node, then for all of its descendants. If + /// the callback returns true, continues visiting descendants. Otherwise, + /// stops immediately after visiting the node that caused the callback to + /// return false. + void visitDepthFirstInTraversalOrder(bool Function(SemanticsObject) callback) { + _visitDepthFirstInTraversalOrder(callback); + } + + bool _visitDepthFirstInTraversalOrder(bool Function(SemanticsObject) callback) { + final bool shouldContinueVisiting = callback(this); + + if (!shouldContinueVisiting) { + return false; + } + + final Int32List? childrenInTraversalOrder = _childrenInTraversalOrder; + + if (childrenInTraversalOrder == null) { + return true; + } + + for (final int childId in childrenInTraversalOrder) { + final SemanticsObject? child = owner._semanticsTree[childId]; + + assert( + child != null, + 'visitDepthFirstInTraversalOrder must only be called after the node ' + 'tree has been established. However, child #$childId does not have its ' + 'SemanticsNode created at the time this method was called.', + ); + + if (!child!._visitDepthFirstInTraversalOrder(callback)) { + return false; + } + } + + return true; + } + @override String toString() { String result = super.toString(); @@ -2170,7 +2261,7 @@ class EngineSemanticsOwner { // A detached node may or may not have some of its descendants reattached // elsewhere. Walk the descendant tree and find all descendants that were // reattached to a parent. Those descendants need to be removed. - detachmentRoot.visitDepthFirst((SemanticsObject node) { + detachmentRoot.visitDepthFirstInRenderOrder((SemanticsObject node) { final SemanticsObject? parent = _attachments[node.id]; if (parent == null) { // Was not reparented and is removed permanently from the tree. @@ -2202,6 +2293,7 @@ class EngineSemanticsOwner { } finally { _phase = SemanticsUpdatePhase.idle; } + _hasNodeRequestingFocus = false; } /// Returns the entire semantics tree for testing. @@ -2235,7 +2327,7 @@ class EngineSemanticsOwner { final SemanticsObject? root = _semanticsTree[0]; if (root != null) { - root.visitDepthFirst((SemanticsObject child) { + root.visitDepthFirstInRenderOrder((SemanticsObject child) { liveIds[child.id] = child._childrenInTraversalOrder?.toList() ?? const []; }); } @@ -2379,6 +2471,23 @@ AFTER: $description _phase = SemanticsUpdatePhase.idle; _oneTimePostUpdateCallbacks.clear(); } + + /// True, if any semantics node requested focus explicitly. + /// + /// Since focus can only be taken by no more than one element, the engine + /// should not request focus for multiple elements. This flag helps resolve + /// that. + bool get hasNodeRequestingFocus => _hasNodeRequestingFocus; + bool _hasNodeRequestingFocus = false; + + /// Declares that a semantics node will explicitly request focus. + /// + /// This prevents others, [Dialog] in particular, from requesting autofocus, + /// as focus can only be taken by one element. Explicit focus has higher + /// precedence than autofocus. + void willRequestFocus() { + _hasNodeRequestingFocus = true; + } } /// Computes the [longest increasing subsequence](http://en.wikipedia.org/wiki/Longest_increasing_subsequence). diff --git a/lib/web_ui/lib/src/engine/semantics/tappable.dart b/lib/web_ui/lib/src/engine/semantics/tappable.dart index e0cdfbb2ebf37..e1abbc2f52a0c 100644 --- a/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/lib/web_ui/lib/src/engine/semantics/tappable.dart @@ -11,6 +11,9 @@ class Button extends PrimaryRoleManager { setAriaRole('button'); } + @override + bool focusAsRouteDefault() => focusable?.focusAsRouteDefault() ?? false; + @override void update() { super.update(); diff --git a/lib/web_ui/lib/src/engine/semantics/text_field.dart b/lib/web_ui/lib/src/engine/semantics/text_field.dart index c3195c1d414ac..120c329d63b35 100644 --- a/lib/web_ui/lib/src/engine/semantics/text_field.dart +++ b/lib/web_ui/lib/src/engine/semantics/text_field.dart @@ -226,6 +226,16 @@ class TextField extends PrimaryRoleManager { return editableElement!; } + @override + bool focusAsRouteDefault() { + final DomHTMLElement? editableElement = this.editableElement; + if (editableElement == null) { + return false; + } + editableElement.focus(); + return true; + } + /// Timer that times when to set the location of the input text. /// /// This is only used for iOS. In iOS, virtual keyboard shifts the screen. From b564a5a00c84fb02d300ab9d4f4b3bf46f323143 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Mon, 13 Nov 2023 12:44:16 -0800 Subject: [PATCH 2/7] add test; clean-up --- .../lib/src/engine/semantics/dialog.dart | 6 +- .../lib/src/engine/semantics/focusable.dart | 11 +- .../test/engine/semantics/semantics_test.dart | 257 +++++++++++++++++- 3 files changed, 260 insertions(+), 14 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index 1221c6231006b..0eab7758a1d93 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -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 // 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 // expectation is to look through all nodes in traversal order and focus @@ -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; }); @@ -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; } } diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index ab89b492aab59..e0fbf54674d74 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -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. bool focusAsRouteDefault() { - print('>>> ${semanticsObject.id} is taking default route focus (label: ${semanticsObject.label})'); owner.element.focus(); return true; } @@ -237,7 +245,6 @@ class AccessibilityFocusManager { return; } - print('>>> calling focus on ${target.element} id="${target.element.id}"'); target.element.focus(); }); } diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 4cc3c8bd60246..8a7bab3a5c893 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -577,6 +577,11 @@ class MockRoleManager extends PrimaryRoleManager { super.update(); _log('update'); } + + @override + bool focusAsRouteDefault() { + throw UnimplementedError(); + } } class MockSemanticsEnabler implements SemanticsEnabler { @@ -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, [ (0, ui.SemanticsAction.didGainAccessibilityFocus, null), ]); + capturedActions.clear(); pumpSemantics(isFocused: false); + expect( + reason: 'The engine never calls blur() explicitly.', + capturedActions, + isEmpty, + ); + + element.blur(); expect(capturedActions, [ - (0, ui.SemanticsAction.didGainAccessibilityFocus, null), (0, ui.SemanticsAction.didLoseAccessibilityFocus, null), ]); @@ -1897,16 +1910,20 @@ void _testCheckables() { }; pumpSemantics(isFocused: false); + final DomElement element = semantics().debugSemanticsTree![0]!.element; expect(capturedActions, isEmpty); pumpSemantics(isFocused: true); expect(capturedActions, [ (0, ui.SemanticsAction.didGainAccessibilityFocus, null), ]); + capturedActions.clear(); pumpSemantics(isFocused: false); + expect(capturedActions, isEmpty); + + element.blur(); expect(capturedActions, [ - (0, ui.SemanticsAction.didGainAccessibilityFocus, null), (0, ui.SemanticsAction.didLoseAccessibilityFocus, null), ]); @@ -2069,16 +2086,20 @@ void _testTappable() { }; pumpSemantics(isFocused: false); + final DomElement element = semantics().debugSemanticsTree![0]!.element; expect(capturedActions, isEmpty); pumpSemantics(isFocused: true); expect(capturedActions, [ (0, ui.SemanticsAction.didGainAccessibilityFocus, null), ]); + capturedActions.clear(); pumpSemantics(isFocused: false); + expect(capturedActions, isEmpty); + + element.blur(); expect(capturedActions, [ - (0, ui.SemanticsAction.didGainAccessibilityFocus, null), (0, ui.SemanticsAction.didLoseAccessibilityFocus, null), ]); @@ -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 + // first descendant will be auto-focused. + test('focuses on the first unfocused Focusable', () async { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + final List capturedActions = []; + 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: [ + tester.updateNode( + id: 1, + children: [ + 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, + [ + (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 + // 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 capturedActions = []; + 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: [ + tester.updateNode( + id: 1, + children: [ + 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, + [ + (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 + // descendant will be auto-focused. + test('focuses on the first non-focusable descedant', () async { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + final List capturedActions = []; + 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: [ + tester.updateNode( + id: 1, + children: [ + 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 capturedActions = []; + 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); @@ -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, [ (1, ui.SemanticsAction.didLoseAccessibilityFocus, null), ]); capturedActions.clear(); - expect(domDocument.activeElement, isNot(element)); // Request focus again manager.changeFocus(true); @@ -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( From 59b11d746418e4a98edd6a34f32a1f86da53afcd Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Mon, 4 Dec 2023 14:46:32 -0800 Subject: [PATCH 3/7] address comments --- .../lib/src/engine/semantics/semantics.dart | 10 ++++- .../test/engine/semantics/semantics_test.dart | 38 ++++++++++++------- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 22198549cd852..4e6316e134ab9 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -609,6 +609,10 @@ abstract class PrimaryRoleManager { /// focus. Not all elements that can take accessibility focus can also take /// input focus. For example, a plain text node cannot take input focus, but /// it can take accessibility focus. + /// + /// Returns `true` if the this 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(); } @@ -2472,7 +2476,11 @@ AFTER: $description _oneTimePostUpdateCallbacks.clear(); } - /// True, if any semantics node requested focus explicitly. + /// True, if any semantics node requested focus explicitly during the latest + /// semantics update. + /// + /// The default value is `false`, and it is reset back to `false` after the + /// semantics update at the end of [updateSemantics]. /// /// Since focus can only be taken by no more than one element, the engine /// should not request focus for multiple elements. This flag helps resolve diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 8a7bab3a5c893..d118ea5d7474a 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -1542,7 +1542,7 @@ void _testIncrementables() { }; pumpSemantics(isFocused: false); - final DomElement element = semantics().debugSemanticsTree![0]!.element.querySelector('input')!; + final DomElement element = owner().debugSemanticsTree![0]!.element.querySelector('input')!; expect(capturedActions, isEmpty); pumpSemantics(isFocused: true); @@ -1910,7 +1910,7 @@ void _testCheckables() { }; pumpSemantics(isFocused: false); - final DomElement element = semantics().debugSemanticsTree![0]!.element; + final DomElement element = owner().debugSemanticsTree![0]!.element; expect(capturedActions, isEmpty); pumpSemantics(isFocused: true); @@ -1919,9 +1919,17 @@ void _testCheckables() { ]); capturedActions.clear(); + // The framework removes focus from the widget (i.e. "blurs" it). Since the + // blurring is initiated by the framework, there's no need to send any + // notifications back to the framework about it. 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. element.blur(); expect(capturedActions, [ (0, ui.SemanticsAction.didLoseAccessibilityFocus, null), @@ -2086,7 +2094,7 @@ void _testTappable() { }; pumpSemantics(isFocused: false); - final DomElement element = semantics().debugSemanticsTree![0]!.element; + final DomElement element = owner().debugSemanticsTree![0]!.element; expect(capturedActions, isEmpty); pumpSemantics(isFocused: true); @@ -2879,7 +2887,7 @@ void _testDialog() { }); // Test the simple scenario of a dialog coming up and containing focusable - // descentants that are not initially focused. The expectation is that the + // descendants that are not initially focused. The expectation is that the // first descendant will be auto-focused. test('focuses on the first unfocused Focusable', () async { semantics() @@ -2891,7 +2899,7 @@ void _testDialog() { capturedActions.add((event.nodeId, event.type, event.arguments)); }; - final SemanticsTester tester = SemanticsTester(semantics()); + final SemanticsTester tester = SemanticsTester(owner()); tester.updateNode( id: 0, scopesRoute: true, @@ -2899,6 +2907,8 @@ void _testDialog() { children: [ tester.updateNode( id: 1, + // None of the children should have isFocused set to `true` to make + // sure that the auto-focus logic kicks in. children: [ tester.updateNode( id: 2, @@ -2939,7 +2949,7 @@ void _testDialog() { }); // Test the scenario of a dialog coming up and containing focusable - // descentants with one of them explicitly requesting focus. The expectation + // descendants with one of them explicitly requesting focus. The expectation // 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 { @@ -2952,7 +2962,7 @@ void _testDialog() { capturedActions.add((event.nodeId, event.type, event.arguments)); }; - final SemanticsTester tester = SemanticsTester(semantics()); + final SemanticsTester tester = SemanticsTester(owner()); tester.updateNode( id: 0, scopesRoute: true, @@ -2980,6 +2990,7 @@ void _testDialog() { isEnabled: true, isButton: true, isFocusable: true, + // Asked for focus explicitly. isFocused: true, rect: const ui.Rect.fromLTRB(0, 0, 100, 50), ), @@ -3000,8 +3011,8 @@ void _testDialog() { }); // Test the scenario of a dialog coming up and containing non-focusable - // descentants that can have a11y focus. The expectation is that the first - // descendant will be auto-focused. + // descendants that can have a11y focus. The expectation is that the first + // descendant will be auto-focused, even if it's not input-focusable. test('focuses on the first non-focusable descedant', () async { semantics() ..debugOverrideTimestampFunction(() => _testTime) @@ -3012,7 +3023,7 @@ void _testDialog() { capturedActions.add((event.nodeId, event.type, event.arguments)); }; - final SemanticsTester tester = SemanticsTester(semantics()); + final SemanticsTester tester = SemanticsTester(owner()); tester.updateNode( id: 0, scopesRoute: true, @@ -3048,7 +3059,7 @@ void _testDialog() { expect(capturedActions, isEmpty); // However, the element should have gotten the focus. - final DomElement element = semantics().debugSemanticsTree![2]!.element; + final DomElement element = owner().debugSemanticsTree![2]!.element; expect(element.tabIndex, -1); expect(domDocument.activeElement, element); @@ -3067,7 +3078,7 @@ void _testDialog() { capturedActions.add((event.nodeId, event.type, event.arguments)); }; - final SemanticsTester tester = SemanticsTester(semantics()); + final SemanticsTester tester = SemanticsTester(owner()); tester.updateNode( id: 0, scopesRoute: true, @@ -3075,9 +3086,8 @@ void _testDialog() { ); tester.apply(); - // The focused node is not focusable, so no notification is sent to the - // framework. expect(capturedActions, isEmpty); + expect(domDocument.activeElement, domDocument.body); semantics().semanticsEnabled = false; }); From 7b6952c880c4b6afbbb9d2f112663c5698ee37ac Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 5 Dec 2023 13:31:11 -0800 Subject: [PATCH 4/7] address comments --- lib/web_ui/lib/src/engine/semantics/focusable.dart | 4 ++++ lib/web_ui/lib/src/engine/semantics/scrollable.dart | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index e0fbf54674d74..68b2781487098 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -43,6 +43,10 @@ class Focusable extends RoleManager { /// explicitly request focus. Instead, the DOM element is being focus directly /// programmatically, simulating the screen reader choosing a default element /// to focus on. + /// + /// Returns `true` if the this 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() { owner.element.focus(); return true; diff --git a/lib/web_ui/lib/src/engine/semantics/scrollable.dart b/lib/web_ui/lib/src/engine/semantics/scrollable.dart index dd73c57436ae3..3c0eb2b6392c3 100644 --- a/lib/web_ui/lib/src/engine/semantics/scrollable.dart +++ b/lib/web_ui/lib/src/engine/semantics/scrollable.dart @@ -241,5 +241,5 @@ class Scrollable extends PrimaryRoleManager { } @override - bool focusAsRouteDefault() => false; + bool focusAsRouteDefault() => focusable?.focusAsRouteDefault() ?? false; } From a182001c385c34e2c4742cbdd9a73d83d12441ad Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Mon, 18 Dec 2023 15:37:22 -0800 Subject: [PATCH 5/7] fix "the this" --- lib/web_ui/lib/src/engine/semantics/focusable.dart | 2 +- lib/web_ui/lib/src/engine/semantics/semantics.dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 68b2781487098..39a4ae77d931f 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -44,7 +44,7 @@ class Focusable extends RoleManager { /// programmatically, simulating the screen reader choosing a default element /// to focus on. /// - /// Returns `true` if the this role manager took the focus. Returns `false` if + /// 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() { diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 4e6316e134ab9..5446c2083417c 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -610,7 +610,7 @@ abstract class PrimaryRoleManager { /// input focus. For example, a plain text node cannot take input focus, but /// it can take accessibility focus. /// - /// Returns `true` if the this role manager took the focus. Returns `false` if + /// 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(); From dd39594d297174191a23cbe006d20ba693f5cab0 Mon Sep 17 00:00:00 2001 From: Yegor Date: Wed, 20 Dec 2023 12:57:40 -0800 Subject: [PATCH 6/7] Update lib/web_ui/lib/src/engine/semantics/focusable.dart Co-authored-by: Mouad Debbar --- lib/web_ui/lib/src/engine/semantics/focusable.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 39a4ae77d931f..35fff64a50158 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -39,7 +39,7 @@ class Focusable extends RoleManager { /// 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 + /// 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. From c3f9ffd0f6772e8e6730eb5b11d8e086da7ae299 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Wed, 20 Dec 2023 15:21:52 -0800 Subject: [PATCH 7/7] clean up visitDepthFirstInRenderOrder --- .../lib/src/engine/semantics/semantics.dart | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 5446c2083417c..b65f4484b7a74 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -1772,13 +1772,18 @@ class SemanticsObject { } /// Recursively visits the tree rooted at `this` node in depth-first fashion - /// in render order. + /// in the order nodes were rendered into the DOM. + /// + /// Useful for debugging only. /// /// Calls the [callback] for `this` node, then for all of its descendants. - void visitDepthFirstInRenderOrder(void Function(SemanticsObject) callback) { + /// + /// Unlike [visitDepthFirstInTraversalOrder] this method can traverse + /// partially updated, incomplete, or inconsistent tree. + void _debugVisitRenderedSemanticNodesDepthFirst(void Function(SemanticsObject) callback) { callback(this); _currentChildrenInRenderOrder?.forEach((SemanticsObject child) { - child.visitDepthFirstInRenderOrder(callback); + child._debugVisitRenderedSemanticNodesDepthFirst(callback); }); } @@ -2265,7 +2270,7 @@ class EngineSemanticsOwner { // A detached node may or may not have some of its descendants reattached // elsewhere. Walk the descendant tree and find all descendants that were // reattached to a parent. Those descendants need to be removed. - detachmentRoot.visitDepthFirstInRenderOrder((SemanticsObject node) { + detachmentRoot.visitDepthFirstInTraversalOrder((SemanticsObject node) { final SemanticsObject? parent = _attachments[node.id]; if (parent == null) { // Was not reparented and is removed permanently from the tree. @@ -2274,8 +2279,8 @@ class EngineSemanticsOwner { assert(node._parent == parent); assert(node.element.parentNode == parent._childContainerElement); } + return true; }); - } for (final SemanticsObject removal in removals) { @@ -2331,7 +2336,7 @@ class EngineSemanticsOwner { final SemanticsObject? root = _semanticsTree[0]; if (root != null) { - root.visitDepthFirstInRenderOrder((SemanticsObject child) { + root._debugVisitRenderedSemanticNodesDepthFirst((SemanticsObject child) { liveIds[child.id] = child._childrenInTraversalOrder?.toList() ?? const []; }); }