Repository navigation
refactor: remove material import from scrollable_semantics_test and selectable_region_context_menu_test - #186611
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes Material library dependencies from the scrollable_semantics_test.dart and selectable_region_context_menu_test.dart files, replacing MaterialApp with TestWidgetsApp and SliverAppBar with a custom SliverPersistentHeader delegate. It also updates the cross-import checker to reflect these changes. Feedback identifies an inconsistency where a fixed-height delegate is used instead of a collapsing one, which may not fully replicate the original SliverAppBar behavior during scrolling.
| pinned: true, | ||
| expandedHeight: kExpandedAppBarHeight, | ||
| flexibleSpace: FlexibleSpaceBar(title: Text('App Bar')), | ||
| delegate: _PinnedHeaderDelegate(height: kExpandedAppBarHeight), |
There was a problem hiding this comment.
In this test case, the SliverPersistentHeader is initialized with a fixed-height delegate (_PinnedHeaderDelegate(height: kExpandedAppBarHeight)). However, the original SliverAppBar would have collapsed to the default toolbar height (56.0) when scrolled.
To maintain consistency with the original behavior and with the other test case implemented below (line 348), consider using the collapsing constructor here as well. This ensures that if the test involves scrolling, the header behaves as expected.
| delegate: _PinnedHeaderDelegate(height: kExpandedAppBarHeight), | |
| delegate: _PinnedHeaderDelegate.collapsing( | |
| minExtent: _kToolbarHeight, | |
| maxExtent: kExpandedAppBarHeight, | |
| ), |
78c40b9 to
d2c9583
Compare
|
selectable_region_context_menu_test.dart is already covered by #186672, which is approved and intentionally scoped to that file. Could this PR be narrowed to scrollable_semantics_test.dart plus its allowlist entry to avoid duplicate ownership/conflicts? |
|
You are right, thanks for calling this out. I should have checked the existing ownership more carefully before picking up those files. #186623 is already closed, and I am closing #186672 in favor of this PR to avoid keeping duplicate work in the queue. I will do a stricter open-PR check before taking new files going forward. |
|
Thank you @MarlonJD, Normally we keep posting in issue itself when we are picking that issue but since this issue contains multiple files and many people were working on it, we needed this kind of communication. But great job, I will review your another pending PRs. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Hi @rkishan516, thank you for the contribution. Had a small comment but mostly looks good to me.
| return Viewport( | ||
| offset: offset, | ||
| slivers: <Widget>[ | ||
| const SliverAppBar( |
There was a problem hiding this comment.
I recommend making a copy of this test in the material library widget tests so we don't lose out on SliverAppBar test coverage here.
There was a problem hiding this comment.
I have added TODO for this, once material is unfrozen for changes, will add this.
There was a problem hiding this comment.
I recommend filing an issue so we don't lose track of this and adding the link to the issue in the TODO.
There was a problem hiding this comment.
Filed #189117 and added the link to the TODO in packages/flutter/test/widgets/scrollable_semantics_test.dart.
1352eaa to
b11b722
Compare
ec1c6bf to
70cc774
Compare
|
Looks like |
flutter/flutter@cf9e8af...846664b 2026-07-14 engine-flutter-autoroll@skia.org Roll Skia from dfcff99566c3 to 88954ef8f36d (1 revision) (flutter/flutter#189440) 2026-07-14 34465683+rkishan516@users.noreply.github.com refactor: remove material import from scrollable_semantics_test and selectable_region_context_menu_test (flutter/flutter#186611) 2026-07-14 engine-flutter-autoroll@skia.org Roll Skia from 3d1fc554f1a2 to dfcff99566c3 (17 revisions) (flutter/flutter#189428) 2026-07-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 2c587df8f05a to 05bf153370c4 (5 revisions) (flutter/flutter#189426) 2026-07-14 nshahan@google.com [flutter_tools] Remove web hot reload flag (flutter/flutter#185994) 2026-07-14 chris@bracken.jp [iOS] Fix flaky keyboard animation test (flutter/flutter#189353) 2026-07-14 flar@google.com [Impeller] Playground expanded role (flutter/flutter#188889) 2026-07-14 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#189409) 2026-07-13 6655696+guidezpl@users.noreply.github.com Update lock-threads dependency to 6.0.2 (flutter/flutter#189053) 2026-07-13 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 6.1.0 to 6.2.0 in the all-github-actions group (flutter/flutter#189396) 2026-07-13 116356835+AbdeMohlbi@users.noreply.github.com Remove outdated todo about `analysis bug on Windows` and update condition to also perform `analysis on windows` (flutter/flutter#189283) 2026-07-13 ahmedsameha1@gmail.com Add more 0x0 size tests part 4 (flutter/flutter#185187) 2026-07-13 engine-flutter-autoroll@skia.org Roll Packages from 20928d5 to ad2eab1 (18 revisions) (flutter/flutter#189387) 2026-07-13 bkonyi@google.com [flutter_tools] Fix ADB device listing output parsing regression (flutter/flutter#189369) 2026-07-13 magder@google.com Stop running most Mac x64 builders that have Mac ARM equivalents on master (flutter/flutter#189301) 2026-07-13 magder@google.com Move a few benchmarks from x64 Intel Macs to ARM (flutter/flutter#189377) 2026-07-13 34871572+gmackall@users.noreply.github.com Add note that `hcpp` needs impeller (flutter/flutter#189382) 2026-07-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from vhIlDkWIy21IrlB9E... to oOETA0ISPouDt2xBo... (flutter/flutter#189349) 2026-07-13 68429735+Vonarian@users.noreply.github.com [flutter_tools] Respect mustMatchAppBuild on Windows native assets (flutter/flutter#186788) 2026-07-13 engine-flutter-autoroll@skia.org Roll Skia from 8bf65996caba to 3d1fc554f1a2 (2 revisions) (flutter/flutter#189350) 2026-07-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 0fc1668c4af4 to 2c587df8f05a (9 revisions) (flutter/flutter#189351) 2026-07-13 1961493+harryterkelsen@users.noreply.github.com [web] Fall back to full CJK fonts for characters not covered by split slices (flutter/flutter#188890) 2026-07-13 magder@google.com Take Mac tool_integration_tests_* out of bringup (flutter/flutter#189368) 2026-07-13 dacoharkes@google.com [hooks] Roll record_use to 1.0 and unpin (flutter/flutter#189366) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC louisehsu@google.com,stuartmorgan@google.com 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
The widgets test 'showOnScreen works with pinned app bar and sliver list' was switched from SliverAppBar to a SliverPersistentHeader delegate in flutter#186611, which left the Material SliverAppBar path without coverage for SemanticsAction.showOnScreen. Restore that coverage in app_bar_sliver_test.dart and drop the TODO that tracked it. Fixes flutter#189117
flutter/flutter#186611 removed SliverAppBar from two showOnScreen tests, but only the sliver-list one was tracked by a TODO. Restore the individual-slivers case as well, which exercises FlexibleSpaceBar collapse behaviour that the SliverPersistentHeader stand-in does not.
The widgets test 'showOnScreen works with pinned app bar and sliver list' in flutter/flutter was switched from SliverAppBar to a SliverPersistentHeader delegate in flutter/flutter#186611, which left the SliverAppBar path without coverage for SemanticsAction.showOnScreen. Add that coverage here, as the original TODO pointed at material_ui and Material is frozen in flutter/flutter. Fixes flutter/flutter#189117
flutter/flutter#186611 removed SliverAppBar from two showOnScreen tests, but only the sliver-list one was tracked by a TODO. Restore the individual-slivers case as well, which exercises FlexibleSpaceBar collapse behaviour that the SliverPersistentHeader stand-in does not.
The widgets test 'showOnScreen works with pinned app bar and sliver list' was switched from SliverAppBar to a SliverPersistentHeader delegate in flutter#186611, which left the Material SliverAppBar path without coverage for SemanticsAction.showOnScreen. Restore that coverage in app_bar_sliver_test.dart and drop the TODO that tracked it. Fixes flutter#189117
In flutter#186611 the widgets test `showOnScreen works with pinned app bar and sliver list` was switched from `SliverAppBar` to a private `SliverPersistentHeader` delegate, as part of removing the `material` import from `scrollable_semantics_test.dart`. That was the only test exercising `SemanticsAction.showOnScreen` against a pinned `SliverAppBar`, so the Material path lost coverage and a TODO was left behind to track restoring it. This PR restores the coverage by adding an equivalent test to `packages/flutter/test/material/app_bar_sliver_test.dart`, which already imports `../widgets/semantics_tester.dart` and is where the other `SliverAppBar` semantics tests live. The test performs `SemanticsAction.showOnScreen` on the first list item and asserts it is revealed *below* the pinned app bar (`dy == expandedHeight`) rather than underneath it. I also removed the TODO in `packages/flutter/test/widgets/scrollable_semantics_test.dart`, since it points at the issue this PR fixes. Note that the TODO says "in material_ui package" — that package does not exist in the repo yet, so the test is placed in the existing Material test suite (which matches the issue description) and will move along with `app_bar_sliver_test.dart` when the decoupling work lands. Happy to keep the TODO instead if you'd prefer to track the `material_ui` move separately. Verification: - `flutter test packages/flutter/test/material/app_bar_sliver_test.dart` — 53 tests pass. - `flutter test packages/flutter/test/widgets/scrollable_semantics_test.dart` — 20 tests pass. - To confirm the assertion is load-bearing, flipping `pinned` to `false` fails the new test with `Expected: <56.0> Actual: <6.0>`. Fixes flutter#189117 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [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. - [x] 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [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
This PR remove material import from scrollable_semantics_test and selectable_region_context_menu_test
Part of: #177415
Pre-launch Checklist
///).