Both ends of a scroll, on the same controller
Every wheel event over any scrollable threw "The Scrollbar's ScrollController has no ScrollPosition attached", repeatedly. `Scrollbar` had no controller, so it asked the PrimaryScrollController; `SingleChildScrollView` had none either, and is only primary on mobile — on a desktop it makes its own and attaches nowhere. The two ends were looking at different controllers. Every `scroll` node has carried a `scrollKey` since the port and the renderer was ignoring it, so the fix is to hold one ScrollController per key and hand it to both. Per key because two positions must never share one, and the chat screen's is per room: switching rooms is a different backlog at a different offset. The layout suite never caught this because the failure arrives on the first wheel event rather than the first frame — so these tests turn a wheel. They also have to say what platform they are: a widget test runs as Android by default, where the view *is* primary and attaches to the controller the scrollbar is watching, and the test passes on the broken renderer. `TargetPlatformVariant.desktop()` rather than an override the test resets itself, which the framework rejects as a leaked debug variable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5594a62 parent: a4be36c modified
flutter/lib/nim_renderer.dart +21 -1 | @@ -47,6 +47,19 @@ class _NimAppState extends State<NimApp> { | ||
| 47 | 47 | // leaks, and this tree is rebuilt on every keystroke. |
| 48 | 48 | final _linkTaps = <String, TapGestureRecognizer>{}; |
| 49 | 49 | |
| 50 | + // One ScrollController per `scrollKey`, for the same reason the entries | |
| 51 | + // have one per key — and for a second reason of its own. A `Scrollbar` with | |
| 52 | + // no controller of its own asks the PrimaryScrollController, and a | |
| 53 | + // SingleChildScrollView is only primary on mobile: on a desktop the two | |
| 54 | + // ends looked at different controllers, so the first wheel event over any | |
| 55 | + // scroll threw "The Scrollbar's ScrollController has no ScrollPosition | |
| 56 | + // attached" and went on throwing it. Naming the controller joins them. | |
| 57 | + // | |
| 58 | + // Two positions must never share one, which is what makes `scrollKey` a | |
| 59 | + // requirement rather than a nicety — the chat screen's is per room, since | |
| 60 | + // switching rooms is a different backlog at a different offset. | |
| 61 | + final _scrollers = <String, ScrollController>{}; | |
| 62 | + | |
| 50 | 63 | @override |
| 51 | 64 | void initState() { |
| 52 | 65 | super.initState(); |
| @@ -80,6 +93,9 @@ class _NimAppState extends State<NimApp> { | ||
| 80 | 93 | for (final r in _linkTaps.values) { |
| 81 | 94 | r.dispose(); |
| 82 | 95 | } |
| 96 | + for (final c in _scrollers.values) { | |
| 97 | + c.dispose(); | |
| 98 | + } | |
| 83 | 99 | super.dispose(); |
| 84 | 100 | } |
| 85 | 101 | |
| @@ -598,14 +614,18 @@ class _NimAppState extends State<NimApp> { | ||
| 598 | 614 | |
| 599 | 615 | case 'scroll': |
| 600 | 616 | { |
| 617 | + // Both ends of the same scroll, named so they are the same one. | |
| 618 | + final c = _scrollers.putIfAbsent( | |
| 619 | + n.prop('scrollKey', 'scroll'), ScrollController.new); | |
| 601 | 620 | Widget body = SingleChildScrollView( |
| 621 | + controller: c, | |
| 602 | 622 | // The backlog reads from the bottom; a settings list from the top. |
| 603 | 623 | reverse: n.prop('stickToBottom', false), |
| 604 | 624 | child: Column( |
| 605 | 625 | crossAxisAlignment: CrossAxisAlignment.start, |
| 606 | 626 | children: _spaced(kids, spacing, vertical: true)), |
| 607 | 627 | ); |
| 608 | - body = Scrollbar(child: body); | |
| 628 | + body = Scrollbar(controller: c, child: body); | |
| 609 | 629 | // A scroll takes what the column has left. Outside a Flex there is |
| 610 | 630 | // nothing to take, and the tree is malformed — `_strandedScroll` is |
| 611 | 631 | // a visible size rather than a correct one, so the layout tests see |
| @@ -47,6 +47,19 @@ class _NimAppState extends State<NimApp> { | |||
| 47 | // leaks, and this tree is rebuilt on every keystroke. | 47 | // leaks, and this tree is rebuilt on every keystroke. |
| 48 | final _linkTaps = <String, TapGestureRecognizer>{}; | 48 | final _linkTaps = <String, TapGestureRecognizer>{}; |
| 49 | 49 | ||
| 50 | + // One ScrollController per `scrollKey`, for the same reason the entries | ||
| 51 | + // have one per key — and for a second reason of its own. A `Scrollbar` with | ||
| 52 | + // no controller of its own asks the PrimaryScrollController, and a | ||
| 53 | + // SingleChildScrollView is only primary on mobile: on a desktop the two | ||
| 54 | + // ends looked at different controllers, so the first wheel event over any | ||
| 55 | + // scroll threw "The Scrollbar's ScrollController has no ScrollPosition | ||
| 56 | + // attached" and went on throwing it. Naming the controller joins them. | ||
| 57 | + // | ||
| 58 | + // Two positions must never share one, which is what makes `scrollKey` a | ||
| 59 | + // requirement rather than a nicety — the chat screen's is per room, since | ||
| 60 | + // switching rooms is a different backlog at a different offset. | ||
| 61 | + final _scrollers = <String, ScrollController>{}; | ||
| 62 | + | ||
| 50 | @override | 63 | @override |
| 51 | void initState() { | 64 | void initState() { |
| 52 | super.initState(); | 65 | super.initState(); |
| @@ -80,6 +93,9 @@ class _NimAppState extends State<NimApp> { | |||
| 80 | for (final r in _linkTaps.values) { | 93 | for (final r in _linkTaps.values) { |
| 81 | r.dispose(); | 94 | r.dispose(); |
| 82 | } | 95 | } |
| 96 | + for (final c in _scrollers.values) { | ||
| 97 | + c.dispose(); | ||
| 98 | + } | ||
| 83 | super.dispose(); | 99 | super.dispose(); |
| 84 | } | 100 | } |
| 85 | 101 | ||
| @@ -598,14 +614,18 @@ class _NimAppState extends State<NimApp> { | |||
| 598 | 614 | ||
| 599 | case 'scroll': | 615 | case 'scroll': |
| 600 | { | 616 | { |
| 617 | + // Both ends of the same scroll, named so they are the same one. | ||
| 618 | + final c = _scrollers.putIfAbsent( | ||
| 619 | + n.prop('scrollKey', 'scroll'), ScrollController.new); | ||
| 601 | Widget body = SingleChildScrollView( | 620 | Widget body = SingleChildScrollView( |
| 621 | + controller: c, | ||
| 602 | // The backlog reads from the bottom; a settings list from the top. | 622 | // The backlog reads from the bottom; a settings list from the top. |
| 603 | reverse: n.prop('stickToBottom', false), | 623 | reverse: n.prop('stickToBottom', false), |
| 604 | child: Column( | 624 | child: Column( |
| 605 | crossAxisAlignment: CrossAxisAlignment.start, | 625 | crossAxisAlignment: CrossAxisAlignment.start, |
| 606 | children: _spaced(kids, spacing, vertical: true)), | 626 | children: _spaced(kids, spacing, vertical: true)), |
| 607 | ); | 627 | ); |
| 608 | - body = Scrollbar(child: body); | 628 | + body = Scrollbar(controller: c, child: body); |
| 609 | // A scroll takes what the column has left. Outside a Flex there is | 629 | // A scroll takes what the column has left. Outside a Flex there is |
| 610 | // nothing to take, and the tree is malformed — `_strandedScroll` is | 630 | // nothing to take, and the tree is malformed — `_strandedScroll` is |
| 611 | // a visible size rather than a correct one, so the layout tests see | 631 | // a visible size rather than a correct one, so the layout tests see |
modified
flutter/test/nim_layout_test.dart +36 -0 | @@ -13,6 +13,7 @@ | ||
| 13 | 13 | /// just test layout |
| 14 | 14 | library; |
| 15 | 15 | |
| 16 | +import 'package:flutter/gestures.dart'; | |
| 16 | 17 | import 'package:flutter/material.dart'; |
| 17 | 18 | import 'package:flutter_test/flutter_test.dart'; |
| 18 | 19 | import 'package:frq_core/frq_core.dart' as core; |
| @@ -179,6 +180,41 @@ void main() { | ||
| 179 | 180 | } |
| 180 | 181 | }); |
| 181 | 182 | |
| 183 | + group('the wheel', () { | |
| 184 | + // Laying a screen out was never enough to catch this one: the failure | |
| 185 | + // arrives on the first wheel event, not on the first frame. A `Scrollbar` | |
| 186 | + // with no controller asks the PrimaryScrollController, and a | |
| 187 | + // SingleChildScrollView is only primary on mobile — so on a desktop the | |
| 188 | + // scrollbar and the view held different controllers, and every scroll | |
| 189 | + // threw "has no ScrollPosition attached". | |
| 190 | + Future<void> wheelOver(WidgetTester tester, Finder target) async { | |
| 191 | + final pointer = TestPointer(1, PointerDeviceKind.mouse); | |
| 192 | + pointer.hover(tester.getCenter(target)); | |
| 193 | + await tester.sendEventToBinding(pointer.scroll(const Offset(0, 60))); | |
| 194 | + await tester.pump(); | |
| 195 | + } | |
| 196 | + | |
| 197 | + for (final screen in ['chat', 'chats', 'discover', 'settings']) { | |
| 198 | + // The platform has to be said out loud. A widget test runs as Android | |
| 199 | + // by default, where a SingleChildScrollView *is* primary and attaches | |
| 200 | + // to the very controller the scrollbar is looking at — so this passed | |
| 201 | + // on the broken renderer while the desktop app threw on every wheel | |
| 202 | + // event. `variant` rather than an override this test resets itself, | |
| 203 | + // which the framework catches as a leaked debug variable. | |
| 204 | + testWidgets('$screen scrolls without losing its scrollbar', | |
| 205 | + variant: TargetPlatformVariant.desktop(), (tester) async { | |
| 206 | + core.demoUi(); | |
| 207 | + if (screen != 'chat') core.dispatch('screen.$screen'); | |
| 208 | + await layOut(tester, sizes['desktop']!); | |
| 209 | + expectLaidOut(tester, '$screen before scrolling'); | |
| 210 | + final bars = find.byType(Scrollbar); | |
| 211 | + expect(bars, findsWidgets, reason: '$screen has nothing to scroll'); | |
| 212 | + await wheelOver(tester, bars.first); | |
| 213 | + expectLaidOut(tester, '$screen on the wheel'); | |
| 214 | + }); | |
| 215 | + } | |
| 216 | + }); | |
| 217 | + | |
| 182 | 218 | group('emoji', () { |
| 183 | 219 | // ✏️ is U+270F plus a variation selector asking for emoji presentation, |
| 184 | 220 | // and DejaVu Sans claims U+270F — so ordinary fallback draws a monochrome |
| @@ -13,6 +13,7 @@ | |||
| 13 | /// just test layout | 13 | /// just test layout |
| 14 | library; | 14 | library; |
| 15 | 15 | ||
| 16 | +import 'package:flutter/gestures.dart'; | ||
| 16 | import 'package:flutter/material.dart'; | 17 | import 'package:flutter/material.dart'; |
| 17 | import 'package:flutter_test/flutter_test.dart'; | 18 | import 'package:flutter_test/flutter_test.dart'; |
| 18 | import 'package:frq_core/frq_core.dart' as core; | 19 | import 'package:frq_core/frq_core.dart' as core; |
| @@ -179,6 +180,41 @@ void main() { | |||
| 179 | } | 180 | } |
| 180 | }); | 181 | }); |
| 181 | 182 | ||
| 183 | + group('the wheel', () { | ||
| 184 | + // Laying a screen out was never enough to catch this one: the failure | ||
| 185 | + // arrives on the first wheel event, not on the first frame. A `Scrollbar` | ||
| 186 | + // with no controller asks the PrimaryScrollController, and a | ||
| 187 | + // SingleChildScrollView is only primary on mobile — so on a desktop the | ||
| 188 | + // scrollbar and the view held different controllers, and every scroll | ||
| 189 | + // threw "has no ScrollPosition attached". | ||
| 190 | + Future<void> wheelOver(WidgetTester tester, Finder target) async { | ||
| 191 | + final pointer = TestPointer(1, PointerDeviceKind.mouse); | ||
| 192 | + pointer.hover(tester.getCenter(target)); | ||
| 193 | + await tester.sendEventToBinding(pointer.scroll(const Offset(0, 60))); | ||
| 194 | + await tester.pump(); | ||
| 195 | + } | ||
| 196 | + | ||
| 197 | + for (final screen in ['chat', 'chats', 'discover', 'settings']) { | ||
| 198 | + // The platform has to be said out loud. A widget test runs as Android | ||
| 199 | + // by default, where a SingleChildScrollView *is* primary and attaches | ||
| 200 | + // to the very controller the scrollbar is looking at — so this passed | ||
| 201 | + // on the broken renderer while the desktop app threw on every wheel | ||
| 202 | + // event. `variant` rather than an override this test resets itself, | ||
| 203 | + // which the framework catches as a leaked debug variable. | ||
| 204 | + testWidgets('$screen scrolls without losing its scrollbar', | ||
| 205 | + variant: TargetPlatformVariant.desktop(), (tester) async { | ||
| 206 | + core.demoUi(); | ||
| 207 | + if (screen != 'chat') core.dispatch('screen.$screen'); | ||
| 208 | + await layOut(tester, sizes['desktop']!); | ||
| 209 | + expectLaidOut(tester, '$screen before scrolling'); | ||
| 210 | + final bars = find.byType(Scrollbar); | ||
| 211 | + expect(bars, findsWidgets, reason: '$screen has nothing to scroll'); | ||
| 212 | + await wheelOver(tester, bars.first); | ||
| 213 | + expectLaidOut(tester, '$screen on the wheel'); | ||
| 214 | + }); | ||
| 215 | + } | ||
| 216 | + }); | ||
| 217 | + | ||
| 182 | group('emoji', () { | 218 | group('emoji', () { |
| 183 | // ✏️ is U+270F plus a variation selector asking for emoji presentation, | 219 | // ✏️ is U+270F plus a variation selector asking for emoji presentation, |
| 184 | // and DejaVu Sans claims U+270F — so ordinary fallback draws a monochrome | 220 | // and DejaVu Sans claims U+270F — so ordinary fallback draws a monochrome |