Sitelet https://github.com/flutter/packages/pull/12341
Skip to content

[material_ui] Add SliverAppBar showOnScreen semantics test coverage - #12341

Closed
Vi-debug wants to merge 3 commits into
flutter:mainfrom
Vi-debug:test-sliver-app-bar-show-on-screen-semantics
Closed

[material_ui] Add SliverAppBar showOnScreen semantics test coverage#12341
Vi-debug wants to merge 3 commits into
flutter:mainfrom
Vi-debug:test-sliver-app-bar-show-on-screen-semantics

Conversation

@Vi-debug

@Vi-debug Vi-debug commented Aug 2, 2026 •
edited
Loading

Copy link
Copy Markdown

In flutter/flutter#186611, scrollable_semantics_test.dart had its material import removed, which meant replacing SliverAppBar with a private SliverPersistentHeader delegate in two tests:

  • showOnScreen works with pinned app bar and sliver list
  • showOnScreen works with pinned app bar and individual slivers

Those were the only tests exercising SemanticsAction.showOnScreen against a pinned SliverAppBar, so that behavior lost its coverage. This PR restores both, in packages/material_ui/test/app_bar_sliver_test.dart — which already imports semantics_tester.dart and is where the other SliverAppBar semantics tests live.

The two cases are not redundant:

  • sliver list — expandedHeight of 56.0, so the app bar never collapses; asserts the revealed item lands at kExpandedAppBarHeight.
  • individual slivers — expandedHeight of 256.0 with the scroll offset already past the collapse point; asserts the revealed item lands at kToolbarHeight. This is the case that actually exercises FlexibleSpaceBar collapse behavior, which the SliverPersistentHeader stand-in does not reproduce.

The coverage is added here rather than in flutter/flutter because Material is frozen there (flutter/flutter#184093), and the TODO left behind by #186611 explicitly pointed at the material_ui package.

A note on scope: flutter/flutter#189117 mentions only the first test, and only that one got a TODO. The second SliverAppBar removal in #186611 was untracked, so restoring only the first would have left the collapsing-app-bar path uncovered with nothing pointing at it. Happy to split the second test out if you'd rather keep this scoped to the issue as filed.

Verification, in packages/material_ui:

  • flutter test test/app_bar_sliver_test.dart — all tests pass.
  • Both new assertions were confirmed load-bearing by flipping pinned to false, which fails them.

No version bump or CHANGELOG entry: this is a test-only change, which is a documented exemption.

Fixes flutter/flutter#189117

Pre-Review Checklist

Footnotes

  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. ↩ ↩2

@Vi-debug
[material_ui] Add SliverAppBar showOnScreen semantics test coverage
2d77036 
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
@github-actions github-actions Bot added triage-framework Should be looked at in framework triage p: material_ui labels Aug 2, 2026
@Vi-debug Vi-debug mentioned this pull request Aug 2, 2026
Merged
10 tasks done
@Vi-debug
[material_ui] Also cover the collapsing pinned SliverAppBar case
56c1989 
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.
@Vi-debug Vi-debug mentioned this pull request Aug 2, 2026
Closed
10 tasks done
@Vi-debug
Vi-debug marked this pull request as ready for review August 2, 2026 16:52
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Aug 2, 2026
View reviewed changes

@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 adds two new widget tests to app_bar_sliver_test.dart to verify that showOnScreen works correctly with a pinned SliverAppBar when using a sliver list or individual slivers. Feedback was provided to use the defined kItemHeight constant instead of a hardcoded value of 72.0 for the height of individual sliver items to ensure consistency.

Comment thread packages/material_ui/test/app_bar_sliver_test.dart Outdated
@Vi-debug Vi-debug mentioned this pull request Aug 2, 2026
Closed
@Vi-debug
[material_ui] Name the scroll offset constant accurately
572a7e2 
kItemHeight was inherited from the upstream test but held 100.0 while the
items were 72.0 tall; it was only ever used to compute the initial scroll
offset. Make kItemHeight the actual item height and give the offset its
own constant. No behaviour change: items are still 72.0 and the initial
offset is still 250.0.
rezaflutter1374
rezaflutter1374 reviewed Aug 3, 2026
View reviewed changes

@rezaflutter1374 rezaflutter1374 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the added test coverage. Could you confirm whether the collapsed SliverAppBar state should also have equivalent semantics coverage?

@Vi-debug

Vi-debug commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Thanks for looking, @rezaflutter1374.

If you mean showOnScreen against a collapsed app bar: that's the second test. The two tests split precisely along that axis:

  • ...and sliver list — expandedHeight: 56.0, so the bar never collapses; asserts dy == kExpandedAppBarHeight.
  • ...and individual slivers — expandedHeight: 256.0, bar is fully collapsed when the action fires; asserts dy == kToolbarHeight.

In the second test the scroll offset (250.0) is past the collapse distance (256 - 56 = 200), so the bar is at its minimum extent when the action fires. I instrumented it to confirm rather than infer:

PAINT EXTENT: 56.0

That's the whole point of the assertion being kToolbarHeight rather than kExpandedAppBarHeight — if the bar were still expanded the item would land at 256.0, and the test would fail. It's the case that exercises the real FlexibleSpaceBar collapse path, which is why I restored it alongside the one the issue actually names.

@Piinks

Piinks commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Hi @Vi-debug, these packages are not accepting contributions yet. We will announce when they are, for now, I am going to close this PR. Thank you!

@Piinks Piinks closed this Aug 3, 2026
@Vi-debug Vi-debug mentioned this pull request Aug 26, 2026
Draft
@Vi-debug

Vi-debug commented Aug 26, 2026

Copy link
Copy Markdown
Author

Superseded by #12631 now that material_ui is accepting contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@rezaflutter1374 rezaflutter1374 rezaflutter1374 left review comments
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

p: material_ui triage-framework Should be looked at in framework triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add SliverAppBar test coverage for showOnScreen semantics

3 participants

@Vi-debug @Piinks @rezaflutter1374