Adapt markNeedsSemanticsUpdate algorithm to new semantics tree compiler (#13274)

* ensures that only semantics boundaries will be added to owner._nodesNeedingSemantics as expected by compiler.
* no longer throws assert if markNeedsSemanticsUpdate is called on non-semantic-boundary render object with a non-semantic-boundary parent.
* Fixes #13109.
* removes onlyLocalUpdates from markNeedsSemanticsUpdate as its no longer needed.
This commit is contained in:
Michael Goderbauer 2017-11-30 12:18:33 -08:00 committed by GitHub
parent a78c9f70da
commit 6493c8b43d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 176 additions and 154 deletions

View file

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

View file

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

View file

@ -2201,81 +2201,53 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
/// Mark this node as needing an update to its semantics description.
///
/// `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 `onlyLocalUpdates` 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.
///
/// If `onlyLocalUpdates` is incorrectly set to true, asserts
/// might throw or the computed semantics tree might be out-of-date without
/// warning.
void markNeedsSemanticsUpdate({ bool onlyLocalUpdates: false }) {
/// This must be called whenever the semantics configuration of this
/// [RenderObject] as annotated by [describeSemanticsConfiguration] changes in
/// any way to update the semantics tree.
void markNeedsSemanticsUpdate() {
assert(!attached || !owner._debugDoingSemantics);
_cachedSemanticsConfiguration = null;
if ((attached && owner._semanticsOwner == null))
return;
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
// ancestor's; mark all the nodes up to that one dirty.
RenderObject node = this;
while (node._semantics == null && node.parent is RenderObject) {
if (node._needsSemanticsUpdate)
return;
// Dirty the semantics tree starting at `this` until we have reached a
// RenderObject that is a semantics boundary. All semantics past this
// RenderObject are still up-to date. Therefore, we will later only rebuild
// the semantics subtree starting at th identified semantics boundary.
final bool wasSemanticsBoundary = _cachedSemanticsConfiguration?.isSemanticBoundary == true;
_cachedSemanticsConfiguration = null;
bool isEffectiveSemanticsBoundary = _semanticsConfiguration.isSemanticBoundary && wasSemanticsBoundary;
RenderObject node = this;
while (!isEffectiveSemanticsBoundary && node.parent is RenderObject) {
if (node != this && node._needsSemanticsUpdate)
break;
node._needsSemanticsUpdate = true;
node = node.parent;
node._cachedSemanticsConfiguration = null;
isEffectiveSemanticsBoundary = node._semanticsConfiguration.isSemanticBoundary;
}
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) {
if (node != this) {
// Reset for `this` happened above already.
node._cachedSemanticsConfiguration = null;
node._needsSemanticsUpdate = true;
node = node.parent;
}
if (!node._needsSemanticsUpdate) {
node._needsSemanticsUpdate = true;
node._cachedSemanticsConfiguration = null;
if (owner != null) {
owner._nodesNeedingSemantics.add(node);
owner.requestVisualUpdate();
}
}
} else {
// The shape of the semantics tree around us may have changed.
// The worst case is that we may have removed a branch of the
// semantics tree, because when that happens we have to go up
// and dirty the nearest _semantics-laden ancestor of the
// affected node to rebuild the tree.
RenderObject node = this;
do {
if (node.parent is! RenderObject)
break;
node._needsSemanticsUpdate = true;
node._cachedSemanticsConfiguration = null;
node = node.parent;
} while (node._semantics == null);
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._cachedSemanticsConfiguration = null;
node._needsSemanticsUpdate = true;
if (owner != null) {
owner._nodesNeedingSemantics.add(node);
owner.requestVisualUpdate();
}
node._needsSemanticsUpdate = true;
if (owner != null) {
assert(node._semanticsConfiguration.isSemanticBoundary || node.parent is! RenderObject);
owner._nodesNeedingSemantics.add(node);
owner.requestVisualUpdate();
}
}
}

View file

@ -992,7 +992,7 @@ abstract class _RenderCustomClip<T> extends RenderProxyBox {
void _markNeedsClip() {
_clip = null;
markNeedsPaint();
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
T get _defaultClip;
@ -3003,7 +3003,7 @@ class RenderSemanticsGestureHandler extends RenderProxyBox {
if (setEquals<SemanticsAction>(value, _validActions))
return;
_validActions = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// Called when the user taps on the render object.
@ -3012,11 +3012,10 @@ class RenderSemanticsGestureHandler extends RenderProxyBox {
set onTap(GestureTapCallback value) {
if (_onTap == value)
return;
final bool hadHandlers = _hasHandlers;
final bool hadHandler = _onTap != null;
_onTap = value;
if ((value != null) != hadHandler)
markNeedsSemanticsUpdate(onlyLocalUpdates: _hasHandlers == hadHandlers);
markNeedsSemanticsUpdate();
}
/// Called when the user presses on the render object for a long period of time.
@ -3025,11 +3024,10 @@ class RenderSemanticsGestureHandler extends RenderProxyBox {
set onLongPress(GestureLongPressCallback value) {
if (_onLongPress == value)
return;
final bool hadHandlers = _hasHandlers;
final bool hadHandler = _onLongPress != null;
_onLongPress = value;
if ((value != null) != hadHandler)
markNeedsSemanticsUpdate(onlyLocalUpdates: _hasHandlers == hadHandlers);
markNeedsSemanticsUpdate();
}
/// Called when the user scrolls to the left or to the right.
@ -3038,11 +3036,10 @@ class RenderSemanticsGestureHandler extends RenderProxyBox {
set onHorizontalDragUpdate(GestureDragUpdateCallback value) {
if (_onHorizontalDragUpdate == value)
return;
final bool hadHandlers = _hasHandlers;
final bool hadHandler = _onHorizontalDragUpdate != null;
_onHorizontalDragUpdate = value;
if ((value != null) != hadHandler)
markNeedsSemanticsUpdate(onlyLocalUpdates: _hasHandlers == hadHandlers);
markNeedsSemanticsUpdate();
}
/// Called when the user scrolls up or down.
@ -3051,11 +3048,10 @@ class RenderSemanticsGestureHandler extends RenderProxyBox {
set onVerticalDragUpdate(GestureDragUpdateCallback value) {
if (_onVerticalDragUpdate == value)
return;
final bool hadHandlers = _hasHandlers;
final bool hadHandler = _onVerticalDragUpdate != null;
_onVerticalDragUpdate = value;
if ((value != null) != hadHandler)
markNeedsSemanticsUpdate(onlyLocalUpdates: _hasHandlers == hadHandlers);
markNeedsSemanticsUpdate();
}
/// The fraction of the dimension of this render box to use when
@ -3296,9 +3292,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set checked(bool value) {
if (checked == value)
return;
final bool hadValue = checked != null;
_checked = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.isSelected] semantic to the given
@ -3308,9 +3303,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set selected(bool value) {
if (selected == value)
return;
final bool hadValue = selected != null;
_selected = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.isButton] semantic to the given value.
@ -3319,9 +3313,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set button(bool value) {
if (button == value)
return;
final bool hadValue = button != null;
_button = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.label] semantic to the given value.
@ -3332,9 +3325,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set label(String value) {
if (_label == value)
return;
final bool hadValue = _label != null;
_label = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.value] semantic to the given value.
@ -3345,9 +3337,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set value(String value) {
if (_value == value)
return;
final bool hadValue = _value != null;
_value = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.increasedValue] semantic to the given
@ -3359,9 +3350,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set increasedValue(String value) {
if (_increasedValue == value)
return;
final bool hadValue = _increasedValue != null;
_increasedValue = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.decreasedValue] semantic to the given
@ -3373,9 +3363,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set decreasedValue(String value) {
if (_decreasedValue == value)
return;
final bool hadValue = _decreasedValue != null;
_decreasedValue = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.hint] semantic to the given value.
@ -3386,9 +3375,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set hint(String value) {
if (_hint == value)
return;
final bool hadValue = _hint != null;
_hint = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// If non-null, sets the [SemanticsNode.textDirection] semantic to the given value.
@ -3400,9 +3388,8 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
set textDirection(TextDirection value) {
if (textDirection == value)
return;
final bool hadValue = textDirection != null;
_textDirection = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: (value != null) == hadValue);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.tap].
@ -3421,7 +3408,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onTap != null;
_onTap = handler;
if ((handler != null) == hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.longPress].
@ -3440,7 +3427,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onLongPress != null;
_onLongPress = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.scrollLeft].
@ -3462,7 +3449,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onScrollLeft != null;
_onScrollLeft = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.scrollRight].
@ -3484,7 +3471,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onScrollRight != null;
_onScrollRight = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.scrollUp].
@ -3506,7 +3493,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onScrollUp != null;
_onScrollUp = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.scrollDown].
@ -3528,7 +3515,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onScrollDown != null;
_onScrollDown = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.increase].
@ -3547,7 +3534,7 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onIncrease != null;
_onIncrease = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
/// The handler for [SemanticsAction.decrease].
@ -3566,20 +3553,11 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
final bool hadValue = _onDecrease != null;
_onDecrease = handler;
if ((handler != null) != hadValue)
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
markNeedsSemanticsUpdate();
}
@override
void describeSemanticsConfiguration(SemanticsConfiguration config) {
assert(
onIncrease == null || (value == null) == (increasedValue == null),
'If "onIncrease" is set either both "value" and "increasedValue" or neither have to be set.',
);
assert(
onDecrease == null || (value == null) == (decreasedValue == null),
'If "onDecrease" is set either both "value" and "decreasedValue" or neither have to be set.',
);
config.isSemanticBoundary = container;
config.explicitChildNodes = explicitChildNodes;

View file

@ -21,21 +21,6 @@ void main() {
renderObject.markNeedsSemanticsUpdate();
expect(onNeedVisualUpdateCallCount, 2);
});
test('ensure frame is scheduled for markNeedsSemanticsUpdate with onlyChanges: true', () {
final TestRenderObject renderObject = new TestRenderObject();
int onNeedVisualUpdateCallCount = 0;
final PipelineOwner owner = new PipelineOwner(onNeedVisualUpdate: () {
onNeedVisualUpdateCallCount +=1;
});
owner.ensureSemantics();
renderObject.attach(owner);
owner.flushSemantics();
expect(onNeedVisualUpdateCallCount, 1);
renderObject.markNeedsSemanticsUpdate(onlyLocalUpdates: true);
expect(onNeedVisualUpdateCallCount, 2);
});
}
class TestRenderObject extends RenderObject {

View file

@ -30,7 +30,7 @@ void main() {
// Initial render does semantics.
expect(semanticsUpdateCount, 1);
expect(testRender.describeSemanticsConfigurationCallCount, isNot(0));
expect(testRender.describeSemanticsConfigurationCallCount, isPositive);
testRender.describeSemanticsConfigurationCallCount = 0;
semanticsUpdateCount = 0;
@ -41,7 +41,7 @@ void main() {
// Object is asked for semantics, but no update is sent.
expect(semanticsUpdateCount, 0);
expect(testRender.describeSemanticsConfigurationCallCount, 1);
expect(testRender.describeSemanticsConfigurationCallCount, isPositive);
testRender.describeSemanticsConfigurationCallCount = 0;
semanticsUpdateCount = 0;
@ -53,7 +53,7 @@ void main() {
// Object is asked for semantics, and update is sent.
expect(semanticsUpdateCount, 1);
expect(testRender.describeSemanticsConfigurationCallCount, 1);
expect(testRender.describeSemanticsConfigurationCallCount, isPositive);
semanticsHandle.dispose();
});

View file

@ -60,7 +60,7 @@ void main() {
expect(node.getSemanticsData().tags, tags);
});
test('after markNeedsSemanticsUpdate(onlyLocalUpdates: true) all render objects between two semantic boundaries are asked for annotations', () {
test('after markNeedsSemanticsUpdate() all render objects between two semantic boundaries are asked for annotations', () {
renderer.pipelineOwner.ensureSemantics();
TestRender middle;
@ -92,7 +92,7 @@ void main() {
expect(root.debugSemantics.getSemanticsData().actions, expectedActions);
middle.action = SemanticsAction.scrollDown;
middle.markNeedsSemanticsUpdate(onlyLocalUpdates: true);
middle.markNeedsSemanticsUpdate();
pumpFrame(phase: EnginePhase.flushSemantics);

View file

@ -223,7 +223,6 @@ void main() {
' └─child: RenderSemanticsGestureHandler#00000\n'
' │ parentData: <none> (can use size)\n'
' │ constraints: BoxConstraints(w=800.0, h=600.0)\n'
' │ semantic boundary\n'
' │ size: Size(800.0, 600.0)\n'
' │ gestures: vertical scroll\n'
'\n'
@ -328,7 +327,6 @@ void main() {
' └─child: RenderSemanticsGestureHandler#00000\n'
' │ parentData: <none> (can use size)\n'
' │ constraints: BoxConstraints(w=800.0, h=600.0)\n'
' │ semantic boundary\n'
' │ size: Size(800.0, 600.0)\n'
' │ gestures: vertical scroll\n'
'\n'

View file

@ -104,11 +104,11 @@ void main() {
final TestSemantics expectedSemantics = new TestSemantics.root(
children: <TestSemantics>[
new TestSemantics.rootChild(
id: 1,
id: 3,
rect: TestSemantics.fullScreen,
children: <TestSemantics>[
new TestSemantics(
id: 2,
id: 4,
rect: TestSemantics.fullScreen,
actions: SemanticsAction.tap.index,
),

View file

@ -9,10 +9,8 @@ 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 {
testWidgets('can cease to be semantics boundary after markNeedsSemanticsUpdate() has already been called once', (WidgetTester tester) async {
final SemanticsTester semantics = new SemanticsTester(tester);
await tester.pumpWidget(
@ -23,8 +21,6 @@ void main() {
),
);
callLog.clear();
// The following should not trigger an assert.
await tester.pumpWidget(
buildTestWidgets(
@ -34,8 +30,6 @@ void main() {
),
);
expect(callLog, <String>['markNeedsSemanticsUpdate(onlyChanges: true)', 'markNeedsSemanticsUpdate(onlyChanges: false)']);
semantics.dispose();
});
}
@ -109,22 +103,20 @@ class RenderTest extends RenderProxyBox {
}
String _label;
String _label = '<>';
set label(String value) {
if (value == _label)
return;
_label = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: true);
callLog.add('markNeedsSemanticsUpdate(onlyChanges: true)');
markNeedsSemanticsUpdate();
}
bool _isSemanticBoundary;
bool _isSemanticBoundary = false;
set isSemanticBoundary(bool value) {
if (_isSemanticBoundary == value)
return;
_isSemanticBoundary = value;
markNeedsSemanticsUpdate(onlyLocalUpdates: false);
callLog.add('markNeedsSemanticsUpdate(onlyChanges: false)');
markNeedsSemanticsUpdate();
}
}

View file

@ -0,0 +1,97 @@
// 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';
void main() {
testWidgets('markNeedsSemanticsUpdate() called on non-boundary with non-boundary parent', (WidgetTester tester) async {
final SemanticsTester semantics = new SemanticsTester(tester);
await tester.pumpWidget(
const Semantics(
container: true,
onTap: dummyTapHandler,
child: const Semantics(
onTap: dummyTapHandler,
child: const Semantics(
onTap: dummyTapHandler,
textDirection: TextDirection.ltr,
label: 'foo',
),
),
),
);
expect(semantics, hasSemantics(new TestSemantics.root(
children: <TestSemantics>[
new TestSemantics.rootChild(
id: 1,
actions: SemanticsAction.tap.index,
children: <TestSemantics>[
new TestSemantics(
id: 2,
actions: SemanticsAction.tap.index,
children: <TestSemantics>[
new TestSemantics(
id: 3,
actions: SemanticsAction.tap.index,
label: 'foo',
)
],
),
],
)
],
), ignoreRect: true, ignoreTransform: true));
// make a change causing call to markNeedsSemanticsUpdate()
// This should not throw an assert.
await tester.pumpWidget(
const Semantics(
container: true,
onTap: dummyTapHandler,
child: const Semantics(
onTap: dummyTapHandler,
child: const Semantics(
onTap: dummyTapHandler,
textDirection: TextDirection.ltr,
label: 'bar', // <-- only change
),
),
),
);
expect(semantics, hasSemantics(new TestSemantics.root(
children: <TestSemantics>[
new TestSemantics.rootChild(
id: 1,
actions: SemanticsAction.tap.index,
children: <TestSemantics>[
new TestSemantics(
id: 2,
actions: SemanticsAction.tap.index,
children: <TestSemantics>[
new TestSemantics(
id: 3,
actions: SemanticsAction.tap.index,
label: 'bar',
)
],
),
],
)
],
), ignoreRect: true, ignoreTransform: true));
semantics.dispose();
});
}
void dummyTapHandler() { }