Reland "Fix a11y placeholder on desktop; auto-enable engine semantics" (#26134)

This reverts commit 78f5dde9.

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

@@ -1480,7 +1480,18 @@ class EngineSemanticsOwner {
   /// Updates the semantics tree from data in the [uiUpdate].
   void updateSemantics(ui.SemanticsUpdate uiUpdate) {
     if (!_semanticsEnabled) {
-      return;
+      if (ui.debugEmulateFlutterTesterEnvironment) {
+        // Running Flutter widget tests in a fake environment. Don't enable
+        // engine semantics. Test semantics trees violate invariants in ways
+        // production implementation isn't built to handle. For example, tests
+        // routinely reset semantics node IDs, which is messing up the update
+        // process.
+        return;
+      } else {
+        // Running a real app. Auto-enable engine semantics.
+        semanticsHelper.dispose(); // placeholder no longer needed
+        semanticsEnabled = true;
+      }
     }
 
     final SemanticsUpdate update = uiUpdate as SemanticsUpdate;

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

@@ -51,6 +51,15 @@ class SemanticsHelper {
   html.Element prepareAccessibilityPlaceholder() {
     return _semanticsEnabler.prepareAccessibilityPlaceholder();
   }
+
+  /// Stops waiting for the user to enable semantics and removes the
+  /// placeholder.
+  ///
+  /// This is used when semantics is enabled programmatically and therefore the
+  /// placehodler is no longer needed.
+  void dispose() {
+    _semanticsEnabler.dispose();
+  }
 }
 
 @visibleForTesting
@@ -92,44 +101,34 @@ abstract class SemanticsEnabler {
   ///
   /// If not they are sent to framework as normal events.
   bool get isWaitingToEnableSemantics;
+
+  /// Stops waiting for the user to enable semantics and removes the placeholder.
+  void dispose();
 }
 
+/// The desktop semantics enabler uses a simpler strategy compared to mobile.
+///
+/// A placeholder element is created completely outside the view and is not
+/// reachable via touch or mouse. Assistive technology can still find it either
+/// using keyboard shortcuts or via next/previous touch gesture (for touch
+/// screens). This simplification removes the need for pointer event
+/// disambiguation or timers. The placeholder simply waits for a click event
+/// and enables semantics.
 @visibleForTesting
 class DesktopSemanticsEnabler extends SemanticsEnabler {
-  /// We do not immediately enable semantics when the user requests it, but
-  /// instead wait for a short period of time before doing it. This is because
-  /// the request comes as an event targeted on the [_semanticsPlaceholder].
-  /// This event, depending on the browser, comes as a burst of events.
-  /// For example, Safari on MacOS sends "pointerup", "pointerdown". So during a
-  /// short time period we consume all events and prevent forwarding to the
-  /// framework. Otherwise, the events will be interpreted twice, once as a
-  /// request to activate semantics, and a second time by Flutter's gesture
-  /// recognizers.
-  @visibleForTesting
-  Timer? semanticsActivationTimer;
-
   /// A temporary placeholder used to capture a request to activate semantics.
   html.Element? _semanticsPlaceholder;
 
-  /// The number of events we processed that could potentially activate
-  /// semantics.
-  int semanticsActivationAttempts = 0;
-
-  /// Instructs [_tryEnableSemantics] to remove [_semanticsPlaceholder].
-  ///
-  /// The placeholder is removed upon any next event.
-  bool _schedulePlaceholderRemoval = false;
-
   /// Whether we are waiting for the user to enable semantics.
   @override
   bool get isWaitingToEnableSemantics => _semanticsPlaceholder != null;
 
   @override
   bool tryEnableSemantics(html.Event event) {
-    if (_schedulePlaceholderRemoval) {
-      _semanticsPlaceholder!.remove();
-      _semanticsPlaceholder = null;
-      semanticsActivationTimer = null;
+    // Semantics may be enabled programmatically. If there's a race between that
+    // and the DOM event, we may end up here while there's no longer a placeholder
+    // to work with.
+    if (!isWaitingToEnableSemantics) {
       return true;
     }
 
@@ -154,37 +153,17 @@ class DesktopSemanticsEnabler extends SemanticsEnabler {
       return true;
     }
 
-    semanticsActivationAttempts += 1;
-    if (semanticsActivationAttempts >= kMaxSemanticsActivationAttempts) {
-      // We have received multiple user events, none of which resulted in
-      // semantics activation. This is a signal that the user is not interested
-      // in semantics, and so we will stop waiting for it.
-      _schedulePlaceholderRemoval = true;
-      return true;
-    }
-
-    if (semanticsActivationTimer != null) {
-      // We are in a waiting period to activate a timer. While the timer is
-      // active we should consume events pertaining to semantics activation.
-      // Otherwise the event will also be interpreted by the framework and
-      // potentially result in activating a gesture in the app.
-      return false;
-    }
-
     // Check for the event target.
     final bool enableConditionPassed = (event.target == _semanticsPlaceholder);
 
-    if (enableConditionPassed) {
-      assert(semanticsActivationTimer == null);
-      semanticsActivationTimer = Timer(_periodToConsumeEvents, () {
-        EngineSemanticsOwner.instance.semanticsEnabled = true;
-        _schedulePlaceholderRemoval = true;
-      });
-      return false;
+    if (!enableConditionPassed) {
+      // This was not a semantics activating event; forward as normal.
+      return true;
     }
 
-    // This was not a semantics activating event; forward as normal.
-    return true;
+    EngineSemanticsOwner.instance.semanticsEnabled = true;
+    dispose();
+    return false;
   }
 
   @override
@@ -199,7 +178,7 @@ class DesktopSemanticsEnabler extends SemanticsEnabler {
 
     // Adding roles to semantics placeholder. 'aria-live' will make sure that
     // the content is announced to the assistive technology user as soon as the
-    // page receives focus. 'tab-index' makes sure the button is the first
+    // page receives focus. 'tabindex' makes sure the button is the first
     // target of tab. 'aria-label' is used to define the placeholder message
     // to the assistive technology user.
     placeholder
@@ -207,6 +186,8 @@ class DesktopSemanticsEnabler extends SemanticsEnabler {
       ..setAttribute('aria-live', 'true')
       ..setAttribute('tabindex', '0')
       ..setAttribute('aria-label', placeholderMessage);
+
+    // The placeholder sits just outside the window so only AT can reach it.
     placeholder.style
       ..position = 'absolute'
       ..left = '-1px'
@@ -215,6 +196,12 @@ class DesktopSemanticsEnabler extends SemanticsEnabler {
       ..height = '1px';
     return placeholder;
   }
+
+  @override
+  void dispose() {
+    _semanticsPlaceholder?.remove();
+    _semanticsPlaceholder = null;
+  }
 }
 
 @visibleForTesting
@@ -254,6 +241,13 @@ class MobileSemanticsEnabler extends SemanticsEnabler {
 
   @override
   bool tryEnableSemantics(html.Event event) {
+    // Semantics may be enabled programmatically. If there's a race between that
+    // and the DOM event, we may end up here while there's no longer a placeholder
+    // to work with.
+    if (!isWaitingToEnableSemantics) {
+      return true;
+    }
+
     if (_schedulePlaceholderRemoval) {
       // The event type can also be click for VoiceOver.
       final bool removeNow = (browserEngine != BrowserEngine.webkit ||
@@ -261,9 +255,7 @@ class MobileSemanticsEnabler extends SemanticsEnabler {
           event.type == 'pointerup' ||
           event.type == 'click');
       if (removeNow) {
-        _semanticsPlaceholder!.remove();
-        _semanticsPlaceholder = null;
-        semanticsActivationTimer = null;
+        dispose();
       }
       return true;
     }
@@ -403,4 +395,11 @@ class MobileSemanticsEnabler extends SemanticsEnabler {
 
     return placeholder;
   }
+
+  @override
+  void dispose() {
+    _semanticsPlaceholder?.remove();
+    _semanticsPlaceholder = null;
+    semanticsActivationTimer = null;
+  }
 }

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

@@ -28,10 +28,6 @@ void testMain() {
       if (_placeholder != null) {
         _placeholder.remove();
       }
-      if (desktopSemanticsEnabler?.semanticsActivationTimer != null) {
-        desktopSemanticsEnabler.semanticsActivationTimer.cancel();
-        desktopSemanticsEnabler.semanticsActivationTimer = null;
-      }
     });
 
     test('prepare accesibility placeholder', () async {
@@ -73,9 +69,7 @@ void testMain() {
 
         expect(shouldForwardToFramework, true);
       }
-    },
-        // TODO(nurhan): https://github.com/flutter/flutter/issues/50754
-        skip: browserEngine == BrowserEngine.edge);
+    });
 
     test(
         'Relevants events targeting placeholder should not be forwarded to the framework',
@@ -93,31 +87,13 @@ void testMain() {
       expect(shouldForwardToFramework, false);
     });
 
-    test(
-        'After max number of relevant events, events should be forwarded to the framework',
-        () async {
-      // Prework. Attach the placeholder to dom.
+    test('disposes of the placeholder', () {
       _placeholder = desktopSemanticsEnabler.prepareAccessibilityPlaceholder();
       html.document.body.append(_placeholder);
 
-      html.Event event = html.MouseEvent('mousedown');
-      _placeholder.dispatchEvent(event);
-
-      bool shouldForwardToFramework =
-          desktopSemanticsEnabler.tryEnableSemantics(event);
-
-      expect(shouldForwardToFramework, false);
-
-      // Send max number of events;
-      for (int i = 1; i <= kMaxSemanticsActivationAttempts; i++) {
-        event = html.MouseEvent('mousedown');
-        _placeholder.dispatchEvent(event);
-
-        shouldForwardToFramework =
-            desktopSemanticsEnabler.tryEnableSemantics(event);
-      }
-
-      expect(shouldForwardToFramework, true);
+      expect(_placeholder.isConnected, isTrue);
+      desktopSemanticsEnabler.dispose();
+      expect(_placeholder.isConnected, isFalse);
     });
   });
 
@@ -168,7 +144,5 @@ void testMain() {
       expect(shouldForwardToFramework, true);
     });
   },  // Run the `MobileSemanticsEnabler` only on mobile browsers.
-      skip: operatingSystem == OperatingSystem.linux ||
-          operatingSystem == OperatingSystem.macOs ||
-          operatingSystem == OperatingSystem.windows);
+      skip: isDesktop);
 }

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

@@ -83,23 +83,60 @@ void _testEngineSemanticsOwner() {
     expect(semantics().mode, AccessibilityMode.unknown);
   });
 
-  test('auto-enables semantics', () async {
+  test('placeholder enables semantics', () async {
     domRenderer.reset(); // triggers `autoEnableOnTap` to be called
     expect(semantics().semanticsEnabled, false);
 
     // Synthesize a click on the placeholder.
     final html.Element placeholder =
         html.document.querySelectorAll('flt-semantics-placeholder').single;
+    expect(placeholder.isConnected, true);
     final html.Rectangle<num> rect = placeholder.getBoundingClientRect();
     placeholder.dispatchEvent(html.MouseEvent(
       'click',
       clientX: (rect.left + (rect.right - rect.left) / 2).floor(),
       clientY: (rect.top + (rect.bottom - rect.top) / 2).floor(),
     ));
-    while (!semantics().semanticsEnabled) {
-      await Future<void>.delayed(const Duration(milliseconds: 50));
+
+    // On mobile semantics is not enabled synchronously. This is because the
+    // placeholder receives pointer events in non-accessibility mode too, and
+    // therefore we wait to see if any subsequent pointer events are issued
+    // indicating that this is not a request to enable accessibility.
+    if (isMobile) {
+      while (!semantics().semanticsEnabled) {
+        await Future<void>.delayed(const Duration(milliseconds: 50));
+      }
+    }
+    expect(semantics().semanticsEnabled, true);
+
+    // The placeholder should be removed
+    if (isMobile) {
+      // On mobile the placeholder is not removed synchronously. Instead it is
+      // removed upon the next DOM event. Otherwise Safari swallows pointerup.
+      expect(placeholder.isConnected, true);
+      placeholder.click();
+      await Future<void>.delayed(Duration.zero);
     }
+    expect(placeholder.isConnected, false);
+  });
+
+  test('auto-enables semantics', () async {
+    domRenderer.reset(); // triggers `autoEnableOnTap` to be called
+    expect(semantics().semanticsEnabled, false);
+
+    final html.Element placeholder =
+        html.document.querySelectorAll('flt-semantics-placeholder').single;
+    expect(placeholder.isConnected, true);
+
+    // Sending a semantics update should auto-enable engine semantics.
+    final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder();
+    updateNode(builder, id: 0);
+    semantics().updateSemantics(builder.build());
+
     expect(semantics().semanticsEnabled, true);
+
+    // The placeholder should be removed
+    expect(placeholder.isConnected, false);
   });
 
   void renderLabel(String label) {