From f07170b45ba9df79501130cea7ea4b05fa86a29a Mon Sep 17 00:00:00 2001 From: Michael Goderbauer Date: Wed, 11 Oct 2017 13:07:19 -0700 Subject: [PATCH] Update Semantics for SingleChildScrollViews (#12376) * Update Semantics for SingleChildScrollViews * refactor * review feedback * added assert and comments * doc --- .../flutter/lib/src/widgets/framework.dart | 5 ++ .../lib/src/widgets/gesture_detector.dart | 18 ++++--- .../lib/src/widgets/scroll_position.dart | 20 +++++++- .../src/widgets/single_child_scroll_view.dart | 13 +++-- packages/flutter/test/material/tabs_test.dart | 50 +++++++++++++++++++ 5 files changed, 93 insertions(+), 13 deletions(-) diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 35d3039eda..bc01d99c54 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -2145,6 +2145,11 @@ class BuildOwner { int _debugStateLockLevel = 0; bool get _debugStateLocked => _debugStateLockLevel > 0; + + /// Whether this widget tree is in the build phase. + /// + /// Only valid when asserts are enabled. + bool get debugBuilding => _debugBuilding; bool _debugBuilding = false; Element _debugCurrentBuildTarget; diff --git a/packages/flutter/lib/src/widgets/gesture_detector.dart b/packages/flutter/lib/src/widgets/gesture_detector.dart index 7273be6af9..93c63729ed 100644 --- a/packages/flutter/lib/src/widgets/gesture_detector.dart +++ b/packages/flutter/lib/src/widgets/gesture_detector.dart @@ -535,29 +535,31 @@ class RawGestureDetectorState extends State { } } - /// This method can be called after the build phase, during the layout of the - /// nearest descendant [RenderObjectWidget] of the gesture detector, to filter - /// the list of available semantic actions. + /// This method can be called outside of the build phase to filter the list of + /// available semantic actions. + /// + /// The actual filtering is happening in the next frame and a frame will be + /// scheduled if non is pending. /// /// This is used by [Scrollable] to configure system accessibility tools so /// that they know in which direction a particular list can be scrolled. /// /// If this is never called, then the actions are not filtered. If the list of - /// actions to filter changes, it must be called again (during the layout of - /// the nearest descendant [RenderObjectWidget] of the gesture detector). + /// actions to filter changes, it must be called again. void replaceSemanticsActions(Set actions) { assert(() { - if (!context.findRenderObject().owner.debugDoingLayout) { + final Element element = context; + if (element.owner.debugBuilding) { throw new FlutterError( 'Unexpected call to replaceSemanticsActions() method of RawGestureDetectorState.\n' - 'The replaceSemanticsActions() method can only be called during the layout phase.' + 'The replaceSemanticsActions() method can only be called outside of the build phase.' ); } return true; }()); if (!widget.excludeFromSemantics) { final RenderSemanticsGestureHandler semanticsGestureHandler = context.findRenderObject(); - semanticsGestureHandler.validActions = actions; + semanticsGestureHandler.validActions = actions; // will call _markNeedsSemanticsUpdate(), if required. } } diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index bf0b4d2165..820e34e9f4 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -371,6 +371,18 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { Set _semanticActions; + /// Called whenever the scroll position or the dimensions of the scroll view + /// change to schedule an update of the available semantics actions. The + /// actual update will be performed in the nxt frame. If non is pending + /// a frame will be scheduled. + /// + /// For example: If the scroll view has been scrolled all the way to the top, + /// the action to scroll further up needs to be removed as the scroll view + /// cannot be scrolled in that direction anymore. + /// + /// This method is potentially called twice per frame (if scroll position and + /// scroll view dimensions both change) and therefore shouldn't do anything + /// expensive. void _updateSemanticActions() { SemanticsAction forward; SemanticsAction backward; @@ -409,7 +421,6 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { applyNewDimensions(); _didChangeViewportDimension = false; } - _updateSemanticActions(); return true; } @@ -437,6 +448,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { void applyNewDimensions() { assert(pixels != null); activity.applyNewDimensions(); + _updateSemanticActions(); // will potentially request a semantics update. } /// Animates the position such that the given object is as visible as possible @@ -613,6 +625,12 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { super.dispose(); } + @override + void notifyListeners() { + _updateSemanticActions(); // will potentially request a semantics update. + super.notifyListeners(); + } + @override void debugFillDescription(List description) { if (debugLabel != null) diff --git a/packages/flutter/lib/src/widgets/single_child_scroll_view.dart b/packages/flutter/lib/src/widgets/single_child_scroll_view.dart index a9c513e071..c852fe6113 100644 --- a/packages/flutter/lib/src/widgets/single_child_scroll_view.dart +++ b/packages/flutter/lib/src/widgets/single_child_scroll_view.dart @@ -205,13 +205,18 @@ class _RenderSingleChildViewport extends RenderBox with RenderObjectWithChildMix if (value == _offset) return; if (attached) - _offset.removeListener(markNeedsPaint); + _offset.removeListener(_hasScrolled); _offset = value; if (attached) - _offset.addListener(markNeedsPaint); + _offset.addListener(_hasScrolled); markNeedsLayout(); } + void _hasScrolled() { + markNeedsPaint(); + markNeedsSemanticsUpdate(); + } + @override void setupParentData(RenderObject child) { // We don't actually use the offset argument in BoxParentData, so let's @@ -223,12 +228,12 @@ class _RenderSingleChildViewport extends RenderBox with RenderObjectWithChildMix @override void attach(PipelineOwner owner) { super.attach(owner); - _offset.addListener(markNeedsPaint); + _offset.addListener(_hasScrolled); } @override void detach() { - _offset.removeListener(markNeedsPaint); + _offset.removeListener(_hasScrolled); super.detach(); } diff --git a/packages/flutter/test/material/tabs_test.dart b/packages/flutter/test/material/tabs_test.dart index cf679a639c..03de12cf86 100644 --- a/packages/flutter/test/material/tabs_test.dart +++ b/packages/flutter/test/material/tabs_test.dart @@ -1038,6 +1038,56 @@ void main() { semantics.dispose(); }); + testWidgets('correct scrolling semantics', (WidgetTester tester) async { + final SemanticsTester semantics = new SemanticsTester(tester); + + final List tabs = new List.generate(20, (int index) { + return new Tab(text: 'This is a very wide tab #$index'); + }); + + final TabController controller = new TabController( + vsync: const TestVSync(), + length: tabs.length, + initialIndex: 0, + ); + + await tester.pumpWidget( + boilerplate( + child: new Semantics( + container: true, + child: new TabBar( + isScrollable: true, + controller: controller, + tabs: tabs, + ), + ), + ), + ); + + expect(semantics, includesNodeWith(actions: [SemanticsAction.scrollLeft])); + expect(semantics, isNot(includesNodeWith(label: 'This is a very wide tab #10'))); + + controller.index = 10; + await tester.pumpAndSettle(); + + expect(semantics, isNot(includesNodeWith(label: 'This is a very wide tab #0'))); + expect(semantics, includesNodeWith(actions: [SemanticsAction.scrollLeft, SemanticsAction.scrollRight])); + expect(semantics, includesNodeWith(label: 'This is a very wide tab #10')); + + controller.index = 19; + await tester.pumpAndSettle(); + + expect(semantics, includesNodeWith(actions: [SemanticsAction.scrollRight])); + + controller.index = 0; + await tester.pumpAndSettle(); + + expect(semantics, includesNodeWith(actions: [SemanticsAction.scrollLeft])); + expect(semantics, includesNodeWith(label: 'This is a very wide tab #0')); + + semantics.dispose(); + }); + testWidgets('TabBar etc with zero tabs', (WidgetTester tester) async { final TabController controller = new TabController( vsync: const TestVSync(),