It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

An existing Git SHA, d8f073007b7c5df50a94e71e9726b32927018a32, 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.

@ishaquehassan Thank you for the great analysis. Before we start a detailed review, it seems that there are a lot of reformating in this PR, which are unnecessary and make the PR really hard to review. Can you undo these lines? (Maybe you used a different auto formatter?)

Thank you for the review! I've force-pushed the branch to remove all the unnecessary dart format changes.

The branch now contains only the 3 relevant commits:

1. fix: reset AppBar _scrolledUnder flag when scroll context changes — the actual fix (9 lines in app_bar.dart)
2. test: add regression tests for AppBar _scrolledUnder reset on scroll context change — regression tests
3. fix: resolve analyzer lint warnings in regression tests, lint fixes for the test code

Sorry for the noise from the auto-formatter, I had inadvertently run dart format on the full files instead of limiting it to just the changed sections.

The PR now only contains 1 file with 9 lines of addition. It doesn't align with your previous comment, and I also remembered seeing more. Did you lose your changelist during the merge?

oops yeah my bad, i accidentally dropped the test commits when i force pushed to remove the extra formatting. pushed them back, should be good now

Also: Analyze errors and some other real errors.

fixed in the latest commit. the analyze failure was dart format complaining about some multi-line expressions that fit on one line. ran dart format, cleaned it up. the other failures (Mac/Windows framework tests, web tests) are all in system_navigator_test and text_field_test, not related to this change

@ishaquehassan FYI recently we've changed the CICD policy in that the presubmit tests will only be triggered with the CICD label. And since labels can only applied by Members, feel free to comment and let us to apply the label. For more details, see #183554. Sorry for the inconvenience while we work on improving this work flow. Thanks!

Thanks for the heads up on the new CICD workflow @dkwingsmt, good to know! Will ask for the label once the approach is finalized.

Still waiting on @Piinks for guidance on the Scaffold-level fix I proposed above. No rush, just want to make sure I build it at the right layer before pushing new code 🙂

@dkwingsmt hey, would you mind adding the CICD label whenever you get a chance? want to make sure the tests pass with the latest changes 😊

Merging in main shouldn't have much of an impact on CI. Give me a moment to provide feedback.

In general, you do not need to merge in master so frequently. :)

@Piinks did some digging in the meantime, think the fix actually belongs in Scaffold, not AppBar 😅

AppBar has no dependency that changes when the body swaps so didChangeDependencies never fires here. What if Scaffold.didUpdateWidget handled it?

@override
void didUpdateWidget(Scaffold oldWidget) {
  super.didUpdateWidget(oldWidget);
  if (widget.body != oldWidget.body) {
    // dispatch synthetic reset signal through ScrollNotificationObserver
  }
}

When body changes, dispatch a synthetic notification with pixels: 0 through ScrollNotificationObserver. AppBar's existing _handleScrollNotification picks it up and resets _scrolledUnder, no changes to AppBar needed.

Covers both cases:

• Scrollable swap: synthetic reset fires first, real ScrollMetricsNotification from new scrollable corrects state after
• Non-scrollable swap: only synthetic reset fires, which is exactly what we want ✅

Only thing I am stuck on is the dispatch mechanism since _notify is private on ScrollNotificationObserver. Right direction or is there something cleaner? 👀

This would not be a good approach here. The _notify method being private is WAI. If you were to call that method it would break the API contract. It is meant to trigger notifications related to scrolling, enabling dispatch of notifications for any other reason could cause issues in many other widgets that listen for scrolling changes.

@Piinks totally makes sense, won't touch _notify. And yeah the frequent master merges were just me being curious whether CI would catch anything new with the latest changes, not a workflow I plan to keep doing 😄

So both approaches I tried are dead ends:

• didChangeDependencies doesn't fire on body swaps
• Synthetic notifications through _notify would break the scrolling API contract

At this point I've run out of ideas on where the reset should live. Would you be able to point me toward the right layer or mechanism for handling this? More than happy to implement whatever direction you think is correct 🙂

At this point I've run out of ideas on where the reset should live. Would you be able to point me toward the right layer or mechanism for handling this? More than happy to implement whatever direction you think is correct 🙂

I am not sure how to resolve the issue, unfortunately I am not able to work on it right now.

At this point I've run out of ideas on where the reset should live. Would you be able to point me toward the right layer or mechanism for handling this? More than happy to implement whatever direction you think is correct 🙂

I am not sure how to resolve the issue, unfortunately I am not able to work on it right now.

No worries, still investigating myself.

@Piinks did a deep dive into the internals and I think I found an approach that actually works. Hear me out on this one 😄

I know I said earlier that didChangeDependencies is fundamentally wrong for body swaps, and that was true at the time because AppBar had no dependency that changed on body swap. But I think the real problem wasn't the lifecycle method itself, it was the missing dependency. If we add that missing link, didChangeDependencies becomes the correct place for the reset.

Why didChangeDependencies never fired

AppBar has no InheritedWidget dependency that changes when the body changes:

• Scaffold.maybeOf(context) uses findAncestorStateOfType (creates no dependency)
• ScrollNotificationObserver.maybeOf(context) depends on _ScrollNotificationObserverScope, but updateShouldNotify only checks state instance identity (unchanged on body swap)
• _ScaffoldScope.updateShouldNotify only checks hasDrawer

No signal path from "body changed" to AppBar exists.

Fix: add the missing signal path

Track body identity in _ScaffoldScope so dependents get notified on body swap.

scaffold.dart:

Add a generation counter to ScaffoldState:

int _bodyGeneration = 0;

Detect body changes in didUpdateWidget via Widget.canUpdate:

@override
void didUpdateWidget(Scaffold oldWidget) {
  super.didUpdateWidget(oldWidget);

  if (widget.body != null && oldWidget.body != null) {
    if (!Widget.canUpdate(widget.body!, oldWidget.body!)) {
      _bodyGeneration++;
    }
  } else if ((widget.body == null) != (oldWidget.body == null)) {
    _bodyGeneration++;
  }

  // ... existing FAB/bottomSheet handling
}

build is always called after didUpdateWidget, so no setState needed.

Add bodyGeneration to _ScaffoldScope:

class _ScaffoldScope extends InheritedWidget {
  const _ScaffoldScope({
    required this.hasDrawer,
    required this.geometryNotifier,
    required this.bodyGeneration,
    required super.child,
  });

  final bool hasDrawer;
  final _ScaffoldGeometryNotifier geometryNotifier;
  final int bodyGeneration;

  @override
  bool updateShouldNotify(_ScaffoldScope oldWidget) {
    return hasDrawer != oldWidget.hasDrawer ||
        bodyGeneration != oldWidget.bodyGeneration;
  }
}

app_bar.dart:

Register the dependency using the existing Scaffold.hasDrawer(context) API, which already calls dependOnInheritedWidgetOfExactType<_ScaffoldScope>() internally. Then reset _scrolledUnder:

@override
void didChangeDependencies() {
  super.didChangeDependencies();
  _scrollNotificationObserver?.removeListener(_handleScrollNotification);

  // Register dependency on _ScaffoldScope so didChangeDependencies
  // fires when the Scaffold body changes (bodyGeneration update).
  Scaffold.hasDrawer(context);

  final ScaffoldState? scaffoldState = Scaffold.maybeOf(context);
  if (scaffoldState != null &&
      (scaffoldState.isDrawerOpen || scaffoldState.isEndDrawerOpen)) {
    return;
  }
  _scrollNotificationObserver = ScrollNotificationObserver.maybeOf(context);
  _scrollNotificationObserver?.addListener(_handleScrollNotification);

  if (_scrolledUnder) {
    setState(() {
      _scrolledUnder = false;
    });
  }
}

How it handles each case

• Scrollable to non-scrollable: bodyGeneration increments, updateShouldNotify returns true, didChangeDependencies fires, _scrolledUnder resets to false. No ScrollMetricsNotification from the non-scrollable body, so it stays false.
• Scrollable to different scrollable: same reset, then the new scrollable fires ScrollMetricsNotification on init, and _handleScrollNotification sets the correct state.
• Same body rebuild (parent setState): Widget.canUpdate returns true (same type, same key), bodyGeneration unchanged, updateShouldNotify false, no didChangeDependencies call.
• Drawer open/close: bodyGeneration unchanged, hasDrawer unchanged, updateShouldNotify false. Existing behavior preserved.

No synthetic notifications, no new public API, zero changes to the scrolling contract. Just an InheritedWidget dependency that was missing.

One thing worth flagging: using Scaffold.hasDrawer(context) purely for the side effect of dependency registration is indirect. If you'd prefer a dedicated method following the geometryOf pattern, happy to add one instead.

Does this direction make sense?

I appreciate the deep dive into the Scaffold internals, but adding a generation counter to such a fundamental widget as a workaround for AppBar state feels like a significant change that could have unintended side effects on performance and rebuild logic for all Scaffold dependents.

At this point, since we've cycled through several implementations (from didChangeDependencies to proposed Scaffold modifications), it’s clear the solution isn't straightforward. To ensure we land on an approach that aligns with the framework's design goals and doesn't introduce technical debt, let's move the design discussion back to the original issue #181701.

This will allow other maintainers and contributors subscribed to the issue to provide feedback on the best path forward before any more code is written, unfortunately myself and @dkwingsmt cannot go back and forth here trying to solve the issue with you. Once a robust strategy is agreed upon there, we can revisit the implementation here.

Totally understand @Piinks, moving the discussion to #181701. Really appreciate you and @dkwingsmt taking the time to walk through all these approaches with me, learned a lot about the framework internals through this process 🙌

Thanks! In the meantime, please stop pushing merges from main to this branch. It is not necessary right now, and creates a lot of noise. 🙏

I'm converting this PR to a draft since we're planning to discuss the design further. (from triage)

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!