Skip to content

Commit

Permalink
Make scrollbar assertions less aggressive (flutter#84809)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Piinks @davidmartos96
Piinks authored and davidmartos96 committed Jun 25, 2021
1 parent 0c15668 commit 62ba020
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 92 deletions.
90 changes: 81 additions & 9 deletions packages/flutter/lib/src/widgets/scrollbar.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -600,14 +600,85 @@ class ScrollbarPainter extends ChangeNotifier implements CustomPainter {
/// along the track exclusive of the thumb will trigger a
/// [ScrollIncrementType.page] based on the relative position to the thumb.
///
/// When using the [PrimaryScrollController], it should not be attached to more
/// When using the [PrimaryScrollController], it must not be attached to more
/// than one [ScrollPosition]. [ScrollView]s that have not been provided a
/// [ScrollController] and have a [ScrollView.scrollDirection] of
/// [Axis.vertical] will automatically attach their ScrollPosition to the
/// PrimaryScrollController. Provide a unique ScrollController to each
/// [Scrollable] in this case to prevent having multiple ScrollPositions
/// attached to the PrimaryScrollController.
///
/// {@tool dartpad --template=stateful_widget_scaffold_center}
/// This sample shows an app with two scrollables in the same route. Since by
/// default, there is one [PrimaryScrollController] per route, and they both have a
/// scroll direction of [Axis.vertical], they would both try to attach to that
/// controller. The [Scrollbar] cannot support multiple positions attached to
/// the same controller, so one [ListView], and its [Scrollbar] have been
/// provided a unique [ScrollController].
///
/// Alternatively, a new PrimaryScrollController could be created above one of
/// the [ListView]s.
///
/// ```dart
/// final ScrollController _firstController = ScrollController();
///
/// @override
/// Widget build(BuildContext context) {
/// return LayoutBuilder(
/// builder: (BuildContext context, BoxConstraints constraints) {
/// return Row(
/// children: <Widget>[
/// SizedBox(
/// width: constraints.maxWidth / 2,
/// // Only one scroll position can be attached to the
/// // PrimaryScrollController if using Scrollbars. Providing a
/// // unique scroll controller to this scroll view prevents it
/// // from attaching to the PrimaryScrollController.
/// child: Scrollbar(
/// isAlwaysShown: true,
/// controller: _firstController,
/// child: ListView.builder(
/// controller: _firstController,
/// itemCount: 100,
/// itemBuilder: (BuildContext context, int index) {
/// return Padding(
/// padding: const EdgeInsets.all(8.0),
/// child: Text('Scrollable 1 : Index $index'),
/// );
/// }
/// ),
/// )
/// ),
/// SizedBox(
/// width: constraints.maxWidth / 2,
/// // This vertical scroll view has not been provided a
/// // ScrollController, so it is using the
/// // PrimaryScrollController.
/// child: Scrollbar(
/// isAlwaysShown: true,
/// child: ListView.builder(
/// itemCount: 100,
/// itemBuilder: (BuildContext context, int index) {
/// return Container(
/// height: 50,
/// color: index.isEven ? Colors.amberAccent : Colors.blueAccent,
/// child: Padding(
/// padding: const EdgeInsets.all(8.0),
/// child: Text('Scrollable 2 : Index $index'),
/// )
/// );
/// }
/// ),
/// )
/// ),
/// ],
/// );
/// }
/// );
/// }
/// ```
/// {@end-tool}
///
/// ### Automatic Scrollbars on Desktop Platforms
///
/// Scrollbars are added to most [Scrollable] widgets by default on
Expand Down Expand Up @@ -1012,7 +1083,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
// Wait one frame and cause an empty scroll event. This allows the
// thumb to show immediately when isAlwaysShown is true. A scroll
// event is required in order to paint the thumb.
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
scrollController!.position.didUpdateScrollPositionBy(0);
}
});
Expand All @@ -1027,11 +1098,11 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
} else if (scrollController != null && enableGestures) {
// Interactive scrollbars need to be properly configured. If it is visible
// for interaction, ensure we are set up properly.
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
}
}

void _debugCheckHasValidScrollPosition() {
bool _debugCheckHasValidScrollPosition() {
final ScrollController? scrollController = widget.controller ?? PrimaryScrollController.of(context);
final bool tryPrimary = widget.controller == null;
final String controllerForError = tryPrimary
Expand Down Expand Up @@ -1110,6 +1181,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
}
return true;
}());
return true;
}

/// This method is responsible for configuring the [scrollbarPainter]
Expand Down Expand Up @@ -1182,7 +1254,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
@protected
@mustCallSuper
void handleThumbPress() {
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
if (getScrollbarDirection() == null) {
return;
}
Expand All @@ -1195,7 +1267,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
@protected
@mustCallSuper
void handleThumbPressStart(Offset localPosition) {
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
_currentController = widget.controller ?? PrimaryScrollController.of(context);
final Axis? direction = getScrollbarDirection();
if (direction == null) {
Expand All @@ -1219,7 +1291,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
@protected
@mustCallSuper
void handleThumbPressUpdate(Offset localPosition) {
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
final Axis? direction = getScrollbarDirection();
if (direction == null) {
return;
Expand All @@ -1240,7 +1312,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv
@protected
@mustCallSuper
void handleThumbPressEnd(Offset localPosition, Velocity velocity) {
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
final Axis? direction = getScrollbarDirection();
if (direction == null) {
return;
Expand All @@ -1252,7 +1324,7 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> with TickerProv

void _handleTrackTapDown(TapDownDetails details) {
// The Scrollbar should page towards the position of the tap on the track.
_debugCheckHasValidScrollPosition();
assert(_debugCheckHasValidScrollPosition());
_currentController = widget.controller ?? PrimaryScrollController.of(context);

double scrollIncrement;
Expand Down
88 changes: 48 additions & 40 deletions packages/flutter/test/cupertino/scrollbar_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,55 +266,57 @@ void main() {

testWidgets('When isAlwaysShown is true, must pass a controller or find PrimaryScrollController',
(WidgetTester tester) async {
Widget viewWithScroll() {
return const Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: MediaQueryData(),
child: CupertinoScrollbar(
isAlwaysShown: true,
child: SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
Widget viewWithScroll() {
return const Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: MediaQueryData(),
child: CupertinoScrollbar(
isAlwaysShown: true,
child: SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
),
),
),
),
),
);
}
);
}

await tester.pumpWidget(viewWithScroll());
final dynamic exception = tester.takeException();
expect(exception, isAssertionError);
});
await tester.pumpWidget(viewWithScroll());
final AssertionError exception = tester.takeException() as AssertionError;
expect(exception, isAssertionError);
},
);

testWidgets('When isAlwaysShown is true, must pass a controller or find PrimarySCrollController that is attached to a scroll view',
(WidgetTester tester) async {
final ScrollController controller = ScrollController();
Widget viewWithScroll() {
return Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: const MediaQueryData(),
child: CupertinoScrollbar(
controller: controller,
isAlwaysShown: true,
child: const SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
final ScrollController controller = ScrollController();
Widget viewWithScroll() {
return Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: const MediaQueryData(),
child: CupertinoScrollbar(
controller: controller,
isAlwaysShown: true,
child: const SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
),
),
),
),
),
);
}
);
}

await tester.pumpWidget(viewWithScroll());
final dynamic exception = tester.takeException();
expect(exception, isAssertionError);
});
await tester.pumpWidget(viewWithScroll());
final AssertionError exception = tester.takeException() as AssertionError;
expect(exception, isAssertionError);
},
);

testWidgets('On first render with isAlwaysShown: true, the thumb shows with PrimaryScrollController', (WidgetTester tester) async {
final ScrollController controller = ScrollController();
Expand Down Expand Up @@ -801,10 +803,16 @@ void main() {

await tester.pumpAndSettle();

final dynamic exception = tester.takeException();
AssertionError? exception = tester.takeException() as AssertionError?;
// The scrollbar is not visible and cannot be interacted with, so no assertion.
expect(exception, isNull);
// Scroll to trigger the scrollbar to come into view.
final TestGesture gesture = await tester.startGesture(tester.getCenter(find.byType(SingleChildScrollView)));
await gesture.moveBy(const Offset(0.0, -20.0));
exception = tester.takeException() as AssertionError;
expect(exception, isAssertionError);
expect(
(exception as AssertionError).message,
exception.message,
contains("The Scrollbar's ScrollController has no ScrollPosition attached."),
);
});
Expand Down
78 changes: 40 additions & 38 deletions packages/flutter/test/material/scrollbar_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ void main() {
of: find.byType(Scrollbar),
matching: find.byType(CustomPaint),
).first);
final dynamic scrollPainter = custom.foregroundPainter;
final ScrollbarPainter? scrollPainter = custom.foregroundPainter as ScrollbarPainter?;
// Dragging makes the scrollbar first appear.
await tester.drag(find.text('0'), const Offset(0.0, -10.0));
await tester.pump(const Duration(milliseconds: 200));
Expand All @@ -141,7 +141,7 @@ void main() {
viewportDimension: 100.0,
axisDirection: AxisDirection.down,
);
scrollPainter.update(metrics, AxisDirection.down);
scrollPainter!.update(metrics, AxisDirection.down);

final TestCanvas canvas = TestCanvas();
scrollPainter.paint(canvas, const Size(10.0, 100.0));
Expand All @@ -152,53 +152,55 @@ void main() {

testWidgets('When isAlwaysShown is true, must pass a controller or find PrimaryScrollController',
(WidgetTester tester) async {
Widget viewWithScroll() {
return _buildBoilerplate(
child: Theme(
data: ThemeData(),
child: const Scrollbar(
isAlwaysShown: true,
child: SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
Widget viewWithScroll() {
return _buildBoilerplate(
child: Theme(
data: ThemeData(),
child: const Scrollbar(
isAlwaysShown: true,
child: SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
),
),
),
),
),
);
}
);
}

await tester.pumpWidget(viewWithScroll());
final dynamic exception = tester.takeException();
expect(exception, isAssertionError);
});
await tester.pumpWidget(viewWithScroll());
final AssertionError exception = tester.takeException() as AssertionError;
expect(exception, isAssertionError);
},
);

testWidgets('When isAlwaysShown is true, must pass a controller that is attached to a scroll view or find PrimaryScrollController',
(WidgetTester tester) async {
final ScrollController controller = ScrollController();
Widget viewWithScroll() {
return _buildBoilerplate(
child: Theme(
data: ThemeData(),
child: Scrollbar(
isAlwaysShown: true,
controller: controller,
child: const SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
final ScrollController controller = ScrollController();
Widget viewWithScroll() {
return _buildBoilerplate(
child: Theme(
data: ThemeData(),
child: Scrollbar(
isAlwaysShown: true,
controller: controller,
child: const SingleChildScrollView(
child: SizedBox(
width: 4000.0,
height: 4000.0,
),
),
),
),
),
);
}
);
}

await tester.pumpWidget(viewWithScroll());
final dynamic exception = tester.takeException();
expect(exception, isAssertionError);
});
await tester.pumpWidget(viewWithScroll());
final AssertionError exception = tester.takeException() as AssertionError;
expect(exception, isAssertionError);
},
);

testWidgets('On first render with isAlwaysShown: true, the thumb shows',
(WidgetTester tester) async {
Expand Down
Loading

0 comments on commit 62ba020

Please sign in to comment.