Updating PrimaryScrollController for Desktop #102099

Piinks · 2022-04-18T19:07:52Z

This updates the PrimaryScrollController to not automatically attach to scroll views on desktop as it does on mobile platforms.
This behavior has caused a lot of issues for users developing desktop applications that have multiple parallel vertical ScrollViews.
This adds two parameters to PrimaryScrollController

automaticallyInheritForPlatforms
- specifies a set of TargetPlatforms that will engage automatic inheritance
scrollDirection
- This adds the ability to specify which axis the PrimaryScrollController should be inherited for.

Also adds static method shouldInherit, which uses the two properties above to determine if automatically inheriting the ScrollController is appropriate for the current configuration.

This design and other approaches is discussed at length in this design document.

Fixes #100264
Related (among others):

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Piinks · 2022-04-18T19:22:31Z

dev/integration_tests/flutter_gallery/lib/demo/colors_demo.dart

@@ -96,6 +96,7 @@ class PaletteTabView extends StatelessWidget {
    final TextStyle blackTextStyle = textTheme.bodyText2!.copyWith(color: Colors.black);
    return Scrollbar(
      child: ListView(
+        primary: true,


These integration tests are run on mobile and desktop platforms, so I have added primary: true so they all inherit the PrimaryScrollController as they did so automatically prior to this change.

Why is this important, though? Would anything break if we don't do this? Does this mean we are breaking users as well and they need to go in to make this change in their apps?

Only on desktop, which is the point of this change.

The automatic attachment of the PrimaryScrollController will no longer happen on desktop. Thee integration test expect that behavior across all of the platforms. The tests could be refactored for each platform, but that seemed outside of the scope for this change.

Without this, the test will fail when run configured to desktop, because it is no longer automatic.

Piinks · 2022-04-18T19:23:09Z

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

@@ -34,14 +44,18 @@ class PrimaryScrollController extends InheritedWidget {
  const PrimaryScrollController({
    super.key,
    required ScrollController this.controller,
+    this.autoForPlatforms = _kMobilePlatforms,


I do not like this name, but have not thought of something else yet.

+1 - brainstorming some other suggestions:

enabledPlatforms - can you do anything with this of running on a platform that's not part of this set?

inheritOnPlatforms

... naming is hard.

It is tricky because it can be inherited on any platform, this is specific to the automatic inheritance.

can you do anything with this of running on a platform that's not part of this set?

I am not sure I understand this question

goderbauer · 2022-04-26T16:22:32Z

dev/integration_tests/flutter_gallery/lib/demo/colors_demo.dart

@@ -96,6 +96,7 @@ class PaletteTabView extends StatelessWidget {
    final TextStyle blackTextStyle = textTheme.bodyText2!.copyWith(color: Colors.black);
    return Scrollbar(
      child: ListView(
+        primary: true,


Why is this important, though? Would anything break if we don't do this? Does this mean we are breaking users as well and they need to go in to make this change in their apps?

dev/integration_tests/flutter_gallery/lib/demo/material/elevation_demo.dart

dev/integration_tests/flutter_gallery/lib/demo/typography_demo.dart

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

goderbauer · 2022-04-26T16:50:17Z

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

-          && Scrollable.of(primaryScrollController.position.context.notificationContext!) != null;
-      }
+      final ScrollController? primaryScrollController = PrimaryScrollController.of(focus.context!);
+      return primaryScrollController != null && primaryScrollController.hasClients;


for my own understanding: why were you able to simplify this so much?

I moved much of this logic to the invoke method. Accessing the position here would throw an error (the bug we are fixing), but the error did not make sense in the stack trace that would result from throwing here.
By throwing from invoke, I thought it would make more sense.

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

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

packages/flutter/test/widgets/scroll_view_test.dart

Piinks · 2022-04-28T20:42:44Z

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

+  ///
+  /// If a [ScrollController] has already been provided to
+  /// [ScrollView.controller], or [ScrollView.primary] is false, this is
+  /// disregarded.


Added more docs, let me know what you think.

Piinks · 2022-04-28T20:55:54Z

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

-          && Scrollable.of(primaryScrollController.position.context.notificationContext!) != null;
-      }
+      final ScrollController? primaryScrollController = PrimaryScrollController.of(focus.context!);
+      return primaryScrollController != null && primaryScrollController.hasClients;


I moved much of this logic to the invoke method. Accessing the position here would throw an error (the bug we are fixing), but the error did not make sense in the stack trace that would result from throwing here.
By throwing from invoke, I thought it would make more sense.

Piinks · 2022-04-28T20:56:43Z

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

+        && Scrollable.of(primaryScrollController.position.context.notificationContext!) == null) {
+        return;
+      }
+      state = Scrollable.of(primaryScrollController.position.context.notificationContext!);


This is where I relocated some of the logic from isEnabled, after we know it is safe to access the position.

goderbauer

LGTM

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

goderbauer · 2022-05-16T17:36:27Z

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

+      }());
+
+      if (primaryScrollController!.position.context.notificationContext == null
+        && Scrollable.of(primaryScrollController.position.context.notificationContext!) == null) {


nit: indentation is off here (condition continuation and body should not have same indentation level). I think I would just indent this line by 2 more spaces...

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

fluttergithubbot · 2022-05-19T00:02:15Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.
This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

Piinks added c: new feature Nothing broken; request for a new capability f: scrolling Viewports, list views, slivers, etc. a: desktop Running on desktop labels Apr 18, 2022

Piinks changed the title ~~Psc 2~~ Updating PrimaryScrollController for Desktop Apr 18, 2022

Piinks commented Apr 18, 2022

View reviewed changes

Piinks force-pushed the psc-2 branch from 52bd139 to 6352bcf Compare April 18, 2022 21:00

Piinks removed f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Apr 18, 2022

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Apr 25, 2022

Piinks requested a review from goderbauer April 25, 2022 23:42

goderbauer reviewed Apr 26, 2022

View reviewed changes

Piinks commented Apr 28, 2022

View reviewed changes

Piinks removed f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels May 4, 2022

NamanShergill mentioned this pull request May 5, 2022

PrimaryScrollController has multiple positions for multiple ScrollViews in the NestedScrollView body, throwing multiple errors in the console NamanShergill/flutter_scroll_to_top#11

Closed

goderbauer approved these changes May 16, 2022

View reviewed changes

Piinks commented May 18, 2022

View reviewed changes

packages/flutter/lib/src/widgets/scrollable.dart Outdated Show resolved Hide resolved

Piinks added the waiting for tree to go green label May 18, 2022

fluttergithubbot removed the waiting for tree to go green label May 19, 2022

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 10, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

62a71cd

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 10, 2022

Roll Flutter from 9202e69febca to feec13a1684b (52 revisions) flutter/packages#2238

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 10, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

7076c2d

…er#102099)

This was referenced Jun 11, 2022

Roll Flutter from 9202e69febca to bc08d85718b6 (61 revisions) flutter/packages#2239

Closed

Roll Flutter from 25e0e290e6a4 to bc08d85718b6 (36 revisions) flutter/plugins#5951

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 11, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

cee5515

…er#102099)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 11, 2022

784520b Updating PrimaryScrollController for Desktop (flutter/flutter…

65da70f

…#102099)

engine-flutter-autoroll mentioned this pull request Jun 11, 2022

Roll Flutter from 9202e69febca to bc08d85718b6 (61 revisions) flutter/packages#2240

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 11, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

218fdc6

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 12, 2022

Roll Flutter from 9202e69febca to 689b5cc68d2f (66 revisions) flutter/packages#2241

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 12, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

7e808bd

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 12, 2022

Roll Flutter from 9202e69febca to 689b5cc68d2f (66 revisions) flutter/packages#2242

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 12, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

dd68f6a

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 13, 2022

Roll Flutter from 9202e69febca to 712860d33b00 (70 revisions) flutter/packages#2243

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 13, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

c870fa2

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 13, 2022

Roll Flutter from 9202e69febca to 712860d33b00 (70 revisions) flutter/packages#2244

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 13, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

8912f4d

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter from 9202e69febca to 0cd8f3d02b8a (76 revisions) flutter/packages#2245

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

93fa544

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter from 9202e69febca to f1c637e7c028 (77 revisions) flutter/packages#2246

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

c1f080b

…er#102099)

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter from 9202e69febca to f1c637e7c028 (77 revisions) flutter/packages#2247

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

a69c99d

…er#102099)

This was referenced Jun 15, 2022

Migration guide for PrimaryScrollController Updates flutter/website#7274

Merged

Apply PrimaryScrollController updates to SingleChildScrollView #106430

Merged

Piinks mentioned this pull request Jul 12, 2022

Enable scrollbars flutter/gallery#735

Merged

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Updating PrimaryScrollController for Desktop (flutter#102099)

d6a0865

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

784520bd0 Updating PrimaryScrollController for Desktop (flutter/flutt…

17b9410

…er#102099)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

784520b Updating PrimaryScrollController for Desktop (flutter/flutter…

2ac6b6a

…#102099)

Piinks mentioned this pull request Feb 14, 2023

Fix license page crash #120728

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating PrimaryScrollController for Desktop #102099

Updating PrimaryScrollController for Desktop #102099

Piinks commented Apr 18, 2022 •

edited

Loading

Piinks Apr 18, 2022

goderbauer Apr 26, 2022

Piinks Apr 28, 2022

Piinks Apr 28, 2022

Piinks Apr 18, 2022

goderbauer Apr 26, 2022

Piinks Apr 28, 2022

Piinks Apr 28, 2022

goderbauer Apr 26, 2022

goderbauer Apr 26, 2022

Piinks Apr 28, 2022

Piinks Apr 28, 2022

Piinks Apr 28, 2022

Piinks Apr 28, 2022

goderbauer left a comment

goderbauer May 16, 2022

fluttergithubbot commented May 19, 2022

Updating PrimaryScrollController for Desktop #102099

Updating PrimaryScrollController for Desktop #102099

Conversation

Piinks commented Apr 18, 2022 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluttergithubbot commented May 19, 2022

Piinks commented Apr 18, 2022 •

edited

Loading