Skip to content

Commit 7bf73e8

Browse files
authored
fix(ScrollAction): unsafe non-null assertion (#157855)
This conditional most likely was meant to use `||` instead of `&&` operator to check if `notificationContext` is null OR if no Scrollable was found for this `notificationContext`. Using the AND operator instead causes the null assertion to fail when notificationContext is null, while also causing an assertion failure when notificationContext is not null, but no Scrollable is found for it. While at it, let's also deduplicate the Scrollable.maybeOf call. This fix is purely speculative, I didn't encounter this error, but rather was reading the ScrollAction's source and stumbled upon this. ~~With that said I wasn't actually able to trigger this error condition, thus why there are no tests included with this PR, if anyone has ideas on how this could be triggered, I'll be happy to include such test cases.~~ fixes #158063 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 parent b13770f commit 7bf73e8

File tree

2 files changed

+92
-6
lines changed

2 files changed

+92
-6
lines changed

packages/flutter/lib/src/widgets/scrollable_helpers.dart

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -460,17 +460,18 @@ class ScrollAction extends ContextAction<ScrollIntent> {
460460
return true;
461461
}());
462462

463-
if (primaryScrollController.position.context.notificationContext == null
464-
&& Scrollable.maybeOf(primaryScrollController.position.context.notificationContext!) == null) {
463+
final BuildContext? notificationContext = primaryScrollController.position.context.notificationContext;
464+
if (notificationContext != null) {
465+
state = Scrollable.maybeOf(notificationContext);
466+
}
467+
if (state == null) {
465468
return;
466469
}
467-
state = Scrollable.maybeOf(primaryScrollController.position.context.notificationContext!);
468470
}
469-
assert(state != null, '$ScrollAction was invoked on a context that has no scrollable parent');
470-
assert(state!.position.hasPixels, 'Scrollable must be laid out before it can be scrolled via a ScrollAction');
471+
assert(state.position.hasPixels, 'Scrollable must be laid out before it can be scrolled via a ScrollAction');
471472

472473
// Don't do anything if the user isn't allowed to scroll.
473-
if (state!.resolvedPhysics != null && !state.resolvedPhysics!.shouldAcceptUserOffset(state.position)) {
474+
if (state.resolvedPhysics != null && !state.resolvedPhysics!.shouldAcceptUserOffset(state.position)) {
474475
return;
475476
}
476477
final double increment = getDirectionalIncrement(state, intent);

packages/flutter/test/widgets/scrollable_helpers_test.dart

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,29 @@
44

55
import 'package:flutter/foundation.dart';
66
import 'package:flutter/material.dart';
7+
import 'package:flutter/rendering.dart';
78
import 'package:flutter/services.dart';
89
import 'package:flutter_test/flutter_test.dart';
910

1011
final LogicalKeyboardKey modifierKey = defaultTargetPlatform == TargetPlatform.macOS
1112
? LogicalKeyboardKey.metaLeft
1213
: LogicalKeyboardKey.controlLeft;
1314

15+
class _NoNotificationContextScrollable extends Scrollable {
16+
const _NoNotificationContextScrollable({
17+
super.controller,
18+
required super.viewportBuilder,
19+
});
20+
21+
@override
22+
ScrollableState createState() => _NoNotificationContextScrollableState();
23+
}
24+
25+
class _NoNotificationContextScrollableState extends ScrollableState {
26+
@override
27+
BuildContext? get notificationContext => null;
28+
}
29+
1430
void main() {
1531
group('ScrollableDetails', (){
1632
test('copyWith / == / hashCode', () {
@@ -590,4 +606,73 @@ void main() {
590606
expect(find.text('The cape as red as blood'), findsOneWidget);
591607
expect(find.text('The hair as yellow as corn'), findsOneWidget);
592608
});
609+
610+
// Regression test for https://github.com/flutter/flutter/issues/158063.
611+
testWidgets('Invoking a ScrollAction when notificationContext is null does not cause an exception.', (WidgetTester tester) async {
612+
const List<LogicalKeyboardKey> keysWithModifier = <LogicalKeyboardKey>[
613+
LogicalKeyboardKey.arrowDown, LogicalKeyboardKey.arrowUp,
614+
];
615+
const List<LogicalKeyboardKey> keys = <LogicalKeyboardKey>[
616+
...keysWithModifier,
617+
LogicalKeyboardKey.pageDown, LogicalKeyboardKey.pageUp,
618+
];
619+
final ScrollController controller = ScrollController();
620+
addTearDown(controller.dispose);
621+
await tester.pumpWidget(
622+
MaterialApp(
623+
theme: ThemeData(platform: TargetPlatform.fuchsia),
624+
home: PrimaryScrollController(
625+
controller: controller,
626+
child: Focus(
627+
autofocus: true,
628+
child: _NoNotificationContextScrollable(
629+
controller: controller,
630+
viewportBuilder: (BuildContext context, ViewportOffset offset) => Viewport(
631+
offset: offset,
632+
slivers: List<Widget>.generate(
633+
20,
634+
(int index) => SliverToBoxAdapter(
635+
child: SizedBox(
636+
key: ValueKey<String>('Box $index'),
637+
height: 50.0,
638+
),
639+
),
640+
),
641+
),
642+
),
643+
),
644+
),
645+
),
646+
);
647+
648+
// Verify the initial scroll offset.
649+
await tester.pumpAndSettle();
650+
expect(controller.position.pixels, equals(0.0));
651+
expect(
652+
tester.getRect(find.byKey(const ValueKey<String>('Box 0'), skipOffstage: false)),
653+
equals(const Rect.fromLTRB(0.0, 0.0, 800.0, 50.0)),
654+
);
655+
656+
for (final LogicalKeyboardKey key in keys) {
657+
// The default web shortcuts do not use a modifier key for ScrollActions.
658+
if (!kIsWeb && keysWithModifier.contains(key)) {
659+
await tester.sendKeyDownEvent(modifierKey);
660+
}
661+
662+
await tester.sendKeyEvent(key);
663+
expect(tester.takeException(), isNull);
664+
665+
if (!kIsWeb && keysWithModifier.contains(key)) {
666+
await tester.sendKeyUpEvent(modifierKey);
667+
}
668+
669+
// No scrollable is found, so the scroll position should not change.
670+
await tester.pumpAndSettle();
671+
expect(controller.position.pixels, equals(0.0));
672+
expect(
673+
tester.getRect(find.byKey(const ValueKey<String>('Box 0'), skipOffstage: false)),
674+
equals(const Rect.fromLTRB(0.0, 0.0, 800.0, 50.0)),
675+
);
676+
}
677+
}, variant: KeySimulatorTransitModeVariant.all());
593678
}

0 commit comments

Comments
 (0)