[web] always add secondary role managers

lib/web_ui/lib/src/engine/semantics/dialog.dart

@@ -76,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) {

lib/web_ui/lib/src/engine/semantics/focusable.dart

@@ -30,15 +30,20 @@ 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;
 
   @override
   void update() {
-    _focusManager.changeFocus(semanticsObject.hasFocus && (!semanticsObject.hasEnabledState || semanticsObject.isEnabled));
+    if (semanticsObject.isFocusable) {
+      if (!_focusManager.isManaging) {
+        _focusManager.manage(semanticsObject.id, semanticsObject.element);
+      }
+      _focusManager.changeFocus(semanticsObject.hasFocus && (!semanticsObject.hasEnabledState || semanticsObject.isEnabled));
+    } else {
+      _focusManager.stopManaging();
+    }
   }
 
   @override
@@ -79,6 +84,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
@@ -127,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.
@@ -135,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) {

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

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;

lib/web_ui/lib/src/engine/semantics/semantics.dart

@@ -449,39 +449,29 @@ abstract class PrimaryRoleManager {
   @visibleForTesting
   List<Role> get debugSecondaryRoles => _secondaryRoleManagers?.map((RoleManager manager) => manager.role).toList() ?? const <Role>[];
 
-  /// Adds generic focus management features, if applicable.
+  /// Adds generic focus management features.
   void addFocusManagement() {
-    if (semanticsObject.isFocusable) {
-      addSecondaryRole(Focusable(semanticsObject));
-    }
+    addSecondaryRole(Focusable(semanticsObject));
   }
 
-  /// Adds generic live region features, if applicable.
+  /// Adds generic live region features.
   void addLiveRegion() {
-    if (semanticsObject.isLiveRegion) {
-      addSecondaryRole(LiveRegion(semanticsObject));
-    }
+    addSecondaryRole(LiveRegion(semanticsObject));
   }
 
-  /// Adds generic route name features, if applicable.
+  /// Adds generic route name features.
   void addRouteName() {
-    if (semanticsObject.namesRoute) {
-      addSecondaryRole(RouteName(semanticsObject));
-    }
+    addSecondaryRole(RouteName(semanticsObject));
   }
 
-  /// Adds generic label features, if applicable.
+  /// Adds generic label features.
   void addLabelAndValue() {
-    if (semanticsObject.hasLabel || semanticsObject.hasValue || semanticsObject.hasTooltip) {
-      addSecondaryRole(LabelAndValue(semanticsObject));
-    }
+    addSecondaryRole(LabelAndValue(semanticsObject));
   }
 
   /// Adds generic functionality for handling taps and clicks.
   void addTappable() {
-    if (semanticsObject.isTappable) {
-      addSecondaryRole(Tappable(semanticsObject));
-    }
+    addSecondaryRole(Tappable(semanticsObject));
   }
 
   /// Adds a secondary role to this primary role manager.
@@ -531,7 +521,7 @@ abstract class PrimaryRoleManager {
   /// gesture mode changes.
   @mustCallSuper
   void dispose() {
-    semanticsObject.clearAriaRole();
+    semanticsObject.element.removeAttribute('role');
     _isDisposed = true;
   }
 }
@@ -1464,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

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);
       }
     }
   }

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,60 @@ void runSemanticsTests() {
   });
 }
 
+void _testRoleManagerLifecycle() {
+  test('Secondary role managers are added upon node initialization', () {
+    semantics()
+      ..debugOverrideTimestampFunction(() => _testTime)
+      ..semanticsEnabled = true;
+
+    // Check that roles are initialized immediately
+    {
+      final SemanticsTester tester = SemanticsTester(semantics());
+      tester.updateNode(
+        id: 0,
+        isButton: true,
+        rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
+      );
+      tester.apply();
+
+      expectSemanticsTree('<sem role="button" style="$rootSemanticStyle"></sem>');
+
+      final SemanticsObject node = semantics().debugSemanticsTree![0]!;
+      expect(node.primaryRole?.role, PrimaryRole.button);
+      expect(
+        node.primaryRole?.debugSecondaryRoles,
+        containsAll(<Role>[Role.focusable, Role.tappable, Role.labelAndValue]),
+      );
+      expect(tester.getSemanticsObject(0).element.tabIndex, -1);
+    }
+
+    // Check that roles apply their functionality upon update.
+    {
+      final SemanticsTester tester = SemanticsTester(semantics());
+      tester.updateNode(
+        id: 0,
+        label: 'a label',
+        isFocusable: true,
+        isButton: true,
+        rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
+      );
+      tester.apply();
+
+      expectSemanticsTree('<sem aria-label="a label" role="button" style="$rootSemanticStyle"></sem>');
+
+      final SemanticsObject node = semantics().debugSemanticsTree![0]!;
+      expect(node.primaryRole?.role, PrimaryRole.button);
+      expect(
+        node.primaryRole?.debugSecondaryRoles,
+        containsAll(<Role>[Role.focusable, Role.tappable, Role.labelAndValue]),
+      );
+      expect(tester.getSemanticsObject(0).element.tabIndex, 0);
+    }
+
+    semantics().semanticsEnabled = false;
+  });
+}
+
 void _testEngineAccessibilityBuilder() {
   final EngineAccessibilityFeaturesBuilder builder =
       EngineAccessibilityFeaturesBuilder(0);
@@ -2681,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, <CapturedAction>[
       (1, ui.SemanticsAction.didGainAccessibilityFocus, null),
     ]);
     capturedActions.clear();
 
+    // Give up focus
     manager.changeFocus(false);
     pumpSemantics(); // triggers post-update callbacks
     expect(capturedActions, <CapturedAction>[
       (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, <CapturedAction>[
+      (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;
   });