ModalBottomSheetRoute: Allow content to consume top notch with SafeArea #122225

chrisbobbe · 2023-03-08T20:03:25Z

With useSafeArea: false, there is no longer a MediaQuery.removePadding with removeTop: true. That MediaQuery was preventing the sheet's content from accurately padding the system intrusion at the top, when desired.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gnprice

Thanks! LGTM. (And I see it's essentially the same as the remaining part of #121588 after #122118 was merged.)

We found on #121588 that these changes caused failures in Google testing. I believe this is nevertheless the right change, for the reasons at #121752 (comment) and in the thread up to #121588 (comment) . But I'd like to have @Hangyujin's input on what's going on in those tests — whether the code in question is just working around broken behavior and can be updated to work with the fixed behavior, or whether perhaps there's something we've missed that makes the existing behavior the right way after all.

gnprice · 2023-03-20T17:27:54Z

@Hangyujin Do you think you might be able to take a look at the Google tests affected by this fix? I see your major bottom-sheet PR #122445 has now been merged :-)

hannah-hyj · 2023-03-20T17:34:50Z

@chrisbobbe Hi Can you rebase this PR to re-trigger google testing? I can't see the failure details for it.

chrisbobbe · 2023-03-20T18:32:51Z

Ah it looks like @gnprice has just done so; thanks, Greg!

gnprice · 2023-03-22T16:04:26Z

@Hangyujin Are you able to see failure details for the PR now?

The reporting of status back to GitHub seems to be getting stuck, but I'm told in #hackers-infra that it ran and there are some failures visible from inside Google.

hannah-hyj · 2023-03-23T23:07:28Z

Talked to a release team member, and he suggested me to try give this an approval to see if it can trigger google testing to start.

gnprice · 2023-03-24T06:02:39Z

Looks like it's still in status pending.

I think the release team member's suggestion may have been about this point:

Google Testing does not start until a Flutter hacker has approved the PR.

But the PR already satisfied that condition, since I'd approved it earlier. So if it's about that condition, then a second approval wouldn't change things.

I'll try updating the branch again. Perhaps on a third roll of the dice, things will work. (Also, perhaps the underlying issue is about the infra running into GitHub rate limits, in which case this might be a less congested time of day.)

Fixes flutter#121752. With `useSafeArea: false`, there is no longer a `MediaQuery.removePadding` with `removeTop: true`. That `MediaQuery` was preventing the sheet's content from accurately padding the system intrusion at the top, when desired.

gnprice · 2023-03-24T18:43:09Z

🎉 Google testing reported results! Failures, as expected.

@Hangyujin when you get a chance, it would be great if you can take a look at what's going on in those tests — whether the code in question is just working around broken behavior and can be updated to work with the fixed behavior, or whether perhaps there's something we've missed that makes the existing behavior the right way after all.

(There are also failures in customer_testing, but those are unrelated to the PR: #123385.)

flutter-dashboard · 2023-04-13T18:43:43Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chrisbobbe · 2023-04-13T21:22:00Z

Just updated with a merge commit.

chrisbobbe · 2023-05-22T18:34:37Z

Just updated again.

chrisbobbe · 2023-06-27T20:13:51Z

Updated.

chrisbobbe · 2023-07-07T20:30:33Z

Hmm, an automated check failed: "Pull Request Labeler / triage (pull_request_target)".

Clicking through to see details, there are some error lines in the output:

[…]
Error: Error: found unexpected type for label 'a: accessibility' (should be array of config options)
Error: found unexpected type for label 'a: accessibility' (should be array of config options)
[…]

Neither this PR nor its linked issue (#121752) are labeled with 'a: accessibility'…hmm.

Ah! Those errors are mentioned in #129666, which has been closed as fixed. I'll make another merge commit to update this, and see if that fixes it.

Hixie · 2023-08-22T22:59:56Z

@chrisbobbe thanks for the contribution! Sorry for the delay in reviewing the test results.

I just triggered an update so that the bots would run again. Can you ping us on Discord when the tests give a result so that someone can help you find out what is going on? Thanks!

Hixie · 2023-08-23T20:30:56Z

Google testing failures: looks like it's caused a bunch of apps to have some additional padding at the top of bottom sheets.

chrisbobbe · 2023-08-23T21:18:32Z

Makes sense; I would expect that to happen in most cases where the bottom-sheet content has a SafeArea with top: true. Each case should be described by one of these three scenarios:

The bottom-sheet content doesn't want to pad the top inset. It makes the mistake of building a SafeArea with top: true and should stop doing that.
The bottom-sheet content wants to pad the top inset, and it wasn't doing so before. The SafeArea with top: true has started to work as intended. (Nothing to change.)
The bottom-sheet content wants to pad the top inset, and it was doing so before: whether to a wrong, hardcoded value, or to the right value because of hacking around the removeTop. The content includes a SafeArea with top: true, which used to be a no-op but now adds padding on top of the hack. The hack should be removed, leaving behind a normal use of SafeArea with top: true.

If that's wrong, perhaps there's something we've missed that makes the existing behavior the right way after all. 🙂

Hixie · 2023-08-23T21:27:07Z

The problem is that this would essentially introduce a silent regression which we could not detect or automation migration for. So we'd essentially be breaking shipping apps without any way for the developer to notice unless they tested every screen on a lot of devices (with different notches), which is more effort than many people can afford.

chrisbobbe · 2023-08-23T22:32:11Z

I see. I think one way forward would be:

Close as WONTFIX ModalBottomSheetRoute: Content can't consume top notch with SafeArea #121752.
Document the current behavior: with useSafeArea: false, if you put a SafeArea in the content, it won't have top padding. That's actually also true of useSafeArea: true, but that's not surprising, because in that case the top edge of the sheet can't actually enter the device's top inset.

Thinking more about this with @gnprice, we don't really understand the use case for useSafeArea: false, which is where #121752 is active. The default is false.

(why we don't understand that use case)

#39205 (comment) mentions one bottom sheet that builds a whole Scaffold with an app bar that "docks" at the top of the screen, and so it mustn't be prevented from entering the top inset, as true would do. I don't know which app that is, or whether it still does it that way. In that app, it's not clear to me whether you'd want top padding or not:

With padding, won't it look wrong when the sheet is open halfway?
Without padding, won't it look wrong when the sheet is open fully ("docked" at the top)?
With dynamic padding computed from the top edge's position, won't that break the Material metaphor?

Possibly, useSafeArea: true should have been the default, but changing that default also sounds disruptive and breaking. But as I said at the top of this comment, some more documentation could be helpful.

Hixie · 2023-08-23T23:17:00Z

Documentation is always welcome, certainly. And I agree that it's not clear that we made the right choices when initially developing this, unfortunately.

chrisbobbe · 2023-09-15T00:32:11Z

Makes sense. I just sent #134793 for that documentation, and so now I'll close this PR, and close #121752 as WONTFIX (after commenting there to refer to #134793).

As discussed at: flutter#122225 (comment)

…134793) As discussed at #122225 (comment), this is a docs change meant to help people in the absence of a fix for #121752, which is being closed as WONTFIX.

…lutter#134793) As discussed at flutter#122225 (comment), this is a docs change meant to help people in the absence of a fix for flutter#121752, which is being closed as WONTFIX.

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 8, 2023

chrisbobbe mentioned this pull request Mar 8, 2023

Fix handling of safe-area padding in modal bottom sheets #121588

Closed

8 tasks

gnprice approved these changes Mar 8, 2023

View reviewed changes

gnprice requested a review from hannah-hyj March 8, 2023 23:04

gnprice force-pushed the pr-bottom-sheet-allow-pad-top-notch branch from ae9992d to 9fd44ea Compare March 20, 2023 18:12

gnprice mentioned this pull request Mar 22, 2023

Google testing fails to report status to GitHub #123289

Closed

hannah-hyj approved these changes Mar 23, 2023

View reviewed changes

gnprice force-pushed the pr-bottom-sheet-allow-pad-top-notch branch from 6f231cc to 93f6de4 Compare March 24, 2023 06:03

Merge branch 'master' into pr-bottom-sheet-allow-pad-top-notch

b269dd0

Merge branch 'master' into pr-bottom-sheet-allow-pad-top-notch

6f6b33e

Merge branch 'master' into pr-bottom-sheet-allow-pad-top-notch

afa37cd

chrisbobbe and others added 2 commits July 7, 2023 13:30

Merge branch 'master' into pr-bottom-sheet-allow-pad-top-notch

5a6ce1f

Merge branch 'master' into pr-bottom-sheet-allow-pad-top-notch

f7478ac

chrisbobbe mentioned this pull request Sep 15, 2023

bottom_sheet: Clarify doc about behavior when useSafeArea is false #134793

Merged

8 tasks

chrisbobbe closed this Sep 15, 2023

chrisbobbe mentioned this pull request Sep 15, 2023

ModalBottomSheetRoute: Content can't consume top notch with SafeArea #121752

Closed

chrisbobbe added a commit to chrisbobbe/flutter that referenced this pull request Oct 6, 2023

bottom_sheet: Clarify doc about behavior when useSafeArea is false

b27b554

As discussed at: flutter#122225 (comment)

ModalBottomSheetRoute: Allow content to consume top notch with SafeArea #122225

ModalBottomSheetRoute: Allow content to consume top notch with SafeArea #122225

Uh oh!

Conversation

chrisbobbe commented Mar 8, 2023

Pre-launch Checklist

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 20, 2023

Uh oh!

hannah-hyj commented Mar 20, 2023

Uh oh!

chrisbobbe commented Mar 20, 2023

Uh oh!

gnprice commented Mar 22, 2023

Uh oh!

hannah-hyj commented Mar 23, 2023

Uh oh!

gnprice commented Mar 24, 2023

Uh oh!

gnprice commented Mar 24, 2023

Uh oh!

flutter-dashboard bot commented Apr 13, 2023

Uh oh!

chrisbobbe commented Apr 13, 2023

Uh oh!

chrisbobbe commented May 22, 2023

Uh oh!

chrisbobbe commented Jun 27, 2023

Uh oh!

chrisbobbe commented Jul 7, 2023

Uh oh!

Hixie commented Aug 22, 2023

Uh oh!

Hixie commented Aug 23, 2023

Uh oh!

chrisbobbe commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Aug 23, 2023

Uh oh!

chrisbobbe commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Aug 23, 2023

Uh oh!

chrisbobbe commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrisbobbe commented Aug 23, 2023 •

edited

Loading

chrisbobbe commented Aug 23, 2023 •

edited

Loading