Fix semantics compiler for offstage children (#24862)

Fixes #20313.

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

@@ -2387,6 +2387,11 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
   /// Updates the semantic information of the render object.
   void _updateSemantics() {
     assert(_semanticsConfiguration.isSemanticBoundary || parent is! RenderObject);
+    if (_needsLayout) {
+      // There's not enough information in this subtree to compute semantics.
+      // The subtree is probably being kept alive by a viewport but not laid out.
+      return;
+    }
     final _SemanticsFragment fragment = _getSemanticsForParent(
       mergeIntoParent: _semantics?.parent?.isPartOfNodeMerging ?? false,
     );
@@ -2405,6 +2410,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
     @required bool mergeIntoParent,
   }) {
     assert(mergeIntoParent != null);
+    assert(!_needsLayout, 'Updated layout information required for $this to calculate semantics.');
 
     final SemanticsConfiguration config = _semanticsConfiguration;
     bool dropSemanticsOfPreviousSiblings = config.isBlockingSemanticsOfPreviouslyPaintedNodes;
@@ -2414,10 +2420,25 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
     final Set<_InterestingSemanticsFragment> toBeMarkedExplicit = Set<_InterestingSemanticsFragment>();
     final bool childrenMergeIntoParent = mergeIntoParent || config.isMergingSemanticsOfDescendants;
 
+    // When set to true there's currently not enough information in this subtree
+    // to compute semantics. In this case the walk needs to be aborted and no
+    // SemanticsNodes in the subtree should be updated.
+    // This will be true for subtrees that are currently kept alive by a
+    // viewport but not laid out.
+    bool abortWalk = false;
+
     visitChildrenForSemantics((RenderObject renderChild) {
+      if (abortWalk || _needsLayout) {
+        abortWalk = true;
+        return;
+      }
       final _SemanticsFragment parentFragment = renderChild._getSemanticsForParent(
         mergeIntoParent: childrenMergeIntoParent,
       );
+      if (parentFragment.abortsWalk) {
+        abortWalk = true;
+        return;
+      }
       if (parentFragment.dropsSemanticsOfPreviousSiblings) {
         fragments.clear();
         toBeMarkedExplicit.clear();
@@ -2446,6 +2467,10 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
       }
     });
 
+    if (abortWalk) {
+      return _AbortingSemanticsFragment(owner: this);
+    }
+
     for (_InterestingSemanticsFragment fragment in toBeMarkedExplicit)
       fragment.markAsExplicit();
 
@@ -3109,6 +3134,17 @@ abstract class _SemanticsFragment {
   /// Returns [_InterestingSemanticsFragment] describing the actual semantic
   /// information that this fragment wants to add to the parent.
   Iterable<_InterestingSemanticsFragment> get interestingFragments;
+
+  /// Whether this fragment wants to abort the semantics walk because the
+  /// information in the tree are not sufficient to calculate semantics.
+  ///
+  /// This happens for subtrees that are currently kept alive by a viewport but
+  /// not laid out.
+  ///
+  /// See also:
+  ///
+  ///  * [_AbortingSemanticsFragment], which sets this to true.
+  bool get abortsWalk => false;
 }
 
 /// A container used when a [RenderObject] wants to add multiple independent
@@ -3392,6 +3428,39 @@ class _SwitchableSemanticsFragment extends _InterestingSemanticsFragment {
   bool get _needsGeometryUpdate => _ancestorChain.length > 1;
 }
 
+
+/// [_SemanticsFragment] used to indicate that the current information in this
+/// subtree is not sufficient to update semantics.
+///
+/// Anybody processing this [_SemanticsFragment] should abort the walk of the
+/// current subtree without updating any [SemanticsNode]s as there is no semantic
+/// information to compute. As a result, this fragment also doesn't carry any
+/// semantics information either.
+class _AbortingSemanticsFragment extends _InterestingSemanticsFragment {
+  _AbortingSemanticsFragment({@required RenderObject owner}) : super(owner: owner, dropsSemanticsOfPreviousSiblings: false);
+
+  @override
+  bool get abortsWalk => true;
+
+  @override
+  SemanticsConfiguration get config => null;
+
+  @override
+  void addAll(Iterable<_InterestingSemanticsFragment> fragments) {
+    assert(false);
+  }
+
+  @override
+  Iterable<SemanticsNode> compileChildren({Rect parentSemanticsClipRect, Rect parentPaintClipRect}) sync* {
+    yield owner._semantics;
+  }
+
+  @override
+  void markAsExplicit() {
+    // Is never explicit.
+  }
+}
+
 /// Helper class that keeps track of the geometry of a [SemanticsNode].
 ///
 /// It is used to annotate a [SemanticsNode] with the current information for

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

@@ -318,6 +318,12 @@ abstract class RenderSliverMultiBoxAdaptor extends RenderSliver
     _keepAliveBucket.values.forEach(visitor);
   }
 
+  @override
+  void visitChildrenForSemantics(RenderObjectVisitor visitor) {
+    super.visitChildren(visitor);
+    // Do not visit children in [_keepAliveBucket].
+  }
+
   /// Called during layout to create and add the child with the given index and
   /// scroll offset.
   ///
@@ -603,4 +609,4 @@ abstract class RenderSliverMultiBoxAdaptor extends RenderSliver
     }
     return children;
   }
-}
+}

packages/flutter/test/rendering/object_test.dart

@@ -15,6 +15,7 @@ void main() {
     });
     owner.ensureSemantics();
     renderObject.attach(owner);
+    renderObject.layout(const BoxConstraints.tightForFinite());  // semantics are only calculated if layout information is up to date.
     owner.flushSemantics();
 
     expect(onNeedVisualUpdateCallCount, 1);

packages/flutter/test/widgets/semantics_keep_alive_offstage_test.dart

@@ -0,0 +1,149 @@
+// Copyright 2018 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('Un-layouted RenderObject in keep alive offstage area do not crash semantics compiler', (WidgetTester tester) async {
+    // Regression test for https://github.com/flutter/flutter/issues/20313.
+
+    final SemanticsTester semantics = SemanticsTester(tester);
+
+    const String initialLabel = 'Foo';
+    const double bottomScrollOffset = 3000.0;
+
+    final ScrollController controller = ScrollController(initialScrollOffset: bottomScrollOffset);
+
+    await tester.pumpWidget(_buildTestWidget(
+      extraPadding: false,
+      text: initialLabel,
+      controller: controller,
+    ));
+    await tester.pumpAndSettle();
+
+    // The ProblemWidget has been instantiated (it is on screen).
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, initialLabel)), hasLength(1));
+    expect(semantics, includesNodeWith(label: initialLabel));
+
+    controller.jumpTo(0.0);
+    await tester.pumpAndSettle();
+
+    // The ProblemWidget is not on screen...
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, initialLabel)), hasLength(0));
+    // ... but still in the tree as offstage.
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, initialLabel, skipOffstage: false)), hasLength(1));
+    expect(semantics, isNot(includesNodeWith(label: initialLabel)));
+
+    // Introduce a new Padding widget to offstage subtree that will not get its
+    // size calculated because it's offstage.
+    await tester.pumpWidget(_buildTestWidget(
+      extraPadding: true,
+      text: initialLabel,
+      controller: controller,
+    ));
+    final RenderPadding renderPadding = tester.renderObject(find.byKey(paddingWidget, skipOffstage: false));
+    expect(renderPadding.hasSize, isFalse);
+    expect(semantics, isNot(includesNodeWith(label: initialLabel)));
+
+    // Change the semantics of the offstage ProblemWidget without crashing.
+    const String newLabel = 'Bar';
+    expect(newLabel, isNot(equals(initialLabel)));
+    await tester.pumpWidget(_buildTestWidget(
+      extraPadding: true,
+      text: newLabel,
+      controller: controller,
+    ));
+
+    // The label has changed.
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, initialLabel, skipOffstage: false)), hasLength(0));
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, newLabel, skipOffstage: false)), hasLength(1));
+    expect(semantics, isNot(includesNodeWith(label: initialLabel)));
+    expect(semantics, isNot(includesNodeWith(label: newLabel)));
+
+    // Bringing the offstage node back on the screen produces correct semantics tree.
+    controller.jumpTo(bottomScrollOffset);
+    await tester.pumpAndSettle();
+
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, initialLabel)), hasLength(0));
+    expect(tester.widgetList(find.widgetWithText(ProblemWidget, newLabel)), hasLength(1));
+    expect(semantics, isNot(includesNodeWith(label: initialLabel)));
+    expect(semantics, includesNodeWith(label: newLabel));
+
+    semantics.dispose();
+  });
+}
+
+final Key paddingWidget = GlobalKey();
+
+Widget _buildTestWidget({bool extraPadding, String text, ScrollController controller}) {
+  return MaterialApp(
+    home: Scaffold(
+      body: Column(
+        children: <Widget>[
+          Expanded(
+            child: Container(),
+          ),
+          Container(
+            height: 500.0,
+            child: ListView(
+              controller: controller,
+              children: List<Widget>.generate(10, (int i) {
+                return Container(
+                  color: i % 2 == 0 ? Colors.red : Colors.blue,
+                  height: 250.0,
+                  child: Text('Item $i'),
+                );
+              })..add(ProblemWidget(
+                extraPadding: extraPadding,
+                text: text,
+              )),
+            ),
+          ),
+          Expanded(
+            child: Container(),
+          ),
+        ],
+      ),
+    ),
+  );
+}
+
+class ProblemWidget extends StatefulWidget {
+  const ProblemWidget({Key key, this.extraPadding, this.text}) : super(key: key);
+
+  final bool extraPadding;
+  final String text;
+
+  @override
+  State<ProblemWidget> createState() => ProblemWidgetState();
+}
+
+class ProblemWidgetState extends State<ProblemWidget> with AutomaticKeepAliveClientMixin<ProblemWidget> {
+  @override
+  Widget build(BuildContext context) {
+    Widget child = Semantics(
+      container: true,
+      child: Text(widget.text),
+    );
+    if (widget.extraPadding) {
+      child = Semantics(
+        container: true,
+        child: Padding(
+          key: paddingWidget,
+          padding: const EdgeInsets.all(20.0),
+          child: child,
+        ),
+      );
+    }
+    return child;
+  }
+
+  @override
+  bool get wantKeepAlive => true;
+}