Skip to content

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

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

Conversation

chrisbobbe
Copy link
Contributor

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.

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 15, 2023
@chrisbobbe
Copy link
Contributor Author

@gnprice

@HansMuller HansMuller self-requested a review September 15, 2023 21:46
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for sending this! Comment below.

I think it'd also be good to try to make this behavior clear in the short paragraph that summarizes useSafeArea in the class's overall doc:

/// The [useSafeArea] parameter specifies whether the sheet will avoid system
/// intrusions on the top, left, and right. If false, no SafeArea is added
/// and the top padding is consumed using [MediaQuery.removePadding].
/// Defaults to false.

The information is there if you understand the implications of "top padding is consumed". But I found that verb "consumed" ambiguous before I'd spent a lot of time reading through how SafeArea and MediaQuery interact. And because this specific behavior of ModalBottomSheetRoute is somewhat peculiar, that makes it unusually difficult to figure out when one is still trying to work out precisely what the terminology means and how the underlying pieces fit together.

Here's one version: "If false, no [SafeArea] is added; and [MediaQuery.removePadding] is applied to the top, so that system intrusions at the top will not be avoided by a [SafeArea] inside the bottom sheet either."

Comment on lines 982 to 988
/// If false, the bottom sheet will extend through any system intrusions
/// at the top, left, and right, and callers may wish to pad those
/// with their own [SafeArea] in [builder]. (Such a [SafeArea] will not
/// actually provide top padding, because [builder] will be run
/// under a [MediaQuery.removePadding] that has `removeTop: true`.
/// To work around, wrap the [SafeArea] in a [MediaQuery] that restates an
/// ambient [MediaQueryData] from outside [builder].)
Copy link
Member

Choose a reason for hiding this comment

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

From the discussion (particularly around #122225 (comment) ), it seems like the best advice for most developers will be to set useSafeArea: true. So we can perhaps foreground that over the workaround of looking at MediaQuery.of from an outer BuildContext.

I think the fact that the top padding gets suppressed in MediaQuery is among the key facts this documentation should make clear. This PR's version does say that, but it's within a parenthetical that's framed as explaining how a "callers may wish to" suggestion would behave, which means that a reader scanning for the pure facts about this widget's behavior may overlook it. The version in main has it framed appropriately, but then the "isn't exposed to" wording doesn't make clear what it's saying. So we should take an unambiguous description like in this PR revision, and move it up the information tree to be placed among the basic facts of what this property does.

Here's a version that does both of those:

Suggested change
/// If false, the bottom sheet will extend through any system intrusions
/// at the top, left, and right, and callers may wish to pad those
/// with their own [SafeArea] in [builder]. (Such a [SafeArea] will not
/// actually provide top padding, because [builder] will be run
/// under a [MediaQuery.removePadding] that has `removeTop: true`.
/// To work around, wrap the [SafeArea] in a [MediaQuery] that restates an
/// ambient [MediaQueryData] from outside [builder].)
/// If false, the bottom sheet will extend through any system intrusions
/// at the top, left, and right.
///
/// If false, then moreover [MediaQuery.removePadding] will be used
/// to remove top padding, so that a [SafeArea] widget inside the bottom
/// sheet will have no effect at the top edge. If this is undesired, consider
/// setting [useSafeArea] to true. Alternatively, wrap the [SafeArea] in a
/// [MediaQuery] that restates an ambient [MediaQueryData] from outside [builder].

@chrisbobbe chrisbobbe force-pushed the pr-bottom-sheet-use-safe-area-false branch from 6b39154 to 1ab53cf Compare October 2, 2023 22:05
@chrisbobbe
Copy link
Contributor Author

Thanks for the review, with those helpful suggestions! Revision pushed, using the text you suggested.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! One small comment remaining but otherwise LGTM.

@@ -792,9 +792,10 @@ class _ModalBottomSheetState<T> extends State<_ModalBottomSheet<T>> {
/// dragged up and down and dismissed by swiping downwards.
///
/// The [useSafeArea] parameter specifies whether the sheet will avoid system
/// intrusions on the top, left, and right. If false, no SafeArea is added
/// and the top padding is consumed using [MediaQuery.removePadding].
/// Defaults to false.
Copy link
Member

Choose a reason for hiding this comment

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

Should keep the "Defaults to false" line — that's helpful context too.

@chrisbobbe chrisbobbe force-pushed the pr-bottom-sheet-use-safe-area-false branch from 1ab53cf to b27b554 Compare October 6, 2023 21:35
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Revision pushed, with that bit added back.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 6, 2023
@auto-submit auto-submit bot merged commit a88a250 into flutter:master Oct 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 7, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 7, 2023
flutter/flutter@ad20089...5207a30

2023-10-07 [email protected] Roll Flutter Engine from 40024059b09e to 8711c1fd2191 (1 revision) (flutter/flutter#136110)
2023-10-07 [email protected] Roll Flutter Engine from eaac056b63a4 to 40024059b09e (1 revision) (flutter/flutter#136108)
2023-10-07 [email protected] Roll Flutter Engine from f71778651333 to eaac056b63a4 (1 revision) (flutter/flutter#136107)
2023-10-07 [email protected] bottom_sheet: Clarify doc about behavior when useSafeArea is false (flutter/flutter#134793)
2023-10-07 [email protected] Roll Flutter Engine from 356ab2a54862 to f71778651333 (1 revision) (flutter/flutter#136106)
2023-10-07 [email protected] Roll Flutter Engine from 1f79667fb860 to 356ab2a54862 (1 revision) (flutter/flutter#136104)
2023-10-07 [email protected] Simplify assertion in `AsyncSnapshot` (flutter/flutter#135899)
2023-10-07 [email protected] Roll Flutter Engine from b28032c157ee to 1f79667fb860 (1 revision) (flutter/flutter#136103)
2023-10-06 [email protected] Roll Flutter Engine from 7bf93bb919d9 to b28032c157ee (1 revision) (flutter/flutter#136101)
2023-10-06 [email protected] Roll Flutter Engine from eb5d5c66a83e to 7bf93bb919d9 (2 revisions) (flutter/flutter#136098)
2023-10-06 [email protected] [Windows] Add first Arm64 plugin tests (flutter/flutter#135512)
2023-10-06 [email protected] Roll Flutter Engine from 1bb228dfedfa to eb5d5c66a83e (2 revisions) (flutter/flutter#136094)
2023-10-06 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.6 to 2.22.0 (flutter/flutter#136095)
2023-10-06 [email protected] Call `markNeedsPaint` when adding overlayChild to `Overlay` (flutter/flutter#135941)
2023-10-06 [email protected] Revert "Marks Linux_samsung_a02 new_gallery__transition_perf to be unflaky" (flutter/flutter#136097)
2023-10-06 [email protected] Marks Linux_samsung_a02 new_gallery__transition_perf to be unflaky (flutter/flutter#135566)
2023-10-06 [email protected] Roll Flutter Engine from 59b6b94e1a51 to 1bb228dfedfa (1 revision) (flutter/flutter#136082)
2023-10-06 [email protected] RenderEditable should dispose created layers. (flutter/flutter#135942)
2023-10-06 [email protected] Marks Windows module_custom_host_app_name_test to be unflaky (flutter/flutter#135961)
2023-10-06 [email protected] Roll Packages from 6714d50 to e578a16 (2 revisions) (flutter/flutter#136074)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
…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.
@chrisbobbe chrisbobbe deleted the pr-bottom-sheet-use-safe-area-false branch October 11, 2023 17:55
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
flutter/flutter@ad20089...5207a30

2023-10-07 [email protected] Roll Flutter Engine from 40024059b09e to 8711c1fd2191 (1 revision) (flutter/flutter#136110)
2023-10-07 [email protected] Roll Flutter Engine from eaac056b63a4 to 40024059b09e (1 revision) (flutter/flutter#136108)
2023-10-07 [email protected] Roll Flutter Engine from f71778651333 to eaac056b63a4 (1 revision) (flutter/flutter#136107)
2023-10-07 [email protected] bottom_sheet: Clarify doc about behavior when useSafeArea is false (flutter/flutter#134793)
2023-10-07 [email protected] Roll Flutter Engine from 356ab2a54862 to f71778651333 (1 revision) (flutter/flutter#136106)
2023-10-07 [email protected] Roll Flutter Engine from 1f79667fb860 to 356ab2a54862 (1 revision) (flutter/flutter#136104)
2023-10-07 [email protected] Simplify assertion in `AsyncSnapshot` (flutter/flutter#135899)
2023-10-07 [email protected] Roll Flutter Engine from b28032c157ee to 1f79667fb860 (1 revision) (flutter/flutter#136103)
2023-10-06 [email protected] Roll Flutter Engine from 7bf93bb919d9 to b28032c157ee (1 revision) (flutter/flutter#136101)
2023-10-06 [email protected] Roll Flutter Engine from eb5d5c66a83e to 7bf93bb919d9 (2 revisions) (flutter/flutter#136098)
2023-10-06 [email protected] [Windows] Add first Arm64 plugin tests (flutter/flutter#135512)
2023-10-06 [email protected] Roll Flutter Engine from 1bb228dfedfa to eb5d5c66a83e (2 revisions) (flutter/flutter#136094)
2023-10-06 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.6 to 2.22.0 (flutter/flutter#136095)
2023-10-06 [email protected] Call `markNeedsPaint` when adding overlayChild to `Overlay` (flutter/flutter#135941)
2023-10-06 [email protected] Revert "Marks Linux_samsung_a02 new_gallery__transition_perf to be unflaky" (flutter/flutter#136097)
2023-10-06 [email protected] Marks Linux_samsung_a02 new_gallery__transition_perf to be unflaky (flutter/flutter#135566)
2023-10-06 [email protected] Roll Flutter Engine from 59b6b94e1a51 to 1bb228dfedfa (1 revision) (flutter/flutter#136082)
2023-10-06 [email protected] RenderEditable should dispose created layers. (flutter/flutter#135942)
2023-10-06 [email protected] Marks Windows module_custom_host_app_name_test to be unflaky (flutter/flutter#135961)
2023-10-06 [email protected] Roll Packages from 6714d50 to e578a16 (2 revisions) (flutter/flutter#136074)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App 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.

5 participants