Skip to content

fix(patrol_finders): stop in-progress fling so scrollTo works with no… - #3123

Draft
Kendru98 wants to merge 1 commit into
masterfrom
fix/scroll-to-no-settle
Draft

fix(patrol_finders): stop in-progress fling so scrollTo works with no…#3123
Kendru98 wants to merge 1 commit into
masterfrom
fix/scroll-to-no-settle

Conversation

@Kendru98

Copy link
Copy Markdown
Collaborator

…Settle

With SettlePolicy.noSettle the scroll loop pumps a single frame after each drag. The drag's pointer-up starts a ballistic fling that the single pump never finishes, and while a Scrollable is mid-fling it wraps its contents in an IgnorePointer - so the hitTestable() visibility check never matches and scrollTo drags until maxScrolls and throws.

Halt any in-progress fling between drags by pinning the scrollable to its current offset (making it idle, which releases the IgnorePointer), without advancing the clock so noSettle keeps its meaning for unrelated infinite animations.

Closes #3115

…Settle

With SettlePolicy.noSettle the scroll loop pumps a single frame after each
drag. The drag's pointer-up starts a ballistic fling that the single pump
never finishes, and while a Scrollable is mid-fling it wraps its contents in
an IgnorePointer - so the hitTestable() visibility check never matches and
scrollTo drags until maxScrolls and throws.

Halt any in-progress fling between drags by pinning the scrollable to its
current offset (making it idle, which releases the IgnorePointer), without
advancing the clock so noSettle keeps its meaning for unrelated infinite
animations.

Closes #3115

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@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 introduces a mechanism to stop in-progress scrolling (mid-fling) when using SettlePolicy.noSettle in PatrolTester, resolving an issue where a target widget remains hit-testable because of an active fling. It also adds a regression test to verify this behavior. The review feedback suggests making the _stopInProgressScrolling method more robust by safely checking the element type and handling potential exceptions when accessing the scroll position.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +988 to 998
void _stopInProgressScrolling() {
for (final element in find.byType(Scrollable).evaluate()) {
final state = (element as StatefulElement).state;
if (state is ScrollableState) {
final position = state.position;
if (position.isScrollingNotifier.value && position.hasPixels) {
position.jumpTo(position.pixels);
}
}
}
}

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.

high

Accessing state.position can throw a StateError if the scroll position has not been established yet (for example, if the Scrollable is offstage or not yet fully laid out). Additionally, casting element directly to StatefulElement using as can throw a TypeError if the element type is unexpected.

To make this more robust and prevent test crashes, use a safe type check (is! StatefulElement) and wrap the scroll position access and mutation in a try-catch block.

  void _stopInProgressScrolling() {
    for (final element in find.byType(Scrollable).evaluate()) {
      if (element is! StatefulElement) {
        continue;
      }
      final state = element.state;
      if (state is ScrollableState) {
        try {
          final position = state.position;
          if (position.isScrollingNotifier.value && position.hasPixels) {
            position.jumpTo(position.pixels);
          }
        } catch (_) {
          // Ignore if the scroll position is not established or cannot be mutated.
        }
      }
    }
  }

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scrollTo(settlePolicy: SettlePolicy.noSettle) gets stuck on CustomScrollView; tester.ensureVisible() works

1 participant