Skip to content

fix a _ScaffoldLayout delegate update bug #104954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented May 30, 2022

Address #60248 (comment)

Found this issue when landing #104958
That change will wrap a LayoutBuilder widget to the body widget and add a dependence with mediaQueryData.

When Floating Action Button bottom padding not consumed by viewInsets change the media data will cause the upper
re-layout boundary dirty and trigger the _ScaffoldLayout re-layout, then the test failed.

So interesting.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 30, 2022
@@ -276,12 +276,17 @@ void main() {
await tester.pumpWidget(
MediaQuery(
data: const MediaQueryData(
padding: EdgeInsets.only(bottom: 20.0),
viewPadding: EdgeInsets.only(bottom: 20.0),
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the Floating Action Button's position has nothing to do with padding.bottom.
Maybe a typo? CC @Piinks : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Likely the case, IIRC this was a big change with a lot of code re-use between tests. Copy paste issue is my guess. 😝

tester.getBottomLeft(find.byType(Placeholder)).dy,
moreOrLessEquals(600.0 - 20.0 - kFloatingActionButtonMargin)
);

// Consume bottom padding - as if by the keyboard opening
await tester.pumpWidget(
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this change, this does not trigger a re-layout, so the test passes.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Excellent find! Thank you for fixing this - LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants