Commit b551f534 authored by Michael Goderbauer's avatar Michael Goderbauer Committed by GitHub

Don't trigger an assert when markNeedsSemanticsUpdate is called multiple times...

Don't trigger an assert when markNeedsSemanticsUpdate is called multiple times in edge cases (#11544)

* Don't trigger assert if a render object ceases to be a semantic boundary

This bug was exposed by https://github.com/flutter/flutter/pull/11309, which caused the following assertion to trigger when scrolling in the Animation demo:

```
The following assertion was thrown during _updateSemantics():
'package:flutter/src/rendering/object.dart': Failed assertion: line 2626 pos 16: 'fragment is
_InterestingSemanticsFragment': is not true.
```

A minimal reproduction of the bug can be found in `semantics_10_test.dart`, which has been added as a regression test for the bug by this PR.

Looking at that test, here is a description of the faulty behaviour:
1. During the second `pumpWidget` call `RenderExcludeSemantics` marks itself as needing a semantics update (due to excluding going from `false` -> `true`).
2. This causes the nearest ancestor with semantics information (here: `RenderSemanticsAnnotations` representing the "container" Semantics widget) to be added to the `_nodesNeedingSemantics` list.
3. `RenderSliverList` (implementation behind ListView) marks itself as needing a semantics update (due to its changing children).
4. This causes the `RenderSemanticsGestureHandler` to be added to the `_nodesNeedingSemantics` list.
5. Next, canDrag is updated from `true` -> `false`. This means, `RenderSemanticsGestureHandler` is no longer a semantics boundary, it marks itself as needing a semantics update.
6. The nearest ancestor with semantics (`RenderSemanticsAnnotations`, the "container") is added to the `_nodesNeedingSemantics` list (this is a no-op because it is already in the list).
7. During `flushSemantics`, the `_nodesNeedingSemantics` list is walked. The first entry (`RenderSemanticsAnnotations`) updates the semantics tree to only contain the container widget and drop everything else (= no children of the ExcludeSemantics widget are walked).
8. The second entry (`RenderSemanticsGestureHandler`) is updated. It does not add any semantics of its own and is no longer a semantics boundary. Therefore, it wants to merge its descendent semantics into its parents. Here is where the assert throws because the algorithm assumes that every entry in the `_nodesNeedingSemantics` list will produce and own an `_InterestingSemanticsFragment` (passing your semantics on to your parents is not interesting).

The problem here seems to be step 4 in combination with step 5. In step 4 we rely on the fact that `RenderSemanticsGestureHandler` is an (explicit or implicit) semantics boundary and that it will be able to absorb the semantics change of `RenderSliverList`. This is true
at this time. However, in step 4 `RenderSemanticsGestureHandler` decides to no longer be an (explicit or implicit) semantics boundary and our assumption from step 5 becomes incorrect. We did nothing to correct this assumption.

This PR removes a node, that could potentially cease to be a (explicit or implicit) semantics boundary from the `_nodesNeedingSemantics` list to fix that problem. Please node that this does not mean that the node's semantics will not be updated: The node's closest ances
tor with semantics is added to that list during the `markNeedsSemanticsUpdate` call. During `flushSemantics` we will walk from this node to update the semantics of it's children (if changed), which will include the node in question.

* tiny fix

* simplify test

* analyzer fixes

* review comments
parent 7d3a1f96
......@@ -164,7 +164,7 @@ Future<Null> runSmokeTest(WidgetTester tester) async {
void main() {
testWidgets('Flutter Gallery app smoke test', runSmokeTest);
testWidgets('Flutter Gallery app smoke test', (WidgetTester tester) async {
testWidgets('Flutter Gallery app smoke test with semantics', (WidgetTester tester) async {
RendererBinding.instance.setSemanticsEnabled(true);
await runSmokeTest(tester);
RendererBinding.instance.setSemanticsEnabled(false);
......
......@@ -1186,7 +1186,7 @@ class PipelineOwner {
}
bool _debugDoingSemantics = false;
final List<RenderObject> _nodesNeedingSemantics = <RenderObject>[];
final Set<RenderObject> _nodesNeedingSemantics = new Set<RenderObject>();
/// Update the semantics for render objects marked as needing a semantics
/// update.
......@@ -1206,14 +1206,16 @@ class PipelineOwner {
assert(_semanticsOwner != null);
assert(() { _debugDoingSemantics = true; return true; });
try {
_nodesNeedingSemantics.sort((RenderObject a, RenderObject b) => a.depth - b.depth);
for (RenderObject node in _nodesNeedingSemantics) {
final List<RenderObject> nodesToProcess = _nodesNeedingSemantics.toList()
..sort((RenderObject a, RenderObject b) => a.depth - b.depth);
_nodesNeedingSemantics.clear();
for (RenderObject node in nodesToProcess) {
if (node._needsSemanticsUpdate && node.owner == this)
node._updateSemantics();
}
_semanticsOwner.sendSemanticsUpdate();
} finally {
_nodesNeedingSemantics.clear();
assert(_nodesNeedingSemantics.isEmpty);
assert(() { _debugDoingSemantics = false; return true; });
Timeline.finishSync();
}
......@@ -2580,6 +2582,16 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
node = node.parent;
} while (node._semantics == null);
node._semantics?.reset();
if (node != this && _semantics != null && _needsSemanticsUpdate) {
// If [this] node has already been added to [owner._nodesNeedingSemantics]
// remove it as it is no longer guaranteed that its semantics
// node will continue to be in the tree. If it still is in the tree, the
// ancestor [node] added to [owner._nodesNeedingSemantics] at the end of
// this block will ensure that the semantics of [this] node actually get
// updated.
// (See semantics_10_test.dart for an example why this is required).
owner._nodesNeedingSemantics.remove(this);
}
if (!node._needsSemanticsUpdate) {
node._needsSemanticsUpdate = true;
if (owner != null) {
......
// 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/widgets.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart';
import 'semantics_tester.dart';
List<String> callLog = <String>[];
void main() {
testWidgets('can call markNeedsSemanticsUpdate(onlyChanges: true) followed by markNeedsSemanticsUpdate(onlyChanges: false)', (WidgetTester tester) async {
final SemanticsTester semantics = new SemanticsTester(tester);
await tester.pumpWidget(
buildTestWidgets(
excludeSemantics: false,
label: 'label',
isSemanticsBoundary: true,
)
);
callLog.clear();
// The following should not trigger an assert.
await tester.pumpWidget(
buildTestWidgets(
excludeSemantics: true,
label: 'label CHANGED',
isSemanticsBoundary: false,
)
);
expect(callLog, <String>['markNeedsSemanticsUpdate(onlyChanges: true)', 'markNeedsSemanticsUpdate(onlyChanges: false)']);
semantics.dispose();
});
}
Widget buildTestWidgets({bool excludeSemantics, String label, bool isSemanticsBoundary}) {
return new Semantics(
label: 'container',
container: true,
child: new ExcludeSemantics(
excluding: excludeSemantics,
child: new TestWidget(
label: label,
isSemanticBoundary: isSemanticsBoundary,
child: new Column(
children: <Widget>[
new Semantics(
label: 'child1',
),
new Semantics(
label: 'child2',
),
],
),
),
),
);
}
class TestWidget extends SingleChildRenderObjectWidget {
const TestWidget({
Key key,
Widget child,
this.label,
this.isSemanticBoundary,
}) : super(key: key, child: child);
final String label;
final bool isSemanticBoundary;
@override
RenderTest createRenderObject(BuildContext context) {
return new RenderTest()
..label = label
..isSemanticBoundary = isSemanticBoundary;
}
@override
void updateRenderObject(BuildContext context, RenderTest renderObject) {
renderObject
..label = label
..isSemanticBoundary = isSemanticBoundary;
}
}
class RenderTest extends RenderProxyBox {
@override
SemanticsAnnotator get semanticsAnnotator => isSemanticBoundary ? _annotate : null;
void _annotate(SemanticsNode node) {
node.label = _label;
}
String _label;
set label(String value) {
if (value == _label)
return;
_label = value;
markNeedsSemanticsUpdate(onlyChanges: true);
callLog.add('markNeedsSemanticsUpdate(onlyChanges: true)');
}
@override
bool get isSemanticBoundary => _isSemanticBoundary;
bool _isSemanticBoundary;
set isSemanticBoundary(bool value) {
if (_isSemanticBoundary == value)
return;
_isSemanticBoundary = value;
markNeedsSemanticsUpdate(onlyChanges: false);
callLog.add('markNeedsSemanticsUpdate(onlyChanges: false)');
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment