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

chrisbobbe · 2023-02-28T06:20:29Z

This makes these changes to how useSafeArea works in the ModalBottomSheetRoute class and its wrapper showModalBottomSheet:

With useSafeArea: true, the added SafeArea now has bottom: false, to prevent a gap from appearing between the bottom edge of the sheet and the bottom edge of the screen, across the system intrusion there. (This is the fix for ModalBottomSheetRoute: useSafeArea puts gap below sheet #121554.)
With useSafeArea: false, there is no longer a MediaQuery.removePadding with removeTop: true. This was preventing the sheet's content from accurately padding the system intrusion at the top, when desired. (This is the fix for ModalBottomSheetRoute: Content can't consume top notch with SafeArea #121752.)

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! This looks like a good fix. Questions below about the docs.

gnprice · 2023-02-28T06:38:34Z

packages/flutter/lib/src/material/bottom_sheet.dart

Is there a way to word this that provides more of a high-level story for what this field means?

In the existing version, I feel like I have that for the "true" case, but not for the "false" case. I don't really understand what "exposed to the top padding" or "un-exposed" etc. means; as best I understand what MediaQuery.removePadding actually does, I'm not seeing how "exposed" is the right verb for it.

Modulo that, I think this could look something like:

/// Whether to avoid system intrusions on the top, left, and right. /// /// If true, a [SafeArea] is used in order to keep the bottom sheet away from /// system intrusions at the top, left, and right sides of the screen. /// /// If false, [??? I don't really understand what the purpose of the `removeTop: true` /// is, so I'm not sure how to write this paragraph.] /// /// In either case, the bottom sheet extends all the way to the bottom of /// the screen, including any system intrusions. /// /// The default is false.

Some of the phrasing there, like "avoid system intrusions", is borrowed from the SafeArea docs.

A related question: in what sort of situation would a developer want to set this to true, or to leave it at its default of false?

[??? I don't really understand what the purpose of the removeTop: true is, so I'm not sure how to write this paragraph.]

I'm not sure about this either. 🤔 The PR that added useSafeArea is #107140, and its description links to #39205 (comment) as the motivation. That comment describes a use case I hadn't considered:

In this case, the scrollable ModalBottomSheet has a pinned AppBar at the top, and so expected to fill the status bar space.

Then in a comment shortly after (#39205 (comment)):

It's a really interesting use case! The bottomsheet actually expands as it scrolls, and then the sheet/appbar kind of 'docks' with the top of the screen when it reaches it.

For that use case, I see why you'd want to decline ModalBottomSheetRoute's offer to insert a SafeArea: that SafeArea would prevent the sheet, and so its built-in app bar, from "docking" / extending all the way to the top of the screen, through the inset / system intrusion there.

But the MediaQuery.removePadding with removeTop isn't necessary to accomplish that, and in fact (to me) it looks like an obstacle to nicely handling the top inset. The sheet extends up through the top inset, but the sheet's interactive content still needs to render outside the top inset. An app bar normally handles that by padding the top inset with a blank portion of its surface, but this app bar can only guess at how much padding to use, since its ambient MediaQueryData.padding.top will be zero instead of the actual unconsumed height of the top inset.

gnprice · 2023-02-28T21:49:27Z

(Continuing from the subthread #121588 (comment):)

It looks like the origin of that MediaQuery.removePadding was in #13497. It was part of #13408, which was also about several other PopupRoute pages: https://github.com/flutter/flutter/pull/13438/files
namely a dialog, or a dropdown, or a popup menu.

I think the idea there was that the route's layout was taking care of adding padding as needed to keep the child away from system intrusions, and so when it adds that padding it should account for it in an adjusted MediaQuery as usual. See #12913 (comment) from just before those changes. That makes sense.

For that use case, I see why you'd want to decline ModalBottomSheetRoute's offer to insert a SafeArea: that SafeArea would prevent the sheet, and so its built-in app bar, from "docking" / extending all the way to the top of the screen, through the inset / system intrusion there.

But the MediaQuery.removePadding with removeTop isn't necessary to accomplish that, and in fact (to me) it looks like an obstacle to nicely handling the top inset.

Yeah. If the useSafeArea: false case isn't actually adding any padding at the top (nor anywhere else), then it doesn't seem like it should be inserting a MediaQuery.removePadding. The latter should generally be a pure accounting matter that tracks the former.

gnprice · 2023-02-28T21:51:50Z

In fact, looking back at that thread #39205, here's something I notice: there are several people showing workarounds that basically are there to defeat the MediaQuery.removePadding, by getting the MediaQueryData.padding value from outside it.

Here's one:

A much simpler approach, just take the Window Object from WidgetBinding instance, and use MediaQueryData.fromWindow(WidgetsBinding.instance.window).padding.top; this gives you the padding required

Another:

showModalBottomSheet(
backgroundColor: Colors.transparent,
isScrollControlled: true,
context: context,
builder: (_) => Padding(
padding:
EdgeInsets.only(top: MediaQuery.of(context).padding.top),
child: Container(color: Colors.indigo, child: Column()),
));

(Note the builder's context is ignored, and the MediaQuery.of(context) refers to the outer context.)

So I wonder if in fact the MediaQuery.removePadding is potentially the wrong behavior for all users. Including the one in #39205 (comment) with the bottomsheet that "docks" with the top of the screen — they may be using one of these workarounds to avoid that behavior too.

In that case, the MediaQuery.removePadding should just be removed. That would certainly make it easier to write good coherent documentation for both cases of this field.

@Piinks, as the voice of that customer at #39205 (comment) with the interesting bottomsheet use case: does that seem like potentially the right answer to you?

(Meanwhile this whole subthread is about the useSafeArea: false case, as part of trying to write clean docs for this property, while the code change in this PR is purely about the useSafeArea: true case. So perhaps this should move to a new issue thread.)

Piinks · 2023-03-01T18:02:27Z

Hey @gnprice!

@Piinks, as the voice of that customer at #39205 (comment) with the interesting bottomsheet use case: does that seem like potentially the right answer to you?

TBH, I do not recall. I think your analysis makes sense though. I would say let's try it and run the internal test suite. That will surface the customer case (if it still exists) and we can take a closer look at it.

Fixes flutter#121554. Fixes flutter#121752.

chrisbobbe · 2023-03-02T01:26:26Z

Cool! I've just filed #121752 for the problematic MediaQuery.removePadding and updated this PR to fix that too. Please take a look! 🙂

gnprice

LGTM! Thanks for the fix.

chrisbobbe · 2023-03-04T00:39:09Z

Thanks for the review!

hannah-hyj

LGTM

hannah-hyj · 2023-03-04T00:59:39Z

Can you update this PR's description about what's changing. Mention removing bottom when useSafeArea is true and the MediaQuery.removePadding issue when useSafeArea is false

chrisbobbe · 2023-03-04T01:29:02Z

Thanks for the review! Done.

gnprice · 2023-03-06T18:23:07Z

It looks like Google testing failed. @Hangyujin are you able to take a look at what the failures were?

One thing I'm curious about is whether the failures are in the case of useSafeArea: true, or useSafeArea: false, or both. This PR really contains two independent fixes, one to each of those cases; so if only one of those is causing the faiulre, we could potentially start by landing just the fix for the other case.

But I think both of the fixes are good fixes, so in any event it'd be good to understand the nature of the failures.

hannah-hyj · 2023-03-06T19:13:22Z

It looks like the test failure happened when useSafeArea: false

I'd suggest separate these two fixes and land the useSafeArea: true case first

chrisbobbe · 2023-03-07T18:51:58Z

I'd suggest separate these two fixes and land the useSafeArea: true case first

Sure, makes sense! Just sent #122118 for the useSafeArea: true case; PTAL. 🙂

chrisbobbe · 2023-03-08T20:06:26Z

I'd suggest separate these two fixes and land the useSafeArea: true case first

And, with #122118 merged, I've just sent #122225 for the second fix; PTAL. 🙂

I'll go ahead and close this PR; it was doing both fixes together before we decided to split them up.

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

gnprice reviewed Feb 28, 2023

View reviewed changes

chrisbobbe mentioned this pull request Mar 1, 2023

msglist: Add message action sheet with "Share" button zulip/zulip-flutter#12

Merged

chrisbobbe mentioned this pull request Mar 2, 2023

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

Closed

Fix handling of safe-area padding in modal bottom sheets

fbb6ad7

Fixes flutter#121554. Fixes flutter#121752.

chrisbobbe force-pushed the pr-modal-bottom-sheet-use-safe-area branch from a6f7cd0 to fbb6ad7 Compare March 2, 2023 01:24

chrisbobbe changed the title ~~Remove bottom gap in ModalBottomSheetRoute with useSafeArea: true~~ Fix handling of safe-area padding in modal bottom sheets Mar 2, 2023

HansMuller requested a review from hannah-hyj March 2, 2023 18:29

gnprice approved these changes Mar 3, 2023

View reviewed changes

hannah-hyj approved these changes Mar 4, 2023

View reviewed changes

chrisbobbe closed this Mar 8, 2023

chrisbobbe deleted the pr-modal-bottom-sheet-use-safe-area branch March 8, 2023 20:06

gnprice mentioned this pull request Jun 27, 2023

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

Closed

8 tasks

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

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

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Feb 28, 2023

Uh oh!

gnprice commented Feb 28, 2023

Uh oh!

Piinks commented Mar 1, 2023

Uh oh!

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

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Mar 4, 2023

Uh oh!

hannah-hyj left a comment

Choose a reason for hiding this comment

Uh oh!

hannah-hyj commented Mar 4, 2023

Uh oh!

chrisbobbe commented Mar 4, 2023

Uh oh!

gnprice commented Mar 6, 2023

Uh oh!

hannah-hyj commented Mar 6, 2023

Uh oh!

chrisbobbe commented Mar 7, 2023

Uh oh!

chrisbobbe commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrisbobbe commented Feb 28, 2023 •

edited

Loading

chrisbobbe Feb 28, 2023 •

edited

Loading

chrisbobbe commented Mar 2, 2023 •

edited

Loading