Skip to content

Remove no-shuffle from scrollbar test - #186656

Merged
auto-submit[bot] merged 1 commit into
flutter:masterfrom
ValentinVignal:flutter/test/fix-scrollbar-tests
Sep 8, 2026
Merged

auto-submit[bot] merged 1 commit into
flutter:masterfrom
ValentinVignal:flutter/test/fix-scrollbar-tests

Conversation

@ValentinVignal

Copy link
Copy Markdown
Contributor

Part of #85160

Pre-launch Checklist

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

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label May 18, 2026
@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages f: scrolling Viewports, list views, slivers, etc. labels May 18, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the 'no-shuffle' tag from the scrollbar tests and reformats several test cases for consistency. It also updates the test verifying that the scrollbar does not show when scrolling an inner widget by wrapping it in a Theme and adding platform-specific assertions for paint calls using a switch statement. Feedback suggests refactoring this switch statement into a loop to reduce code duplication across multiple keys.

Comment on lines +1416 to +1433
switch (theme.platform) {
case TargetPlatform.android:
// On android, draws two rectangles for the track and thumb.
expect(tester.renderObject(find.byKey(key2)), paintsExactlyCountTimes(#drawRect, 2));
expect(tester.renderObject(find.byKey(key2)), paintsExactlyCountTimes(#drawRRect, 0));
expect(tester.renderObject(find.byKey(key1)), paintsExactlyCountTimes(#drawRect, 2));
expect(tester.renderObject(find.byKey(key1)), paintsExactlyCountTimes(#drawRRect, 0));
case TargetPlatform.iOS:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.macOS:
case TargetPlatform.windows:
// Draws a rectangle for the track and a rounded rectangle for the thumb.
expect(tester.renderObject(find.byKey(key2)), paintsExactlyCountTimes(#drawRect, 1));
expect(tester.renderObject(find.byKey(key2)), paintsExactlyCountTimes(#drawRRect, 1));
expect(tester.renderObject(find.byKey(key1)), paintsExactlyCountTimes(#drawRect, 1));
expect(tester.renderObject(find.byKey(key1)), paintsExactlyCountTimes(#drawRRect, 1));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The switch statement on theme.platform can be simplified and made more robust by using a loop to iterate over the keys being checked. This reduces code duplication and makes it easier to maintain if more keys are added in the future.

    for (final GlobalKey key in <GlobalKey>[key1, key2]) {
      switch (theme.platform) {
        case TargetPlatform.android:
          // On android, draws two rectangles for the track and thumb.
          expect(tester.renderObject(find.byKey(key)), paintsExactlyCountTimes(#drawRect, 2));
          expect(tester.renderObject(find.byKey(key)), paintsExactlyCountTimes(#drawRRect, 0));
        case TargetPlatform.iOS:
        case TargetPlatform.fuchsia:
        case TargetPlatform.linux:
        case TargetPlatform.macOS:
        case TargetPlatform.windows:
          // Draws a rectangle for the track and a rounded rectangle for the thumb.
          expect(tester.renderObject(find.byKey(key)), paintsExactlyCountTimes(#drawRect, 1));
          expect(tester.renderObject(find.byKey(key)), paintsExactlyCountTimes(#drawRRect, 1));
      }
    }

Comment on lines +1364 to +1369
final theme = ThemeData.light();

await tester.pumpWidget(
Theme(
data: theme,
child: Directionality(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix.

Depending on the theme, if the platform is Android or not, a radius is set:

void didChangeDependencies() {
final ThemeData theme = Theme.of(context);
_colorScheme = theme.colorScheme;
_scrollbarTheme = ScrollbarTheme.of(context);
switch (theme.platform) {
case TargetPlatform.android:
_useAndroidScrollbar = true;
case TargetPlatform.iOS:
case TargetPlatform.linux:
case TargetPlatform.fuchsia:
case TargetPlatform.macOS:
case TargetPlatform.windows:
_useAndroidScrollbar = false;
}
super.didChangeDependencies();
}

..radius =
widget.radius ??
_scrollbarTheme.radius ??
(_useAndroidScrollbar ? null : _kScrollbarRadius)

When there is a radius, it uses drawRRect, and when it is null it uses drawRect

When no theme is provided in the tree, Theme.of(context) returns the static _kFallbackTheme

static final ThemeData _kFallbackTheme = ThemeData.fallback();

which stores the platform value only once for the entire test file.

Since this test has the variant TargetPlatformVariant.all(), if the first one to run is android, then the matchers

expect(tester.renderObject(find.byKey(key)), paintsExactlyCountTimes(#drawRect, 2)

were passing.

But if any other platform was run before android, the matches would fail, because #drawRect is only called once.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Thanks for the fix:)

@victorsanni victorsanni added CICD Run CI/CD and removed CICD Run CI/CD labels May 18, 2026
@dkwingsmt
dkwingsmt requested a review from QuncCccccc May 20, 2026 18:29

@QuncCccccc QuncCccccc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM:)!

Comment on lines +1364 to +1369
final theme = ThemeData.light();

await tester.pumpWidget(
Theme(
data: theme,
child: Directionality(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Thanks for the fix:)

@dkwingsmt

Copy link
Copy Markdown
Contributor

Thanks for your contribution! This PR has reached completion, however we're not able to merge this PR for the moment due to the Material/Cupertino code freeze (#184093). As soon as the decoupling is complete, we will post a way to port this PR to the new packages to land. In the meantime, I'll convert this PR to a draft and apply a label so that we can quickly find it when the freeze is lifted.

@dkwingsmt
dkwingsmt marked this pull request as draft May 27, 2026 05:33
@dkwingsmt dkwingsmt added the waiting for code freeze This PR is waiting for a code freeze to resolve. label May 27, 2026
@Piinks Piinks added the Decoupling: Not ready to port yet Instructions will be provided when this is ready to move to flutter/packages. label Jun 24, 2026
@Piinks

Piinks commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

I've marked this PR as not ready to port to flutter/packages yet.
We'll provide instructions to move this change over to material_ui/cupertino_ui once ready to receive PRs. Thank you!

@ValentinVignal

Copy link
Copy Markdown
Contributor Author

PR has been ported: flutter/packages#12527

auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Aug 21, 2026
Part of flutter/flutter#85160

Ported from flutter/flutter#186656

## Pre-Review Checklist

**Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
@ValentinVignal

ValentinVignal commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor Author

If we want to remove the no-shuffle tag from the repository, we'd need to remove it from the tests currently using it. Re-opening

(cc @justinmc )

@ValentinVignal ValentinVignal reopened this Sep 4, 2026
@fluttergithubbot

Copy link
Copy Markdown
Contributor

An existing Git SHA, 4d647757cf6486820fd5fbe510789808f937e7e5, was detected, and no actions were taken.

To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with --force) that already was pushed before, push a blank commit (git commit --allow-empty -m "Trigger Build") or rebase to continue.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

This pull request contains changes to Material or Cupertino, which are currently frozen in this repository.

Changes should be made in material_ui and/or cupertino_ui in the flutter/packages repository.

Please refer to #188444 for instructions.

@ValentinVignal
ValentinVignal force-pushed the flutter/test/fix-scrollbar-tests branch from 4d64775 to 232287c Compare September 4, 2026 01:43
@ValentinVignal
ValentinVignal marked this pull request as ready for review September 4, 2026 01:43

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the 'no-shuffle' tag from scrollbar_test.dart and updates the test suite to handle platform-specific rendering assertions for scrollbars. Specifically, the test checking that the scrollbar does not show when scrolling an inner scrollable widget is updated to wrap the widget tree in a light theme and assert on the correct number of drawn rectangles or rounded rectangles depending on the target platform. Additionally, some tests are reformatted for style consistency. There are no review comments, so no further feedback is provided.

@Piinks Piinks added autosubmit Merge PR when tree becomes green via auto submit App and removed waiting for code freeze This PR is waiting for a code freeze to resolve. Decoupling: Not ready to port yet Instructions will be provided when this is ready to move to flutter/packages. labels Sep 8, 2026
@Piinks Piinks added the override code freeze Override an active code freeze. label Sep 8, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Sep 8, 2026
Merged via the queue into flutter:master with commit 9f6cac2 Sep 8, 2026
26 of 31 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Sep 8, 2026
flutter/flutter@63170e9...b444e78

2026-09-08 [email protected] Clean up lint errors in workflows (flutter/flutter#191987)
2026-09-08 [email protected] Remove no-shuffle tag from dropdown_test.dart (flutter/flutter#186398)
2026-09-08 [email protected] Remove no-shuffle from scrollbar test (flutter/flutter#186656)
2026-09-08 [email protected] [tool] Let --local-engine reach the build system again (flutter/flutter#192361)
2026-09-08 [email protected] [flutter_tools] Safely remove web loading indicator on dart-app-ready (flutter/flutter#192233)
2026-09-08 [email protected] [Linux] Don't clear the OpenGL context when a frame has been drawn (flutter/flutter#192137)
2026-09-08 [email protected] ci: update dartdoc from 9.0.6 to 9.0.9 (flutter/flutter#192405)
2026-09-07 [email protected] Add itemExtentBuilder support to ListView.separated (flutter/flutter#191157)
2026-09-07 [email protected] Roll Skia from e1437642d259 to b3fa80a0e168 (5 revisions) (flutter/flutter#192362)
2026-09-07 [email protected] Roll Skia from 2df865d9d369 to e1437642d259 (1 revision) (flutter/flutter#192360)
2026-09-07 [email protected] Roll Skia from c7aa5512d148 to 2df865d9d369 (2 revisions) (flutter/flutter#192357)
2026-09-06 [email protected] Roll Skia from db200e27a634 to c7aa5512d148 (2 revisions) (flutter/flutter#192351)
2026-09-06 [email protected] Roll Fuchsia Linux SDK from _IgixhH4vEgdz9Oqn... to p8hHkUi8uWT9R3N4z... (flutter/flutter#192349)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
…r#12791)

flutter/flutter@63170e9...b444e78

2026-09-08 [email protected] Clean up lint errors in workflows (flutter/flutter#191987)
2026-09-08 [email protected] Remove no-shuffle tag from dropdown_test.dart (flutter/flutter#186398)
2026-09-08 [email protected] Remove no-shuffle from scrollbar test (flutter/flutter#186656)
2026-09-08 [email protected] [tool] Let --local-engine reach the build system again (flutter/flutter#192361)
2026-09-08 [email protected] [flutter_tools] Safely remove web loading indicator on dart-app-ready (flutter/flutter#192233)
2026-09-08 [email protected] [Linux] Don't clear the OpenGL context when a frame has been drawn (flutter/flutter#192137)
2026-09-08 [email protected] ci: update dartdoc from 9.0.6 to 9.0.9 (flutter/flutter#192405)
2026-09-07 [email protected] Add itemExtentBuilder support to ListView.separated (flutter/flutter#191157)
2026-09-07 [email protected] Roll Skia from e1437642d259 to b3fa80a0e168 (5 revisions) (flutter/flutter#192362)
2026-09-07 [email protected] Roll Skia from 2df865d9d369 to e1437642d259 (1 revision) (flutter/flutter#192360)
2026-09-07 [email protected] Roll Skia from c7aa5512d148 to 2df865d9d369 (2 revisions) (flutter/flutter#192357)
2026-09-06 [email protected] Roll Skia from db200e27a634 to c7aa5512d148 (2 revisions) (flutter/flutter#192351)
2026-09-06 [email protected] Roll Fuchsia Linux SDK from _IgixhH4vEgdz9Oqn... to p8hHkUi8uWT9R3N4z... (flutter/flutter#192349)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
…r#12791)

flutter/flutter@63170e9...b444e78

2026-09-08 [email protected] Clean up lint errors in workflows (flutter/flutter#191987)
2026-09-08 [email protected] Remove no-shuffle tag from dropdown_test.dart (flutter/flutter#186398)
2026-09-08 [email protected] Remove no-shuffle from scrollbar test (flutter/flutter#186656)
2026-09-08 [email protected] [tool] Let --local-engine reach the build system again (flutter/flutter#192361)
2026-09-08 [email protected] [flutter_tools] Safely remove web loading indicator on dart-app-ready (flutter/flutter#192233)
2026-09-08 [email protected] [Linux] Don't clear the OpenGL context when a frame has been drawn (flutter/flutter#192137)
2026-09-08 [email protected] ci: update dartdoc from 9.0.6 to 9.0.9 (flutter/flutter#192405)
2026-09-07 [email protected] Add itemExtentBuilder support to ListView.separated (flutter/flutter#191157)
2026-09-07 [email protected] Roll Skia from e1437642d259 to b3fa80a0e168 (5 revisions) (flutter/flutter#192362)
2026-09-07 [email protected] Roll Skia from 2df865d9d369 to e1437642d259 (1 revision) (flutter/flutter#192360)
2026-09-07 [email protected] Roll Skia from c7aa5512d148 to 2df865d9d369 (2 revisions) (flutter/flutter#192357)
2026-09-06 [email protected] Roll Skia from db200e27a634 to c7aa5512d148 (2 revisions) (flutter/flutter#192351)
2026-09-06 [email protected] Roll Fuchsia Linux SDK from _IgixhH4vEgdz9Oqn... to p8hHkUi8uWT9R3N4z... (flutter/flutter#192349)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. override code freeze Override an active code freeze. p: material_ui material_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants