From f4904b14593e9d0df90902172f3a3fc571f919ad Mon Sep 17 00:00:00 2001 From: Hans Muller Date: Wed, 14 Sep 2016 10:44:51 -0700 Subject: [PATCH] Refresh indicator overscroll (#5836) * Added OverscrollIndicatorEdge et al * RefreshIndicator only clamps its scrollable edge * added a test * Updated the test * fixed lint-os * fixed a typo * Scrollable should restore its viewport dimensions when it reappears * removed an accidental commit * updated per review feedback --- .../lib/demo/overscroll_demo.dart | 48 +++---- .../flutter_gallery/lib/demo/pesto_demo.dart | 2 +- packages/flutter/lib/src/material/app.dart | 23 +++- .../flutter/lib/src/material/drop_down.dart | 4 +- .../src/material/overscroll_indicator.dart | 32 ++++- .../lib/src/material/refresh_indicator.dart | 17 ++- .../flutter/lib/src/material/scaffold.dart | 7 +- .../lib/src/widgets/clamp_overscrolls.dart | 123 +++++++++++++----- .../lib/src/widgets/scroll_configuration.dart | 6 +- .../flutter/lib/src/widgets/scrollable.dart | 18 +++ .../test/widget/clamp_overscrolls_test.dart | 73 +++++++++++ 11 files changed, 277 insertions(+), 76 deletions(-) create mode 100644 packages/flutter/test/widget/clamp_overscrolls_test.dart diff --git a/examples/flutter_gallery/lib/demo/overscroll_demo.dart b/examples/flutter_gallery/lib/demo/overscroll_demo.dart index 68c9af03487..b456c904259 100644 --- a/examples/flutter_gallery/lib/demo/overscroll_demo.dart +++ b/examples/flutter_gallery/lib/demo/overscroll_demo.dart @@ -45,46 +45,34 @@ class OverscrollDemoState extends State { @override Widget build(BuildContext context) { + Widget body = new MaterialList( + type: MaterialListType.threeLine, + padding: const EdgeInsets.all(8.0), + scrollableKey: _scrollableKey, + children: _items.map((String item) { + return new ListItem( + isThreeLine: true, + leading: new CircleAvatar(child: new Text(item)), + title: new Text('This item represents $item.'), + subtitle: new Text('Even more additional list item information appears on line three.') + ); + }) + ); + String indicatorTypeText; - switch(_type) { + switch (_type) { case IndicatorType.overscroll: indicatorTypeText = 'Over-scroll indicator'; break; - case IndicatorType.refresh: - indicatorTypeText = 'Refresh indicator'; - break; - } - - // The default ScrollConfiguration doesn't include the - // OverscrollIndicator. That's what we want, since this demo - // adds the OverscrollIndicator itself. - Widget body = new ScrollConfiguration( - child: new MaterialList( - type: MaterialListType.threeLine, - padding: const EdgeInsets.all(8.0), - scrollableKey: _scrollableKey, - children: _items.map((String item) { - return new ListItem( - isThreeLine: true, - leading: new CircleAvatar(child: new Text(item)), - title: new Text('This item represents $item.'), - subtitle: new Text('Even more additional list item information appears on line three.') - ); - }) - ) - ); - switch(_type) { - case IndicatorType.overscroll: - body = new OverscrollIndicator(child: body); - break; case IndicatorType.refresh: body = new RefreshIndicator( key: _refreshIndicatorKey, - child: body, refresh: refresh, scrollableKey: _scrollableKey, - location: RefreshIndicatorLocation.top + location: RefreshIndicatorLocation.top, + child: body, ); + indicatorTypeText = 'Refresh indicator'; break; } diff --git a/examples/flutter_gallery/lib/demo/pesto_demo.dart b/examples/flutter_gallery/lib/demo/pesto_demo.dart index f52dad56d0a..e537e9da2ac 100644 --- a/examples/flutter_gallery/lib/demo/pesto_demo.dart +++ b/examples/flutter_gallery/lib/demo/pesto_demo.dart @@ -304,7 +304,7 @@ class _RecipePageState extends State { ) ), new ClampOverscrolls( - value: true, + edge: ScrollableEdge.both, child: new ScrollableViewport( scrollableKey: _scrollableKey, child: new RepaintBoundary( diff --git a/packages/flutter/lib/src/material/app.dart b/packages/flutter/lib/src/material/app.dart index 0d919ca3c83..7e454e5d36f 100644 --- a/packages/flutter/lib/src/material/app.dart +++ b/packages/flutter/lib/src/material/app.dart @@ -173,8 +173,29 @@ class _ScrollLikeMountainViewDelegate extends ScrollConfigurationDelegate { @override ExtentScrollBehavior createScrollBehavior() => new OverscrollWhenScrollableBehavior(platform: TargetPlatform.android); + ScrollableEdge _overscrollIndicatorEdge(ScrollableEdge edge) { + switch (edge) { + case ScrollableEdge.leading: + return ScrollableEdge.trailing; + case ScrollableEdge.trailing: + return ScrollableEdge.leading; + case ScrollableEdge.both: + return ScrollableEdge.none; + case ScrollableEdge.none: + return ScrollableEdge.both; + } + return ScrollableEdge.both; + } + @override - Widget wrapScrollWidget(Widget scrollWidget) => new OverscrollIndicator(child: scrollWidget); + Widget wrapScrollWidget(BuildContext context, Widget scrollWidget) { + // Only introduce an overscroll indicator for the edges of the scrollable + // that aren't already clamped. + return new OverscrollIndicator( + edge: _overscrollIndicatorEdge(ClampOverscrolls.of(context)?.edge), + child: scrollWidget + ); + } @override bool updateShouldNotify(ScrollConfigurationDelegate old) => false; diff --git a/packages/flutter/lib/src/material/drop_down.dart b/packages/flutter/lib/src/material/drop_down.dart index c107e335153..601018886c3 100644 --- a/packages/flutter/lib/src/material/drop_down.dart +++ b/packages/flutter/lib/src/material/drop_down.dart @@ -89,7 +89,9 @@ class _DropDownScrollConfigurationDelegate extends ScrollConfigurationDelegate { ExtentScrollBehavior createScrollBehavior() => new OverscrollWhenScrollableBehavior(platform: platform); @override - Widget wrapScrollWidget(Widget scrollWidget) => new ClampOverscrolls(value: true, child: scrollWidget); + Widget wrapScrollWidget(BuildContext context, Widget scrollWidget) { + return new ClampOverscrolls(edge: ScrollableEdge.both, child: scrollWidget); + } @override bool updateShouldNotify(ScrollConfigurationDelegate old) => platform != old.platform; diff --git a/packages/flutter/lib/src/material/overscroll_indicator.dart b/packages/flutter/lib/src/material/overscroll_indicator.dart index 90bfe8241e8..fc4c6fa4ba8 100644 --- a/packages/flutter/lib/src/material/overscroll_indicator.dart +++ b/packages/flutter/lib/src/material/overscroll_indicator.dart @@ -50,7 +50,7 @@ class _Painter extends CustomPainter { final double width = size.width; final double height = size.height; - switch(scrollDirection) { + switch (scrollDirection) { case Axis.vertical: final double radius = width * _kSizeToRadius; final double centerX = width / 2.0; @@ -97,9 +97,11 @@ class OverscrollIndicator extends StatefulWidget { OverscrollIndicator({ Key key, this.scrollableKey, + this.edge: ScrollableEdge.both, this.child }) : super(key: key) { assert(child != null); + assert(edge != null); } /// Identifies the [Scrollable] descendant of child that the overscroll @@ -107,6 +109,9 @@ class OverscrollIndicator extends StatefulWidget { /// descendant. final Key scrollableKey; + /// Where the overscroll indicator should appear. + final ScrollableEdge edge; + /// The overscroll indicator will be stacked on top of this child. The /// indicator will appear when child's [Scrollable] descendant is /// over-scrolled. @@ -167,10 +172,12 @@ class _OverscrollIndicatorState extends State { // Hide the indicator as soon as user starts scrolling in the reverse direction of overscroll. if (_isReverseScroll(value)) { _hide(_kNormalHideDuration); - } else { + } else if (_isMatchingOverscrollEdge(value)) { // Changing the animation's value causes an implicit setState(). _dragPosition = details?.globalPosition ?? Point.origin; _extentAnimation.value = value < _minScrollOffset ? _minScrollOffset - value : value - _maxScrollOffset; + } else { + _hide(_kNormalHideDuration); } } _updateState(scrollable); @@ -194,6 +201,20 @@ class _OverscrollIndicatorState extends State { ((scrollOffset - _scrollOffset).abs() > kPixelScrollTolerance.distance); } + bool _isMatchingOverscrollEdge(double scrollOffset) { + switch (config.edge) { + case ScrollableEdge.both: + return true; + case ScrollableEdge.leading: + return scrollOffset < _minScrollOffset; + case ScrollableEdge.trailing: + return scrollOffset > _maxScrollOffset; + case ScrollableEdge.none: + return false; + } + return false; + } + bool _isReverseScroll(double scrollOffset) { final double delta = _scrollOffset - scrollOffset; return scrollOffset < _minScrollOffset ? delta < 0.0 : delta > 0.0; @@ -208,7 +229,7 @@ class _OverscrollIndicatorState extends State { } final ScrollableState scrollable = notification.scrollable; - switch(notification.kind) { + switch (notification.kind) { case ScrollNotificationKind.started: _onScrollStarted(scrollable); break; @@ -256,9 +277,10 @@ class _OverscrollIndicatorState extends State { child: child ); }, - child: new ClampOverscrolls( + child: new ClampOverscrolls.inherit( + context: context, + edge: config.edge, child: config.child, - value: true ) ) ); diff --git a/packages/flutter/lib/src/material/refresh_indicator.dart b/packages/flutter/lib/src/material/refresh_indicator.dart index fde1b973920..8a9fd12807a 100644 --- a/packages/flutter/lib/src/material/refresh_indicator.dart +++ b/packages/flutter/lib/src/material/refresh_indicator.dart @@ -331,6 +331,18 @@ class RefreshIndicatorState extends State { } } + ScrollableEdge get _clampOverscrollsEdge { + switch (config.location) { + case RefreshIndicatorLocation.top: + return ScrollableEdge.leading; + case RefreshIndicatorLocation.bottom: + return ScrollableEdge.trailing; + case RefreshIndicatorLocation.both: + return ScrollableEdge.both; + } + return ScrollableEdge.none; + } + @override Widget build(BuildContext context) { final ThemeData theme = Theme.of(context); @@ -353,9 +365,10 @@ class RefreshIndicatorState extends State { onPointerUp: _handlePointerUp, child: new Stack( children: [ - new ClampOverscrolls( + new ClampOverscrolls.inherit( + context: context, + edge: _clampOverscrollsEdge, child: config.child, - value: true ), new Positioned( top: _isIndicatorAtTop ? 0.0 : null, diff --git a/packages/flutter/lib/src/material/scaffold.dart b/packages/flutter/lib/src/material/scaffold.dart index 1778bf46be4..b20e8236bff 100644 --- a/packages/flutter/lib/src/material/scaffold.dart +++ b/packages/flutter/lib/src/material/scaffold.dart @@ -609,10 +609,9 @@ class ScaffoldState extends State { if ((scrollable.config.scrollDirection == Axis.vertical) && (config.scrollableKey == null || config.scrollableKey == scrollable.config.key)) { double newScrollOffset = scrollable.scrollOffset; - if (ClampOverscrolls.of(scrollable.context)) { - ExtentScrollBehavior limits = scrollable.scrollBehavior; - newScrollOffset = newScrollOffset.clamp(limits.minScrollOffset, limits.maxScrollOffset); - } + final ClampOverscrolls clampOverscrolls = ClampOverscrolls.of(context); + if (clampOverscrolls != null) + newScrollOffset = clampOverscrolls.clampScrollOffset(scrollable); if (_scrollOffset != newScrollOffset) { setState(() { _scrollOffsetDelta = _scrollOffset - newScrollOffset; diff --git a/packages/flutter/lib/src/widgets/clamp_overscrolls.dart b/packages/flutter/lib/src/widgets/clamp_overscrolls.dart index 20eb7634c76..bfe626ef0eb 100644 --- a/packages/flutter/lib/src/widgets/clamp_overscrolls.dart +++ b/packages/flutter/lib/src/widgets/clamp_overscrolls.dart @@ -13,39 +13,103 @@ import 'scrollable.dart'; /// scrolled to the given `scrollOffset`. typedef Widget ViewportBuilder(BuildContext context, ScrollableState state, double scrollOffset); -/// A widget that controls whether [Scrollable] descendants will overscroll. +/// A widget that controls whether viewport descendants will overscroll their contents. +/// Overscrolling is clamped at the beginning or end or both according to the +/// [edge] parameter. /// -/// If `true`, the ClampOverscroll's [Scrollable] descendant will clamp its -/// viewport's scrollOffsets to the [ScrollBehavior]'s min and max values. -/// In this case the Scrollable's scrollOffset will still over- and undershoot -/// the ScrollBehavior's limits, but the viewport itself will not. +/// Scroll offset limits are defined by the enclosing Scrollable's [ScrollBehavior]. class ClampOverscrolls extends InheritedWidget { - /// Creates a widget that controls whether [Scrollable] descendants will overscroll. + /// Creates a widget that controls whether viewport descendants will overscroll + /// their contents. /// - /// The [value] and [child] arguments must not be null. + /// The [edge] and [child] arguments must not be null. ClampOverscrolls({ Key key, - @required this.value, - @required Widget child + this.edge: ScrollableEdge.none, + @required Widget child, }) : super(key: key, child: child) { - assert(value != null); + assert(edge != null); assert(child != null); } - /// Whether [Scrollable] descendants should clamp their viewport's - /// scrollOffset values when they are less than the [ScrollBehavior]'s minimum - /// or greater than its maximum. - final bool value; + /// Creates a widget that controls whether viewport descendants will overscroll + /// based on the given [edge] and the inherited ClampOverscrolls widget for + /// the given [context]. For example if edge is ScrollableEdge.leading + /// and a ClampOverscrolls ancestor exists that specified ScrollableEdge.trailing, + /// then this widget would clamp both scrollable edges. + /// + /// The [context], [edge] and [child] arguments must not be null. + factory ClampOverscrolls.inherit({ + Key key, + @required BuildContext context, + @required ScrollableEdge edge: ScrollableEdge.none, + @required Widget child + }) { + assert(context != null); + assert(edge != null); + assert(child != null); - /// Whether a [Scrollable] widget within the given context should overscroll. - static bool of(BuildContext context) { - final ClampOverscrolls result = context.inheritFromWidgetOfExactType(ClampOverscrolls); - return result?.value ?? false; + // The child's clamped edge is the union of the given edge and the + // parent's clamped edge. + ScrollableEdge parentEdge = ClampOverscrolls.of(context)?.edge ?? ScrollableEdge.none; + ScrollableEdge childEdge = edge; + switch (parentEdge) { + case ScrollableEdge.leading: + if (edge == ScrollableEdge.trailing || edge == ScrollableEdge.both) + childEdge = ScrollableEdge.both; + break; + case ScrollableEdge.trailing: + if (edge == ScrollableEdge.leading || edge == ScrollableEdge.both) + childEdge = ScrollableEdge.both; + break; + case ScrollableEdge.both: + childEdge = ScrollableEdge.both; + break; + case ScrollableEdge.none: + break; + } + + return new ClampOverscrolls( + key: key, + edge: childEdge, + child: child + ); } - /// If ClampOverscrolls is true, clamps the ScrollableState's scrollOffset to the - /// [ScrollBehavior] minimum and maximum values and then constructs the viewport - /// with the clamped scrollOffset. ClampOverscrolls is reset to false for viewport + /// Defines when viewport scrollOffsets are clamped in terms of the scrollDirection. + /// If edge is `leading` the viewport's scrollOffset will be clamped at its minimum + /// value (often 0.0). If edge is `trailing` then the scrollOffset will be clamped + /// to its maximum value. If edge is `both` then both the leading and trailing + /// constraints are applied. + final ScrollableEdge edge; + + /// Return the [scrollable]'s scrollOffset clamped according to [edge]. + double clampScrollOffset(ScrollableState scrollable) { + final double scrollOffset = scrollable.scrollOffset; + final double minScrollOffset = scrollable.scrollBehavior.minScrollOffset; + final double maxScrollOffset = scrollable.scrollBehavior.maxScrollOffset; + switch (edge) { + case ScrollableEdge.both: + return scrollOffset.clamp(minScrollOffset, maxScrollOffset); + case ScrollableEdge.leading: + return scrollOffset.clamp(minScrollOffset, double.INFINITY); + case ScrollableEdge.trailing: + return scrollOffset.clamp(double.NEGATIVE_INFINITY, maxScrollOffset); + case ScrollableEdge.none: + return scrollOffset; + } + return scrollOffset; + } + + /// The closest instance of this class that encloses the given context. + static ClampOverscrolls of(BuildContext context) { + return context.inheritFromWidgetOfExactType(ClampOverscrolls); + } + + /// Clamps the new viewport's scroll offset according to the value of + /// `ClampOverscrolls.of(context).edge`. + /// + /// The clamped overscroll edge is reset to [ScrollableEdge.none] for the viewport's /// descendants. /// /// This utility function is typically used by [Scrollable.builder] callbacks. @@ -54,22 +118,23 @@ class ClampOverscrolls extends InheritedWidget { // by the container and content size. But we don't know those until we // layout the viewport, which happens after build phase. We need to rethink // this. - final bool clampOverscrolls = ClampOverscrolls.of(context); - final double clampedScrollOffset = clampOverscrolls - ? state.scrollOffset.clamp(state.scrollBehavior.minScrollOffset, state.scrollBehavior.maxScrollOffset) - : state.scrollOffset; + final ClampOverscrolls clampOverscrolls = ClampOverscrolls.of(context); + if (clampOverscrolls == null) + return builder(context, state, state.scrollOffset); + + final double clampedScrollOffset = clampOverscrolls.clampScrollOffset(state); Widget viewport = builder(context, state, clampedScrollOffset); - if (clampOverscrolls) - viewport = new ClampOverscrolls(value: false, child: viewport); + if (clampOverscrolls.edge != ScrollableEdge.none) + viewport = new ClampOverscrolls(edge: ScrollableEdge.none, child: viewport); return viewport; } @override - bool updateShouldNotify(ClampOverscrolls old) => value != old.value; + bool updateShouldNotify(ClampOverscrolls old) => edge != old.edge; @override void debugFillDescription(List description) { super.debugFillDescription(description); - description.add('value: $value'); + description.add('edge: $edge'); } } diff --git a/packages/flutter/lib/src/widgets/scroll_configuration.dart b/packages/flutter/lib/src/widgets/scroll_configuration.dart index dda1ca39b57..029ed0710be 100644 --- a/packages/flutter/lib/src/widgets/scroll_configuration.dart +++ b/packages/flutter/lib/src/widgets/scroll_configuration.dart @@ -1,4 +1,4 @@ -// Copyright 2016 The Chromium Authors. All rights reserved. +/// Copyright 2016 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. @@ -28,7 +28,7 @@ abstract class ScrollConfigurationDelegate { /// Scrollable they create. It can be used to add widgets that wrap the /// Scrollable, like scrollbars or overscroll indicators. By default the /// [scrollWidget] parameter is returned unchanged. - Widget wrapScrollWidget(Widget scrollWidget) => scrollWidget; + Widget wrapScrollWidget(BuildContext context, Widget scrollWidget) => scrollWidget; /// Overrides should return true if this ScrollConfigurationDelegate differs /// from the provided old delegate in a way that requires rebuilding its @@ -87,7 +87,7 @@ class ScrollConfiguration extends InheritedWidget { /// A utility function that calls [ScrollConfigurationDelegate.wrapScrollWidget]. static Widget wrap(BuildContext context, Widget scrollWidget) { - return ScrollConfiguration.of(context).wrapScrollWidget(scrollWidget); + return ScrollConfiguration.of(context).wrapScrollWidget(context, scrollWidget); } @override diff --git a/packages/flutter/lib/src/widgets/scrollable.dart b/packages/flutter/lib/src/widgets/scrollable.dart index 8d55e71945e..5267676839c 100644 --- a/packages/flutter/lib/src/widgets/scrollable.dart +++ b/packages/flutter/lib/src/widgets/scrollable.dart @@ -19,6 +19,24 @@ import 'page_storage.dart'; import 'scroll_behavior.dart'; import 'scroll_configuration.dart'; +/// Identifies one or both limits of a [Scrollable] in terms of its scrollDirection. +enum ScrollableEdge { + /// The top and bottom of the scrollable if its scrollDirection is vertical + /// or the left and right if its scrollDirection is horizontal. + both, + + /// Only the top of the scrollable if its scrollDirection is vertical, + /// or only the left if its scrollDirection is horizontal. + leading, + + /// Only the bottom of the scrollable if its scroll-direction is vertical, + /// or only the right if its scrollDirection is horizontal. + trailing, + + /// The overscroll indicator should not appear at all. + none, +} + /// The accuracy to which scrolling is computed. final Tolerance kPixelScrollTolerance = new Tolerance( // TODO(ianh): Handle the case of the device pixel ratio changing. diff --git a/packages/flutter/test/widget/clamp_overscrolls_test.dart b/packages/flutter/test/widget/clamp_overscrolls_test.dart new file mode 100644 index 00000000000..ff3c7db343d --- /dev/null +++ b/packages/flutter/test/widget/clamp_overscrolls_test.dart @@ -0,0 +1,73 @@ +// Copyright 2016 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_test/flutter_test.dart'; +import 'package:flutter/widgets.dart'; + +// Assuming that the test container is 800x600. The height of the +// viewport's contents is 650.0, the top and bottom text children +// are 100 pixels high and top/left edge of both widgets are visible. +// The top of the bottom widget is at 550 (the top of the top widget +// is at 0). The top of the bottom widget is 500 when it has been +// scrolled completely into view. +Widget buildFrame(ScrollableEdge clampedEdge) { + return new ClampOverscrolls( + edge: clampedEdge, + child: new ScrollableViewport( + scrollableKey: new UniqueKey(), + child: new SizedBox( + height: 650.0, + child: new Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + new SizedBox(height: 100.0, child: new Text('top')), + new Flexible(child: new Container()), + new SizedBox(height: 100.0, child: new Text('bottom')), + ] + ) + ) + ) + ); +} + +void main() { + testWidgets('ClampOverscrolls', (WidgetTester tester) async { + + // Scroll the target text widget by offset and then return its origin + // in global coordinates. + Future locationAfterScroll(String target, Offset offset) async { + await tester.scrollAt(tester.getTopLeft(find.text(target)), offset); + await tester.pump(); + final RenderBox textBox = tester.renderObject(find.text(target)); + final Point widgetOrigin = textBox.localToGlobal(Point.origin); + await tester.pump(const Duration(seconds: 1)); // Allow overscroll to settle + return new Future.value(widgetOrigin); + } + + await tester.pumpWidget(buildFrame(ScrollableEdge.none)); + Point origin = await locationAfterScroll('top', const Offset(0.0, 400.0)); + expect(origin.y, greaterThan(0.0)); + origin = await locationAfterScroll('bottom', const Offset(0.0, -400.0)); + expect(origin.y, lessThan(500.0)); + + + await tester.pumpWidget(buildFrame(ScrollableEdge.both)); + origin = await locationAfterScroll('top', const Offset(0.0, 400.0)); + expect(origin.y, equals(0.0)); + origin = await locationAfterScroll('bottom', const Offset(0.0, -400.0)); + expect(origin.y, equals(500.0)); + + await tester.pumpWidget(buildFrame(ScrollableEdge.leading)); + origin = await locationAfterScroll('top', const Offset(0.0, 400.0)); + expect(origin.y, equals(0.0)); + origin = await locationAfterScroll('bottom', const Offset(0.0, -400.0)); + expect(origin.y, lessThan(500.0)); + + await tester.pumpWidget(buildFrame(ScrollableEdge.trailing)); + origin = await locationAfterScroll('top', const Offset(0.0, 400.0)); + expect(origin.y, greaterThan(0.0)); + origin = await locationAfterScroll('bottom', const Offset(0.0, -400.0)); + expect(origin.y, equals(500.0)); + }); +}