From 40dc0395c0870f0ab874e0efe2b02871ac3845b7 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Thu, 13 Jul 2023 17:55:08 -0700 Subject: [PATCH 1/4] [web] update role manager list --- .../lib/src/engine/semantics/semantics.dart | 86 ++++++++++++---- .../test/engine/semantics/semantics_test.dart | 99 +++++++++++++++++++ 2 files changed, 165 insertions(+), 20 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 8e3971b6dfc1b..176d2fc3abb8a 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -413,6 +413,14 @@ enum Role { routeName, } +/// Creates a secondary [RoleManager] for a [SemanticsObject]. +/// +/// The implementation is expected to inspect the semantics object (flags, +/// actions, etc) and determine whether it _should_ have this secondary role. +/// If the node should _not_ have this role, the funtion would return `null`. +/// Otherwise, the function would instantiate the role manager and return it. +typedef RoleManagerFactory = RoleManager? Function(SemanticsObject); + /// Responsible for setting the `role` ARIA attribute and for attaching zero or /// more secondary [RoleManager]s to a [SemanticsObject]. abstract class PrimaryRoleManager { @@ -443,6 +451,9 @@ abstract class PrimaryRoleManager { List? get secondaryRoleManagers => _secondaryRoleManagers; List? _secondaryRoleManagers; + /// Factories for role managers that have not yet been instantiated. + Map? _secondaryRoleFactories; + /// Identifiers of secondary roles used by this primary role manager. /// /// This is only meant to be used in tests. @@ -451,37 +462,52 @@ abstract class PrimaryRoleManager { /// Adds generic focus management features, if applicable. void addFocusManagement() { - if (semanticsObject.isFocusable) { - addSecondaryRole(Focusable(semanticsObject)); - } + addSecondaryRole(Role.focusable, (SemanticsObject semanticsObject) { + if (semanticsObject.isFocusable) { + return Focusable(semanticsObject); + } + return null; + }); } /// Adds generic live region features, if applicable. void addLiveRegion() { - if (semanticsObject.isLiveRegion) { - addSecondaryRole(LiveRegion(semanticsObject)); - } + addSecondaryRole(Role.liveRegion, (SemanticsObject semanticsObject) { + if (semanticsObject.isLiveRegion) { + return LiveRegion(semanticsObject); + } + return null; + }); } /// Adds generic route name features, if applicable. void addRouteName() { - if (semanticsObject.namesRoute) { - addSecondaryRole(RouteName(semanticsObject)); - } + addSecondaryRole(Role.routeName, (SemanticsObject semanticsObject) { + if (semanticsObject.namesRoute) { + return RouteName(semanticsObject); + } + return null; + }); } /// Adds generic label features, if applicable. void addLabelAndValue() { - if (semanticsObject.hasLabel || semanticsObject.hasValue || semanticsObject.hasTooltip) { - addSecondaryRole(LabelAndValue(semanticsObject)); - } + addSecondaryRole(Role.labelAndValue, (SemanticsObject semanticsObject) { + if (semanticsObject.hasLabel || semanticsObject.hasValue || semanticsObject.hasTooltip) { + return LabelAndValue(semanticsObject); + } + return null; + }); } /// Adds generic functionality for handling taps and clicks. void addTappable() { - if (semanticsObject.isTappable) { - addSecondaryRole(Tappable(semanticsObject)); - } + addSecondaryRole(Role.tappable, (SemanticsObject semanticsObject) { + if (semanticsObject.isTappable) { + return Tappable(semanticsObject); + } + return null; + }); } /// Adds a secondary role to this primary role manager. @@ -489,13 +515,16 @@ abstract class PrimaryRoleManager { /// This method should be called by concrete implementations of /// [PrimaryRoleManager] during initialization. @protected - void addSecondaryRole(RoleManager secondaryRoleManager) { + void addSecondaryRole(Role role, RoleManagerFactory roleManagerFactory) { + _secondaryRoleFactories ??= {}; + final Map secondaryRoleFactories = _secondaryRoleFactories!; + assert( - _secondaryRoleManagers?.any((RoleManager manager) => manager.role == secondaryRoleManager.role) != true, - 'Cannot add secondary role ${secondaryRoleManager.role}. This object already has this secondary role.', + !secondaryRoleFactories.containsKey(role), + 'Cannot add secondary role $role. This semantic node already has this role.', ); - _secondaryRoleManagers ??= []; - _secondaryRoleManagers!.add(secondaryRoleManager); + + secondaryRoleFactories[role] = roleManagerFactory; } /// Called immediately after the fields of the [semanticsObject] are updated @@ -509,6 +538,23 @@ abstract class PrimaryRoleManager { /// the object. @mustCallSuper void update() { + final Map? secondaryRoleFactories = _secondaryRoleFactories; + if (secondaryRoleFactories != null && secondaryRoleFactories.isNotEmpty) { + secondaryRoleFactories.forEach((Role role, RoleManagerFactory roleFactory) { + final RoleManager? secondaryRoleManager = roleFactory(semanticsObject); + if (secondaryRoleManager != null) { + _secondaryRoleManagers ??= []; + _secondaryRoleManagers!.add(secondaryRoleManager); + } + }); + + // A role manager should only be added once. So remove any factories that + // contributed a role manager. + _secondaryRoleManagers?.forEach((RoleManager roleManager) { + secondaryRoleFactories.remove(roleManager.role); + }); + } + final List? secondaryRoles = _secondaryRoleManagers; if (secondaryRoles == null) { return; diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 03a030358a74c..99590f66bfb5c 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -44,6 +44,9 @@ void runSemanticsTests() { group('longestIncreasingSubsequence', () { _testLongestIncreasingSubsequence(); }); + group('Role managers', () { + _testRoleManagerLifecycle(); + }); group('container', () { _testContainer(); }); @@ -91,6 +94,102 @@ void runSemanticsTests() { }); } +void _testRoleManagerLifecycle() { + test('Secondary role managers are added upon node creation', () { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + final SemanticsTester tester = SemanticsTester(semantics()); + tester.updateNode( + id: 0, + isFocusable: true, + label: 'this is a label', + hasTap: true, + hasEnabledState: true, + isEnabled: true, + isButton: true, + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ); + tester.apply(); + + expectSemanticsTree(''' + +'''); + + final SemanticsObject node = semantics().debugSemanticsTree![0]!; + expect(node.primaryRole?.role, PrimaryRole.button); + expect( + node.primaryRole?.debugSecondaryRoles, + containsAll([Role.focusable, Role.tappable, Role.labelAndValue]), + ); + expect(tester.getSemanticsObject(0).element.tabIndex, 0); + + semantics().semanticsEnabled = false; + }); + + test('Secondary role managers are added upon node update', () { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + // Create without a label + { + final SemanticsTester tester = SemanticsTester(semantics()); + tester.updateNode( + id: 0, + isFocusable: true, + label: 'this is a label', + hasTap: true, + hasEnabledState: true, + isEnabled: true, + isButton: true, + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ); + tester.apply(); + + expectSemanticsTree(''' + +'''); + + final SemanticsObject node = semantics().debugSemanticsTree![0]!; + expect(node.primaryRole?.role, PrimaryRole.button); + expect( + node.primaryRole?.debugSecondaryRoles, + containsAll([Role.focusable, Role.tappable]), + ); + expect(tester.getSemanticsObject(0).element.tabIndex, 0); + } + + // Update with a label + { + final SemanticsTester tester = SemanticsTester(semantics()); + tester.updateNode( + id: 0, + isFocusable: true, + hasTap: true, + hasEnabledState: true, + isEnabled: true, + isButton: true, + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ); + tester.apply(); + + expectSemanticsTree(''); + + final SemanticsObject node = semantics().debugSemanticsTree![0]!; + expect(node.primaryRole?.role, PrimaryRole.button); + expect( + node.primaryRole?.debugSecondaryRoles, + containsAll([Role.focusable, Role.tappable]), + ); + expect(tester.getSemanticsObject(0).element.tabIndex, 0); + } + + semantics().semanticsEnabled = false; + }); +} + void _testEngineAccessibilityBuilder() { final EngineAccessibilityFeaturesBuilder builder = EngineAccessibilityFeaturesBuilder(0); From f5e3789b54cf258b7ee97fe1c29f8cb40d30dc6b Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Fri, 14 Jul 2023 14:18:16 -0700 Subject: [PATCH 2/4] always add secondary role managers --- .../lib/src/engine/semantics/dialog.dart | 4 + .../lib/src/engine/semantics/focusable.dart | 17 ++++ .../src/engine/semantics/label_and_value.dart | 1 - .../lib/src/engine/semantics/live_region.dart | 4 + .../lib/src/engine/semantics/semantics.dart | 91 +++---------------- .../lib/src/engine/semantics/tappable.dart | 5 +- .../test/engine/semantics/semantics_test.dart | 59 ++---------- 7 files changed, 50 insertions(+), 131 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index 7e8f58ec6c73d..673702cecb775 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -67,6 +67,10 @@ class RouteName extends RoleManager { @override void update() { + if (!semanticsObject.namesRoute) { + return; + } + // NOTE(yjbanov): this does not handle the case when the node structure // changes such that this RouteName is no longer attached to the same // dialog. While this is technically expressible using the semantics API, diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 4de57af1c6948..673f12e372443 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -38,6 +38,20 @@ class Focusable extends RoleManager { @override void update() { + if (!_focusManager.isManaging && !semanticsObject.isFocusable) { + // Nothing to do about this node. It's neither focusable, nor being managed. + return; + } + + if (!_focusManager.isManaging) { + // This line is only reachable iff the node is focusable but not being + // managed. So the focus manager is told to start managing it. + _focusManager.manage(semanticsObject.id, semanticsObject.element); + } + _updateFocus(); + } + + void _updateFocus() { _focusManager.changeFocus(semanticsObject.hasFocus && (!semanticsObject.hasEnabledState || semanticsObject.isEnabled)); } @@ -79,6 +93,9 @@ class AccessibilityFocusManager { _FocusTarget? _target; + /// Whether this focus manager is managing a focusable target. + bool get isManaging => _target != null; + /// Starts managing the focus of the given [element]. /// /// The "focus" and "blur" DOM events are forwarded to the framework-side diff --git a/lib/web_ui/lib/src/engine/semantics/label_and_value.dart b/lib/web_ui/lib/src/engine/semantics/label_and_value.dart index 643f305fc7287..23421e13590d1 100644 --- a/lib/web_ui/lib/src/engine/semantics/label_and_value.dart +++ b/lib/web_ui/lib/src/engine/semantics/label_and_value.dart @@ -68,7 +68,6 @@ class LabelAndValue extends RoleManager { void _cleanUpDom() { semanticsObject.element.removeAttribute('aria-label'); - semanticsObject.clearAriaRole(); } @override diff --git a/lib/web_ui/lib/src/engine/semantics/live_region.dart b/lib/web_ui/lib/src/engine/semantics/live_region.dart index 239b9b600a13d..c922cddd717b7 100644 --- a/lib/web_ui/lib/src/engine/semantics/live_region.dart +++ b/lib/web_ui/lib/src/engine/semantics/live_region.dart @@ -22,6 +22,10 @@ class LiveRegion extends RoleManager { @override void update() { + if (!semanticsObject.isLiveRegion) { + return; + } + // Avoid announcing the same message over and over. if (_lastAnnouncement != semanticsObject.label) { _lastAnnouncement = semanticsObject.label; diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 176d2fc3abb8a..8f19b2ef10e72 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -413,14 +413,6 @@ enum Role { routeName, } -/// Creates a secondary [RoleManager] for a [SemanticsObject]. -/// -/// The implementation is expected to inspect the semantics object (flags, -/// actions, etc) and determine whether it _should_ have this secondary role. -/// If the node should _not_ have this role, the funtion would return `null`. -/// Otherwise, the function would instantiate the role manager and return it. -typedef RoleManagerFactory = RoleManager? Function(SemanticsObject); - /// Responsible for setting the `role` ARIA attribute and for attaching zero or /// more secondary [RoleManager]s to a [SemanticsObject]. abstract class PrimaryRoleManager { @@ -451,63 +443,35 @@ abstract class PrimaryRoleManager { List? get secondaryRoleManagers => _secondaryRoleManagers; List? _secondaryRoleManagers; - /// Factories for role managers that have not yet been instantiated. - Map? _secondaryRoleFactories; - /// Identifiers of secondary roles used by this primary role manager. /// /// This is only meant to be used in tests. @visibleForTesting List get debugSecondaryRoles => _secondaryRoleManagers?.map((RoleManager manager) => manager.role).toList() ?? const []; - /// Adds generic focus management features, if applicable. + /// Adds generic focus management features. void addFocusManagement() { - addSecondaryRole(Role.focusable, (SemanticsObject semanticsObject) { - if (semanticsObject.isFocusable) { - return Focusable(semanticsObject); - } - return null; - }); + addSecondaryRole(Focusable(semanticsObject)); } - /// Adds generic live region features, if applicable. + /// Adds generic live region features. void addLiveRegion() { - addSecondaryRole(Role.liveRegion, (SemanticsObject semanticsObject) { - if (semanticsObject.isLiveRegion) { - return LiveRegion(semanticsObject); - } - return null; - }); + addSecondaryRole(LiveRegion(semanticsObject)); } - /// Adds generic route name features, if applicable. + /// Adds generic route name features. void addRouteName() { - addSecondaryRole(Role.routeName, (SemanticsObject semanticsObject) { - if (semanticsObject.namesRoute) { - return RouteName(semanticsObject); - } - return null; - }); + addSecondaryRole(RouteName(semanticsObject)); } - /// Adds generic label features, if applicable. + /// Adds generic label features. void addLabelAndValue() { - addSecondaryRole(Role.labelAndValue, (SemanticsObject semanticsObject) { - if (semanticsObject.hasLabel || semanticsObject.hasValue || semanticsObject.hasTooltip) { - return LabelAndValue(semanticsObject); - } - return null; - }); + addSecondaryRole(LabelAndValue(semanticsObject)); } /// Adds generic functionality for handling taps and clicks. void addTappable() { - addSecondaryRole(Role.tappable, (SemanticsObject semanticsObject) { - if (semanticsObject.isTappable) { - return Tappable(semanticsObject); - } - return null; - }); + addSecondaryRole(Tappable(semanticsObject)); } /// Adds a secondary role to this primary role manager. @@ -515,16 +479,13 @@ abstract class PrimaryRoleManager { /// This method should be called by concrete implementations of /// [PrimaryRoleManager] during initialization. @protected - void addSecondaryRole(Role role, RoleManagerFactory roleManagerFactory) { - _secondaryRoleFactories ??= {}; - final Map secondaryRoleFactories = _secondaryRoleFactories!; - + void addSecondaryRole(RoleManager secondaryRoleManager) { assert( - !secondaryRoleFactories.containsKey(role), - 'Cannot add secondary role $role. This semantic node already has this role.', + _secondaryRoleManagers?.any((RoleManager manager) => manager.role == secondaryRoleManager.role) != true, + 'Cannot add secondary role ${secondaryRoleManager.role}. This object already has this secondary role.', ); - - secondaryRoleFactories[role] = roleManagerFactory; + _secondaryRoleManagers ??= []; + _secondaryRoleManagers!.add(secondaryRoleManager); } /// Called immediately after the fields of the [semanticsObject] are updated @@ -538,23 +499,6 @@ abstract class PrimaryRoleManager { /// the object. @mustCallSuper void update() { - final Map? secondaryRoleFactories = _secondaryRoleFactories; - if (secondaryRoleFactories != null && secondaryRoleFactories.isNotEmpty) { - secondaryRoleFactories.forEach((Role role, RoleManagerFactory roleFactory) { - final RoleManager? secondaryRoleManager = roleFactory(semanticsObject); - if (secondaryRoleManager != null) { - _secondaryRoleManagers ??= []; - _secondaryRoleManagers!.add(secondaryRoleManager); - } - }); - - // A role manager should only be added once. So remove any factories that - // contributed a role manager. - _secondaryRoleManagers?.forEach((RoleManager roleManager) { - secondaryRoleFactories.remove(roleManager.role); - }); - } - final List? secondaryRoles = _secondaryRoleManagers; if (secondaryRoles == null) { return; @@ -577,7 +521,7 @@ abstract class PrimaryRoleManager { /// gesture mode changes. @mustCallSuper void dispose() { - semanticsObject.clearAriaRole(); + semanticsObject.element.removeAttribute('role'); _isDisposed = true; } } @@ -1510,11 +1454,6 @@ class SemanticsObject { element.setAttribute('role', ariaRoleName); } - /// Removes the `role` HTML attribue, if any. - void clearAriaRole() { - element.removeAttribute('role'); - } - /// The primary role of this node. /// /// The primary role is assigned by [updateSelf] based on the combination of diff --git a/lib/web_ui/lib/src/engine/semantics/tappable.dart b/lib/web_ui/lib/src/engine/semantics/tappable.dart index 39362f9976a0a..259a9c7d55669 100644 --- a/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/lib/web_ui/lib/src/engine/semantics/tappable.dart @@ -40,8 +40,7 @@ class Tappable extends RoleManager { @override void update() { - final DomElement element = semanticsObject.element; - if (semanticsObject.enabledState() == EnabledState.disabled || !semanticsObject.isTappable) { + if (!semanticsObject.isTappable || semanticsObject.enabledState() == EnabledState.disabled) { _stopListening(); } else { if (_clickListener == null) { @@ -52,7 +51,7 @@ class Tappable extends RoleManager { EnginePlatformDispatcher.instance.invokeOnSemanticsAction( semanticsObject.id, ui.SemanticsAction.tap, null); }); - element.addEventListener('click', _clickListener); + semanticsObject.element.addEventListener('click', _clickListener); } } } diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 99590f66bfb5c..83a8f9817e178 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -95,93 +95,50 @@ void runSemanticsTests() { } void _testRoleManagerLifecycle() { - test('Secondary role managers are added upon node creation', () { + test('Secondary role managers are added upon node initialization', () { semantics() ..debugOverrideTimestampFunction(() => _testTime) ..semanticsEnabled = true; - final SemanticsTester tester = SemanticsTester(semantics()); - tester.updateNode( - id: 0, - isFocusable: true, - label: 'this is a label', - hasTap: true, - hasEnabledState: true, - isEnabled: true, - isButton: true, - rect: const ui.Rect.fromLTRB(0, 0, 100, 50), - ); - tester.apply(); - - expectSemanticsTree(''' - -'''); - - final SemanticsObject node = semantics().debugSemanticsTree![0]!; - expect(node.primaryRole?.role, PrimaryRole.button); - expect( - node.primaryRole?.debugSecondaryRoles, - containsAll([Role.focusable, Role.tappable, Role.labelAndValue]), - ); - expect(tester.getSemanticsObject(0).element.tabIndex, 0); - - semantics().semanticsEnabled = false; - }); - - test('Secondary role managers are added upon node update', () { - semantics() - ..debugOverrideTimestampFunction(() => _testTime) - ..semanticsEnabled = true; - - // Create without a label + // Check that roles are initialized immediately { final SemanticsTester tester = SemanticsTester(semantics()); tester.updateNode( id: 0, - isFocusable: true, - label: 'this is a label', - hasTap: true, - hasEnabledState: true, - isEnabled: true, isButton: true, rect: const ui.Rect.fromLTRB(0, 0, 100, 50), ); tester.apply(); - expectSemanticsTree(''' - -'''); + expectSemanticsTree(''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; expect(node.primaryRole?.role, PrimaryRole.button); expect( node.primaryRole?.debugSecondaryRoles, - containsAll([Role.focusable, Role.tappable]), + containsAll([Role.focusable, Role.tappable, Role.labelAndValue]), ); expect(tester.getSemanticsObject(0).element.tabIndex, 0); } - // Update with a label + // Check that roles apply their functionality upon update. { final SemanticsTester tester = SemanticsTester(semantics()); tester.updateNode( id: 0, - isFocusable: true, - hasTap: true, - hasEnabledState: true, - isEnabled: true, + label: 'a label', isButton: true, rect: const ui.Rect.fromLTRB(0, 0, 100, 50), ); tester.apply(); - expectSemanticsTree(''); + expectSemanticsTree(''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; expect(node.primaryRole?.role, PrimaryRole.button); expect( node.primaryRole?.debugSecondaryRoles, - containsAll([Role.focusable, Role.tappable]), + containsAll([Role.focusable, Role.tappable, Role.labelAndValue]), ); expect(tester.getSemanticsObject(0).element.tabIndex, 0); } From 0d0ecc6242c83e9efb2b2d1e70415d56abde1e53 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Fri, 14 Jul 2023 14:26:03 -0700 Subject: [PATCH 3/4] improve logic in focusable --- .../lib/src/engine/semantics/dialog.dart | 8 +++---- .../lib/src/engine/semantics/focusable.dart | 21 +++++++------------ 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index 673702cecb775..8155ba4838415 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -67,10 +67,6 @@ class RouteName extends RoleManager { @override void update() { - if (!semanticsObject.namesRoute) { - return; - } - // NOTE(yjbanov): this does not handle the case when the node structure // changes such that this RouteName is no longer attached to the same // dialog. While this is technically expressible using the semantics API, @@ -80,6 +76,10 @@ class RouteName extends RoleManager { // semantics code. Since reparenting can be done with no update to either // the Dialog or RouteName we'd have to scan intermediate nodes for // structural changes. + if (!semanticsObject.namesRoute) { + return; + } + if (semanticsObject.isLabelDirty) { final Dialog? dialog = _dialog; if (dialog != null) { diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 673f12e372443..5c6ffe9e9867a 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -38,21 +38,14 @@ class Focusable extends RoleManager { @override void update() { - if (!_focusManager.isManaging && !semanticsObject.isFocusable) { - // Nothing to do about this node. It's neither focusable, nor being managed. - return; - } - - if (!_focusManager.isManaging) { - // This line is only reachable iff the node is focusable but not being - // managed. So the focus manager is told to start managing it. - _focusManager.manage(semanticsObject.id, semanticsObject.element); + if (semanticsObject.isFocusable) { + if (!_focusManager.isManaging) { + _focusManager.manage(semanticsObject.id, semanticsObject.element); + } + _focusManager.changeFocus(semanticsObject.hasFocus && (!semanticsObject.hasEnabledState || semanticsObject.isEnabled)); + } else { + _focusManager.stopManaging(); } - _updateFocus(); - } - - void _updateFocus() { - _focusManager.changeFocus(semanticsObject.hasFocus && (!semanticsObject.hasEnabledState || semanticsObject.isEnabled)); } @override From 99d3e3523885f67862b25d37ea24ec113a50fcf2 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Fri, 14 Jul 2023 15:59:31 -0700 Subject: [PATCH 4/4] blur element when not managing; initialize in update() --- .../lib/src/engine/semantics/focusable.dart | 11 ++++-- .../test/engine/semantics/semantics_test.dart | 39 ++++++++++++++++++- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 5c6ffe9e9867a..4b3285dbdf399 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -30,9 +30,7 @@ import 'semantics.dart'; class Focusable extends RoleManager { Focusable(SemanticsObject semanticsObject) : _focusManager = AccessibilityFocusManager(semanticsObject.owner), - super(Role.focusable, semanticsObject) { - _focusManager.manage(semanticsObject.id, semanticsObject.element); - } + super(Role.focusable, semanticsObject); final AccessibilityFocusManager _focusManager; @@ -137,6 +135,7 @@ class AccessibilityFocusManager { /// Stops managing the focus of the current element, if any. void stopManaging() { final _FocusTarget? target = _target; + _target = null; if (target == null) { /// Nothing is being managed. Just return. @@ -145,7 +144,11 @@ class AccessibilityFocusManager { target.element.removeEventListener('focus', target.domFocusListener); target.element.removeEventListener('blur', target.domBlurListener); - _target = null; + + // 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) { diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 83a8f9817e178..4c4f51eb0a49b 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -118,7 +118,7 @@ void _testRoleManagerLifecycle() { node.primaryRole?.debugSecondaryRoles, containsAll([Role.focusable, Role.tappable, Role.labelAndValue]), ); - expect(tester.getSemanticsObject(0).element.tabIndex, 0); + expect(tester.getSemanticsObject(0).element.tabIndex, -1); } // Check that roles apply their functionality upon update. @@ -127,6 +127,7 @@ void _testRoleManagerLifecycle() { tester.updateNode( id: 0, label: 'a label', + isFocusable: true, isButton: true, rect: const ui.Rect.fromLTRB(0, 0, 100, 50), ); @@ -2737,28 +2738,62 @@ void _testFocusable() { expect(element.tabIndex, -1); domDocument.body!.append(element); + // Start managing element manager.manage(1, element); expect(element.tabIndex, 0); expect(capturedActions, isEmpty); + expect(domDocument.activeElement, isNot(element)); + // Request focus manager.changeFocus(true); pumpSemantics(); // triggers post-update callbacks + expect(domDocument.activeElement, element); expect(capturedActions, [ (1, ui.SemanticsAction.didGainAccessibilityFocus, null), ]); capturedActions.clear(); + // Give up focus manager.changeFocus(false); pumpSemantics(); // triggers post-update callbacks expect(capturedActions, [ (1, ui.SemanticsAction.didLoseAccessibilityFocus, null), ]); capturedActions.clear(); + expect(domDocument.activeElement, isNot(element)); + + // Request focus again + manager.changeFocus(true); + pumpSemantics(); // triggers post-update callbacks + expect(domDocument.activeElement, element); + expect(capturedActions, [ + (1, ui.SemanticsAction.didGainAccessibilityFocus, null), + ]); + 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.', + capturedActions, + isEmpty, + ); + expect(domDocument.activeElement, isNot(element)); + + // Attempt to request focus when not managing an element. manager.changeFocus(true); pumpSemantics(); // triggers post-update callbacks - expect(capturedActions, isEmpty); + expect( + reason: 'Attempting to request focus on a node that is not managed should ' + 'not result in any notifications to the framework.', + capturedActions, + isEmpty, + ); + expect(domDocument.activeElement, isNot(element)); semantics().semanticsEnabled = false; });