address comments

Author: yjbanov

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

@@ -605,6 +605,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();
 }
 
@@ -2468,7 +2472,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

lib/web_ui/test/engine/semantics/semantics_test.dart

@@ -1537,7 +1537,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);
@@ -1905,7 +1905,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);
@@ -1914,9 +1914,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, <CapturedAction>[
       (0, ui.SemanticsAction.didLoseAccessibilityFocus, null),
@@ -2081,7 +2089,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);
@@ -2874,7 +2882,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()
@@ -2886,14 +2894,16 @@ 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,
       transform: Matrix4.identity().toFloat64(),
       children: <SemanticsNodeUpdate>[
         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: <SemanticsNodeUpdate>[
             tester.updateNode(
               id: 2,
@@ -2934,7 +2944,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 {
@@ -2947,7 +2957,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,
@@ -2975,6 +2985,7 @@ void _testDialog() {
               isEnabled: true,
               isButton: true,
               isFocusable: true,
+              // Asked for focus explicitly.
               isFocused: true,
               rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
             ),
@@ -2995,8 +3006,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)
@@ -3007,7 +3018,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,
@@ -3043,7 +3054,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);
 
@@ -3062,17 +3073,16 @@ 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,
       transform: Matrix4.identity().toFloat64(),
     );
     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;
   });