diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index cf837ecc82b..02ba09f2b28 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/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 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 diff --git a/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart b/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart index 3757425a33a..e719a493257 100644 --- a/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart +++ b/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; } -} \ No newline at end of file +} diff --git a/packages/flutter/test/rendering/object_test.dart b/packages/flutter/test/rendering/object_test.dart index 209ce4953c0..3d3ec89ac70 100644 --- a/packages/flutter/test/rendering/object_test.dart +++ b/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); diff --git a/packages/flutter/test/widgets/semantics_keep_alive_offstage_test.dart b/packages/flutter/test/widgets/semantics_keep_alive_offstage_test.dart new file mode 100644 index 00000000000..92d07368ca0 --- /dev/null +++ b/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: [ + Expanded( + child: Container(), + ), + Container( + height: 500.0, + child: ListView( + controller: controller, + children: List.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 createState() => ProblemWidgetState(); +} + +class ProblemWidgetState extends State with AutomaticKeepAliveClientMixin { + @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; +}