Doc updates for Semantics; always reset SemanticsNode (#11770)

* refactor to ensureAction + some related doc fixes

* Update docs for markNeedsSemanticsUpdate

* rewording

* rewording

* ensureAction test

* ensureAction test

* ensureAction test

* more tests

* refactor to allways reset node

* tiny fixes

* more test

* doc fixes

* one more test

* review comments

packages/flutter/lib/src/cupertino/switch.dart

@@ -210,7 +210,7 @@ class _RenderCupertinoSwitch extends RenderConstrainedBox implements SemanticsAc
     if (value == _value)
       return;
     _value = value;
-    markNeedsSemanticsUpdate(onlyChanges: true, noGeometry: true);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: true, noGeometry: true);
     _position
       ..curve = Curves.ease
       ..reverseCurve = Curves.ease.flipped;

packages/flutter/lib/src/material/toggleable.dart

@@ -122,7 +122,7 @@ abstract class RenderToggleable extends RenderConstrainedBox implements Semantic
     if (value == _value)
       return;
     _value = value;
-    markNeedsSemanticsUpdate(onlyChanges: true, noGeometry: true);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: true, noGeometry: true);
     _position
       ..curve = Curves.easeIn
       ..reverseCurve = Curves.easeOut;

packages/flutter/lib/src/rendering/object.dart

@@ -2551,36 +2551,44 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
     _semantics?.reset();
   }
 
-  /// Mark this node as needing an update to its semantics
-  /// description.
-  ///
-  /// If the change did not involve a removal or addition of semantics, only the
-  /// change of semantics (e.g. isChecked changing from true to false, as
-  /// opposed to isChecked changing from being true to not being changed at
-  /// all), then you can pass the onlyChanges argument with the value true to
-  /// reduce the cost. If semantics are being added or removed, more work needs
-  /// to be done to update the semantics tree. If you pass 'onlyChanges: true'
-  /// but this node, which previously had a SemanticsNode, no longer has one, or
-  /// previously did not set any semantics, but now does, or previously had a
-  /// child that returned annotators, but no longer does, or other such
-  /// combinations, then you will either assert during the subsequent call to
-  /// [PipelineOwner.flushSemantics()] or you will have out-of-date information
-  /// in the semantics tree.
-  ///
-  /// If the geometry might have changed in any way, then again, more work needs
-  /// to be done to update the semantics tree (to deal with clips). You can pass
-  /// the noGeometry argument to avoid this work in the case where only the
-  /// labels or flags changed. If you pass 'noGeometry: true' when the geometry
-  /// did change, the semantic tree will be out of date.
-  void markNeedsSemanticsUpdate({ bool onlyChanges: false, bool noGeometry: false }) {
+  /// Mark this node as needing an update to its semantics description.
+  ///
+  /// The parameters [onlyLocalUpdates] and [noGeometry] tell the framework
+  /// how much of the semantics have changed. Bigger changes (indicated by
+  /// setting one or both parameters to `false`) are more expansive to compute.
+  ///
+  /// [onlyLocalUpdates] should be set to `true` to reduce cost if the semantics
+  /// update does not in any way change the shape of the semantics tree (e.g.
+  /// [SemanticsNode]s will neither be added/removed from the tree nor be moved
+  /// within the tree). In other words, with [onlyLocalChanges] the
+  /// [RenderObject] can indicate that it only wants to perform updates on the
+  /// local [SemanticsNode] (e.g. changing a label or flag) without affecting
+  /// other nodes in the tree.
+  ///
+  /// [onlyLocalUpdates] has to be set to `false` in the following cases as they
+  /// will change the shape of the tree:
+  ///
+  /// 1. [isSemanticBoundary] changed its value.
+  /// 2. [semanticsAnnotator] changed from or to returning `null` and
+  ///    [isSemanticBoundary] isn't `true`.
+  ///
+  /// [noGeometry] should be set to `true` to reduce cost if the geometry (e.g.
+  /// size and position) of the corresponding [SemanticsNode] has not
+  /// changed. Examples for such semantic updates that don't require a geometry
+  /// update are changes to flags, labels, or actions.
+  ///
+  /// If [onlyLocalUpdates] or [noGeometry] are incorrectly set to true, asserts
+  /// might throw or the computed semantics tree might be out-of-date without
+  /// warning.
+  void markNeedsSemanticsUpdate({ bool onlyLocalUpdates: false, bool noGeometry: false }) {
     assert(!attached || !owner._debugDoingSemantics);
-    if ((attached && owner._semanticsOwner == null) || (_needsSemanticsUpdate && onlyChanges && (_needsSemanticsGeometryUpdate || noGeometry)))
+    if ((attached && owner._semanticsOwner == null) || (_needsSemanticsUpdate && onlyLocalUpdates && (_needsSemanticsGeometryUpdate || noGeometry)))
       return;
     if (!noGeometry && (_semantics == null || (_semantics.hasChildren && _semantics.wasAffectedByClip))) {
       // Since the geometry might have changed, we need to make sure to reapply any clips.
       _needsSemanticsGeometryUpdate = true;
     }
-    if (onlyChanges) {
+    if (onlyLocalUpdates) {
       // The shape of the tree didn't change, but the details did.
       // If we have our own SemanticsNode (our _semantics isn't null)
       // then mark ourselves dirty. If we don't then we are using an
@@ -2590,9 +2598,11 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
         if (node._needsSemanticsUpdate)
           return;
         node._needsSemanticsUpdate = true;
+        node.resetSemantics();
         node = node.parent;
       }
       if (!node._needsSemanticsUpdate) {
+        node.resetSemantics();
         node._needsSemanticsUpdate = true;
         if (owner != null) {
           owner._nodesNeedingSemantics.add(node);
@@ -2739,15 +2749,14 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
   /// this object. The function returned by this function will be used to
   /// annotate the [SemanticsNode] for this object.
   ///
-  /// Semantic annotations are persistent. Values set in one pass will still be
-  /// set in the next pass. Therefore it is important to explicitly set fields
-  /// to false once they are no longer true; setting them to true when they are
-  /// to be enabled, and not setting them at all when they are not, will mean
-  /// they remain set once enabled once and will never get unset.
+  /// Semantic annotations are not persisted between subsequent calls to an
+  /// annotator. The [SemanticsAnnotator] should always set all options
+  /// (e.g. flags, labels, actions, etc.) to the values it cares about given
+  /// the current state of the [RenderObject].
   ///
   /// If the return value will change from null to non-null (or vice versa), and
   /// [isSemanticBoundary] isn't true, then the associated call to
-  /// [markNeedsSemanticsUpdate] must not have `onlyChanges` set, as it is
+  /// [markNeedsSemanticsUpdate] must not have `onlyLocalUpdates` set, as it is
   /// possible that the node should be entirely removed.
   ///
   /// If the annotation should only happen under certain conditions, `null`

packages/flutter/lib/src/rendering/proxy_box.dart

@@ -992,7 +992,7 @@ abstract class _RenderCustomClip<T> extends RenderProxyBox {
   void _markNeedsClip() {
     _clip = null;
     markNeedsPaint();
-    markNeedsSemanticsUpdate(onlyChanges: true);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: true);
   }
 
   T get _defaultClip;
@@ -2823,7 +2823,7 @@ class RenderSemanticsGestureHandler extends RenderProxyBox implements SemanticsA
     if (setEquals<SemanticsAction>(value, _validActions))
       return;
     _validActions = value;
-    markNeedsSemanticsUpdate(onlyChanges: true);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: true);
   }
 
    /// Called when the user taps on the render object.
@@ -2836,7 +2836,7 @@ class RenderSemanticsGestureHandler extends RenderProxyBox implements SemanticsA
     final bool hadHandler = _onTap != null;
     _onTap = value;
     if ((value != null) != hadHandler)
-      markNeedsSemanticsUpdate(onlyChanges: isSemanticBoundary == wasSemanticBoundary);
+      markNeedsSemanticsUpdate(onlyLocalUpdates: isSemanticBoundary == wasSemanticBoundary);
   }
 
   /// Called when the user presses on the render object for a long period of time.
@@ -2849,7 +2849,7 @@ class RenderSemanticsGestureHandler extends RenderProxyBox implements SemanticsA
     final bool hadHandler = _onLongPress != null;
     _onLongPress = value;
     if ((value != null) != hadHandler)
-      markNeedsSemanticsUpdate(onlyChanges: isSemanticBoundary == wasSemanticBoundary);
+      markNeedsSemanticsUpdate(onlyLocalUpdates: isSemanticBoundary == wasSemanticBoundary);
   }
 
   /// Called when the user scrolls to the left or to the right.
@@ -2862,7 +2862,7 @@ class RenderSemanticsGestureHandler extends RenderProxyBox implements SemanticsA
     final bool hadHandler = _onHorizontalDragUpdate != null;
     _onHorizontalDragUpdate = value;
     if ((value != null) != hadHandler)
-      markNeedsSemanticsUpdate(onlyChanges: isSemanticBoundary == wasSemanticBoundary);
+      markNeedsSemanticsUpdate(onlyLocalUpdates: isSemanticBoundary == wasSemanticBoundary);
   }
 
   /// Called when the user scrolls up or down.
@@ -2875,7 +2875,7 @@ class RenderSemanticsGestureHandler extends RenderProxyBox implements SemanticsA
     final bool hadHandler = _onVerticalDragUpdate != null;
     _onVerticalDragUpdate = value;
     if ((value != null) != hadHandler)
-      markNeedsSemanticsUpdate(onlyChanges: isSemanticBoundary == wasSemanticBoundary);
+      markNeedsSemanticsUpdate(onlyLocalUpdates: isSemanticBoundary == wasSemanticBoundary);
   }
 
   /// The fraction of the dimension of this render box to use when
@@ -3074,7 +3074,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
       return;
     final bool hadValue = checked != null;
     _checked = value;
-    markNeedsSemanticsUpdate(onlyChanges: (value != null) == hadValue);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
   }
 
   /// If non-null, sets the [SemanticsNode.isSelected] semantic to the given
@@ -3086,7 +3086,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
       return;
     final bool hadValue = selected != null;
     _selected = value;
-    markNeedsSemanticsUpdate(onlyChanges: (value != null) == hadValue);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
   }
 
   /// If non-null, sets the [SemanticsNode.label] semantic to the given value.
@@ -3097,7 +3097,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
       return;
     final bool hadValue = label != null;
     _label = value;
-    markNeedsSemanticsUpdate(onlyChanges: (value != null) == hadValue);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
   }
 
   @override

packages/flutter/lib/src/rendering/semantics.dart

@@ -258,6 +258,7 @@ class SemanticsNode extends AbstractNode {
   /// [SemanticsActionHandler.performAction] will be called with the chosen
   /// action.
   void addAction(SemanticsAction action) {
+    assert(action != null);
     final int index = action.index;
     if ((_actions & index) == 0) {
       _actions |= index;
@@ -352,11 +353,7 @@ class SemanticsNode extends AbstractNode {
 
   Set<SemanticsTag> _tags = new Set<SemanticsTag>();
 
-  /// Ensures that the [SemanticsNode] is or is not tagged with [tag].
-  ///
-  /// If [isPresent] is `true` it will ensure that the tag is present. If
-  /// [isPresent] is `false` it will ensure that the node is not tagged with
-  /// [tag].
+  /// Tags the [SemanticsNode] with [tag].
   ///
   /// Tags are not sent to the engine. They can be used by a parent
   /// [SemanticsNode] to figure out how to add the node as a child.
@@ -365,11 +362,9 @@ class SemanticsNode extends AbstractNode {
   ///
   ///  * [SemanticsTag], whose documentation discusses the purposes of tags.
   ///  * [hasTag] to check if the node has a certain tag.
-  void ensureTag(SemanticsTag tag, { bool isPresent: true }) {
-    if (isPresent)
-      _tags.add(tag);
-    else
-      _tags.remove(tag);
+  void addTag(SemanticsTag tag) {
+    assert(tag != null);
+    _tags.add(tag);
   }
 
   /// Check if the [SemanticsNode] is tagged with [tag].

packages/flutter/lib/src/rendering/sliver_persistent_header.dart

@@ -224,7 +224,7 @@ abstract class RenderSliverPersistentHeader extends RenderSliver with RenderObje
   SemanticsAnnotator get semanticsAnnotator => _excludeFromSemanticsScrolling ? _annotate : null;
 
   void _annotate(SemanticsNode node) {
-    node.ensureTag(RenderSemanticsGestureHandler.excludeFromScrolling);
+    node.addTag(RenderSemanticsGestureHandler.excludeFromScrolling);
   }
 
   @override

packages/flutter/lib/src/rendering/viewport.dart

@@ -91,7 +91,7 @@ abstract class RenderViewportBase<ParentDataClass extends ContainerParentDataMix
   SemanticsAnnotator get semanticsAnnotator => _annotate;
 
   void _annotate(SemanticsNode node) {
-    node.ensureTag(RenderSemanticsGestureHandler.useTwoPaneSemantics);
+    node.addTag(RenderSemanticsGestureHandler.useTwoPaneSemantics);
   }
 
   /// The direction in which the [SliverConstraints.scrollOffset] increases.

packages/flutter/test/rendering/object_test.dart

@@ -33,7 +33,7 @@ void main() {
     owner.flushSemantics();
 
     expect(onNeedVisualUpdateCallCount, 1);
-    renderObject.markNeedsSemanticsUpdate(onlyChanges: true);
+    renderObject.markNeedsSemanticsUpdate(onlyLocalUpdates: true);
     expect(onNeedVisualUpdateCallCount, 2);
   });
 }

packages/flutter/test/rendering/proxy_box_test.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'package:flutter/foundation.dart';
+import 'package:flutter/gestures.dart';
 import 'package:flutter/rendering.dart';
 import 'package:test/test.dart';
 
@@ -69,4 +70,25 @@ void main() {
 
     debugDefaultTargetPlatformOverride = null;
   });
+
+  test('RenderSemanticsGestureHandler adds/removes correct semantic actions', () {
+    SemanticsNode node = new SemanticsNode();
+    final RenderSemanticsGestureHandler renderObj = new RenderSemanticsGestureHandler(
+      onTap: () {},
+      onHorizontalDragUpdate: (DragUpdateDetails details) {},
+    );
+
+    renderObj.semanticsAnnotator(node);
+    expect(node.getSemanticsData().hasAction(SemanticsAction.tap), isTrue);
+    expect(node.getSemanticsData().hasAction(SemanticsAction.scrollLeft), isTrue);
+    expect(node.getSemanticsData().hasAction(SemanticsAction.scrollRight), isTrue);
+
+    node = new SemanticsNode();
+    renderObj.validActions = <SemanticsAction>[SemanticsAction.tap, SemanticsAction.scrollLeft].toSet();
+
+    renderObj.semanticsAnnotator(node);
+    expect(node.getSemanticsData().hasAction(SemanticsAction.tap), isTrue);
+    expect(node.getSemanticsData().hasAction(SemanticsAction.scrollLeft), isTrue);
+    expect(node.getSemanticsData().hasAction(SemanticsAction.scrollRight), isFalse);
+  });
 }

packages/flutter/test/rendering/semantics_test.dart

@@ -5,10 +5,11 @@
 import 'package:flutter/rendering.dart';
 import 'package:test/test.dart';
 
+import 'rendering_tester.dart';
+
 
 void main() {
   group('SemanticsNode', () {
-
     const SemanticsTag tag1 = const SemanticsTag('Tag One');
     const SemanticsTag tag2 = const SemanticsTag('Tag Two');
     const SemanticsTag tag3 = const SemanticsTag('Tag Three');
@@ -19,35 +20,19 @@ void main() {
       expect(node.hasTag(tag1), isFalse);
       expect(node.hasTag(tag2), isFalse);
 
-      node.ensureTag(tag1);
-      expect(node.hasTag(tag1), isTrue);
-      expect(node.hasTag(tag2), isFalse);
-
-      node.ensureTag(tag2, isPresent: false);
+      node.addTag(tag1);
       expect(node.hasTag(tag1), isTrue);
       expect(node.hasTag(tag2), isFalse);
 
-      node.ensureTag(tag2, isPresent: true);
-      expect(node.hasTag(tag1), isTrue);
-      expect(node.hasTag(tag2), isTrue);
-
-      node.ensureTag(tag2, isPresent: true);
+      node.addTag(tag2);
       expect(node.hasTag(tag1), isTrue);
       expect(node.hasTag(tag2), isTrue);
-
-      node.ensureTag(tag1, isPresent: false);
-      expect(node.hasTag(tag1), isFalse);
-      expect(node.hasTag(tag2), isTrue);
-
-      node.ensureTag(tag2, isPresent: false);
-      expect(node.hasTag(tag1), isFalse);
-      expect(node.hasTag(tag2), isFalse);
     });
 
     test('getSemanticsData includes tags', () {
       final SemanticsNode node = new SemanticsNode()
-        ..ensureTag(tag1)
-        ..ensureTag(tag2);
+        ..addTag(tag1)
+        ..addTag(tag2);
 
       final Set<SemanticsTag> expected = new Set<SemanticsTag>()
         ..add(tag1)
@@ -57,7 +42,7 @@ void main() {
 
       node.mergeAllDescendantsIntoThisNode = true;
       node.addChildren(<SemanticsNode>[
-        new SemanticsNode()..ensureTag(tag3)
+        new SemanticsNode()..addTag(tag3)
       ]);
       node.finalizeChildren();
 
@@ -65,6 +50,66 @@ void main() {
 
       expect(node.getSemanticsData().tags, expected);
     });
+
+    test('after markNeedsSemanticsUpdate(onlyLocalUpdates: true) all render objects between two semantic boundaries are asked for annotations', () {
+      renderer.pipelineOwner.ensureSemantics();
+
+      TestRender middle;
+      final TestRender root = new TestRender(
+        action: SemanticsAction.tap,
+        isSemanticBoundary: true,
+        child: new TestRender(
+          action: SemanticsAction.longPress,
+          isSemanticBoundary: false,
+          child: middle = new TestRender(
+            action: SemanticsAction.scrollLeft,
+            isSemanticBoundary: false,
+            child: new TestRender(
+              action: SemanticsAction.scrollRight,
+              isSemanticBoundary: false,
+              child: new TestRender(
+                action: SemanticsAction.scrollUp,
+                isSemanticBoundary: true,
+              )
+            )
+          )
+        )
+      );
+
+      layout(root);
+      pumpFrame(phase: EnginePhase.flushSemantics);
+
+      int expectedActions = SemanticsAction.tap.index | SemanticsAction.longPress.index | SemanticsAction.scrollLeft.index | SemanticsAction.scrollRight.index;
+      expect(root.debugSemantics.getSemanticsData().actions, expectedActions);
+
+      middle.action = SemanticsAction.scrollDown;
+      middle.markNeedsSemanticsUpdate(onlyLocalUpdates: true);
+      expect(root.debugSemantics.getSemanticsData().actions, 0); // SemanticsNode is reset
+
+      pumpFrame(phase: EnginePhase.flushSemantics);
+
+      expectedActions = SemanticsAction.tap.index | SemanticsAction.longPress.index | SemanticsAction.scrollDown.index | SemanticsAction.scrollRight.index;
+      expect(root.debugSemantics.getSemanticsData().actions, expectedActions);
+
+      debugDumpSemanticsTree();
+    });
   });
+}
+
+class TestRender extends RenderProxyBox {
+
+  TestRender({ this.action, this.isSemanticBoundary, RenderObject child }) : super(child);
+
+  @override
+  final bool isSemanticBoundary;
+
+  SemanticsAction action;
+
+  @override
+  SemanticsAnnotator get semanticsAnnotator => _annotate;
+
+  void _annotate(SemanticsNode node) {
+    node.addAction(action);
+  }
 
 }

packages/flutter/test/widgets/semantics_10_test.dart

@@ -103,7 +103,7 @@ class RenderTest extends RenderProxyBox {
     if (value == _label)
       return;
     _label = value;
-    markNeedsSemanticsUpdate(onlyChanges: true);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: true);
     callLog.add('markNeedsSemanticsUpdate(onlyChanges: true)');
   }
 
@@ -114,7 +114,7 @@ class RenderTest extends RenderProxyBox {
     if (_isSemanticBoundary == value)
       return;
     _isSemanticBoundary = value;
-    markNeedsSemanticsUpdate(onlyChanges: false);
+    markNeedsSemanticsUpdate(onlyLocalUpdates: false);
     callLog.add('markNeedsSemanticsUpdate(onlyChanges: false)');
   }
 }

packages/flutter/test/widgets/semantics_11_test.dart

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter/material.dart';
+import 'package:flutter/rendering.dart';
+import 'package:flutter/widgets.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+import 'semantics_tester.dart';
+
+void main() {
+  testWidgets('markNeedsSemanticsUpdate allways resets node', (WidgetTester tester) async {
+    final SemanticsTester semantics = new SemanticsTester(tester);
+
+    await tester.pumpWidget(new TestWidget());
+    final RenderTest renderObj = tester.renderObject(find.byType(TestWidget));
+    expect(renderObj.labelWasReset, hasLength(1));
+    expect(renderObj.labelWasReset.last, true);
+    expect(semantics, includesNodeWith(label: 'Label 1'));
+
+    renderObj.markNeedsSemanticsUpdate(onlyLocalUpdates: false, noGeometry: false);
+    await tester.pumpAndSettle();
+
+    expect(renderObj.labelWasReset, hasLength(2));
+    expect(renderObj.labelWasReset.last, true);
+    expect(semantics, includesNodeWith(label: 'Label 2'));
+
+    renderObj.markNeedsSemanticsUpdate(onlyLocalUpdates: true, noGeometry: false);
+    await tester.pumpAndSettle();
+
+    expect(renderObj.labelWasReset, hasLength(3));
+    expect(renderObj.labelWasReset.last, true);
+    expect(semantics, includesNodeWith(label: 'Label 3'));
+
+    renderObj.markNeedsSemanticsUpdate(onlyLocalUpdates: true, noGeometry: true);
+    await tester.pumpAndSettle();
+
+    expect(renderObj.labelWasReset, hasLength(4));
+    expect(renderObj.labelWasReset.last, true);
+    expect(semantics, includesNodeWith(label: 'Label 4'));
+
+    renderObj.markNeedsSemanticsUpdate(onlyLocalUpdates: false, noGeometry: true);
+    await tester.pumpAndSettle();
+
+    expect(renderObj.labelWasReset, hasLength(5));
+    expect(renderObj.labelWasReset.last, true);
+    expect(semantics, includesNodeWith(label: 'Label 5'));
+
+    semantics.dispose();
+  });
+}
+
+class TestWidget extends SingleChildRenderObjectWidget {
+  const TestWidget({
+    Key key,
+    Widget child,
+  }) : super(key: key, child: child);
+
+  @override
+  RenderTest createRenderObject(BuildContext context) {
+    return new RenderTest();
+  }
+}
+
+class RenderTest extends RenderProxyBox {
+  List<bool> labelWasReset = <bool>[];
+
+  @override
+  SemanticsAnnotator get semanticsAnnotator => _annotate;
+
+  void _annotate(SemanticsNode node) {
+    labelWasReset.add(node.label == '');
+    node.label = "Label ${labelWasReset.length}";
+  }
+}