Skip to content

Conversation

@ahmedsameha1
Copy link
Contributor

This is my attempt to handle #6537 for the Banner widget.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 24, 2025
Copy link
Contributor

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

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 a regression test to ensure the Banner widget does not crash when rendered in a zero-sized area. The test case is well-written and covers the intended scenario.

While reviewing this change, I also looked at the implementation of the Banner widget in packages/flutter/lib/src/widgets/banner.dart and found a couple of issues that could be addressed in a follow-up change:

  1. Painter Lifecycle: In _BannerState, the BannerPainter is recreated and disposed on every build call. This is inefficient and can lead to a race condition where a painter is used after being disposed. A safer approach would be to delay the disposal of the old painter until the next frame.

  2. Incomplete shouldRepaint: The BannerPainter.shouldRepaint method does not check for changes in textDirection, layoutDirection, or shadow. This can lead to incorrect rendering if these properties change.

These are separate issues from the test being added, but are relevant to the overall quality of the Banner widget.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

Checklist:

  • The test is in the correct file
  • The test name goes “does not crash at zero area”
  • The target widget is wrapped by Center (or is fullscreen)
  • The target widget does not have an overlay, or the overlay is tested
  • The target widget is expected to have a size of exactly Size.zero

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 30, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Dec 30, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Dec 30, 2025
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 30, 2025
@dkwingsmt
Copy link
Contributor

@ahmedsameha1 Can you resolve the merge conflict? (kinda hard to do on web)

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2026
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2026
@auto-submit
Copy link
Contributor

auto-submit bot commented Jan 13, 2026

autosubmit label was removed for flutter/flutter/180254, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR.

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2026
@auto-submit auto-submit bot added this pull request to the merge queue Jan 13, 2026
Merged via the queue into flutter:master with commit beb57ad Jan 13, 2026
71 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 15, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 15, 2026
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 15, 2026
Manual roll requested by tarrinneal@google.com

flutter/flutter@793b0b8...b45a73b

2026-01-13 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#180900)
2026-01-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 34318de9874b to ebaf52c13799 (1 revision) (flutter/flutter#180870)
2026-01-13 ahmedsameha1@gmail.com Make sure that an ErrorWidget doesn't crash in 0x0 environment (flutter/flutter#180830)
2026-01-13 ahmedsameha1@gmail.com Make sure that a Banner doesn't crash in 0x0 environment (flutter/flutter#180254)
2026-01-13 30870216+gaaclarke@users.noreply.github.com Adds metal background to engine dart tests (flutter/flutter#180700)
2026-01-13 huy@nevercode.io Add TabBar API example for scroll notification integration (flutter/flutter#180728)
2026-01-13 engine-flutter-autoroll@skia.org Roll Skia from 714d0af2eda7 to 8f3134206e8d (1 revision) (flutter/flutter#180874)
2026-01-13 97480502+b-luk@users.noreply.github.com Add build-time checks for ImpellerC's SkSL compiler (flutter/flutter#180861)

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 stuartmorgan@google.com,tarrinneal@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
ikramhasan pushed a commit to ikramhasan/flutter that referenced this pull request Jan 15, 2026
)

This is my attempt to handle
flutter#6537 for the Banner widget.

Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants