Fix: `CupertinoActionSheet` should take up max height when actions section is short #150708

dkwingsmt · 2024-06-24T17:48:42Z

This PR addresses a bug from the previous rewrite of the CupertinoActionSheet height allocation algorithm.

The previous approach assigns the content section with a minimal height. As a result, if the actions section is shorter than _kActionSheetActionsSectionMinHeight, the remaining space will not be taken by the content section.

As far as I know, this algorithm can not be implemented by simply compositing simple layout widgets, and therefore I created a new widget _PriorityColumn that uses a custom layout algorithm. (The resulting code is still shorter (and cleaner) than before the rewrite!)

Some other trivial refactor is also done to clean up the code around parameters of _ActionSheetMainSheet.

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

bleroux

While reading this PR for learning purposes, I spotted two small nits.

packages/flutter/lib/src/cupertino/dialog.dart

Co-authored-by: Bruno Leroux <[email protected]>

…o as-priority-column

dkwingsmt · 2024-06-26T18:01:05Z

packages/flutter/test/cupertino/action_sheet_test.dart


 void main() {
-  testWidgets('Overall looks correctly under light theme', (WidgetTester tester) async {
+  testWidgets('Overall appearance is correct for the light theme', (WidgetTester tester) async {


I sneaked in some test name fixes as suggested by Hans in #150725 (comment)

chunhtai

refactor looks good, just some question and nits

chunhtai · 2024-07-01T20:09:21Z

packages/flutter/lib/src/cupertino/dialog.dart

+// Both child widgets stretch horizontally to the parent's maximum width
+// constraint, with vertical space allocated in this priority:
+//
+//  1. The `bottom` widget receives its requested height, up to a


I can't parse this sentence. according to the code height <= bottomMinHeight.
should it be call bottomMaxHeight

It might be easier to understand it from the other way.

If both widgets fit in the container, great, render both in their intrinsic height.

If not, then prioritize reducing the bottom widget's height.

However, the bottom widget can not be reduced too much. It has a minHeight. If the bottom widget has been reduced to minHeight, then further reduction has to come from the top widget.

packages/flutter/lib/src/cupertino/dialog.dart

chunhtai · 2024-07-01T20:20:14Z

packages/flutter/lib/src/cupertino/dialog.dart

+  @override
+  double computeMinIntrinsicHeight(double width) {
+    assert(childCount == 2);
+    return firstChild!.getMinIntrinsicHeight(width) + lastChild!.getMinIntrinsicHeight(width);


This should also roughly follow the layout algorithm, are we assume this widget will not be wrap in a intrinsicHeight widget?

should this just call _childHeight with infinite height constrant?

Sorry, I may not be understanding computeMinIntrinsicHeight correctly. Can you explain more how you think it should change and why?

I originally thought the bottom will have min height, but looks like it can be as small as it can. sorry for the confusion, this looks fine.

packages/flutter/lib/src/cupertino/dialog.dart

…o as-priority-column

This reverts commit 00663af.

chunhtai

LGTM

…ction is short (flutter#150708) This PR addresses a bug from the previous rewrite of the `CupertinoActionSheet` height allocation algorithm. The previous approach assigns the content section with a minimal height. As a result, if the actions section is shorter than `_kActionSheetActionsSectionMinHeight`, the remaining space will not be taken by the content section. As far as I know, this algorithm can not be implemented by simply compositing simple layout widgets, and therefore I created a new widget `_PriorityColumn` that uses a custom layout algorithm. (The resulting code is still shorter (and cleaner) than before the rewrite!) Some other trivial refactor is also done to clean up the code around parameters of `_ActionSheetMainSheet`.

…tions section is short (flutter/flutter#150708)

flutter/flutter@99bb2ff...af913a7 2024-07-02 [email protected] Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter/flutter#151073) 2024-07-02 [email protected] ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter/flutter#143538) 2024-07-02 [email protected] Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter/flutter#151169) 2024-07-02 [email protected] docimports for painting (flutter/flutter#151143) 2024-07-02 [email protected] docimports for scheduler (flutter/flutter#151126) 2024-07-02 [email protected] `dismissible.dart` code cleanup (flutter/flutter#150276) 2024-07-02 [email protected] docimports for physics (flutter/flutter#151125) 2024-07-02 [email protected] docimports for services (flutter/flutter#151134) 2024-07-02 [email protected] docimports for cupertino (flutter/flutter#151149) 2024-07-02 [email protected] docimports for gestures (flutter/flutter#151123) 2024-07-02 [email protected] Docimports for foundation (flutter/flutter#151119) 2024-07-02 [email protected] docimports for semantics (flutter/flutter#151132) 2024-07-02 [email protected] [flutter_driver] add allocator mtl to memory event allowlist. (flutter/flutter#151153) 2024-07-02 [email protected] Roll Flutter Engine from 40c087b31515 to 433d360eee11 (7 revisions) (flutter/flutter#151165) 2024-07-02 [email protected] Refactor BuildInfo to always require packageConfigPath (flutter/flutter#150559) 2024-07-02 [email protected] Roll Flutter Engine from d3c5bd66a78f to 40c087b31515 (1 revision) (flutter/flutter#151156) 2024-07-02 [email protected] Roll Flutter Engine from fc5bc14e6091 to d3c5bd66a78f (1 revision) (flutter/flutter#151155) 2024-07-02 [email protected] Fix: `CupertinoActionSheet` should take up max height when actions section is short (flutter/flutter#150708) 2024-07-02 [email protected] Roll Flutter Engine from 3456fee1a6b9 to fc5bc14e6091 (8 revisions) (flutter/flutter#151150) 2024-07-02 [email protected] [tool] remove some temporary printTrace calls (flutter/flutter#151074) 2024-07-01 [email protected] Implementing a few switch statements (flutter/flutter#150946) 2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (#150969)" (flutter/flutter#151147) 2024-07-01 [email protected] Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (flutter/flutter#150969) 2024-07-01 [email protected] Roll Flutter Engine from b57a044ed10f to 3456fee1a6b9 (5 revisions) (flutter/flutter#151127) 2024-07-01 [email protected] Read AndroidManifest.xml and emit manifest-aar-impeller-(enabled|disabled) analytics (flutter/flutter#150970) 2024-07-01 [email protected] More docimports for animation library (flutter/flutter#151011) 2024-07-01 [email protected] Bump dartdoc to 8.0.10 (flutter/flutter#151107) 2024-07-01 [email protected] Fix missing `[` in docs (flutter/flutter#151091) 2024-07-01 [email protected] Roll pub packages (flutter/flutter#151028) 2024-07-01 [email protected] Fix teardown of a FocusScopeNode in material/app_test (flutter/flutter#151115) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ction is short (flutter#150708) This PR addresses a bug from the previous rewrite of the `CupertinoActionSheet` height allocation algorithm. The previous approach assigns the content section with a minimal height. As a result, if the actions section is shorter than `_kActionSheetActionsSectionMinHeight`, the remaining space will not be taken by the content section. As far as I know, this algorithm can not be implemented by simply compositing simple layout widgets, and therefore I created a new widget `_PriorityColumn` that uses a custom layout algorithm. (The resulting code is still shorter (and cleaner) than before the rewrite!) Some other trivial refactor is also done to clean up the code around parameters of `_ActionSheetMainSheet`.

…tions section is short (flutter/flutter#150708)

dkwingsmt added 2 commits June 24, 2024 10:34

Test

0133e85

Code

4db647e

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Jun 24, 2024

dkwingsmt requested a review from chunhtai June 24, 2024 17:49

dkwingsmt added 4 commits June 24, 2024 11:12

Fix doc

18cb69b

Merge remote-tracking branch 'origin/main' into as-priority-column

18f897b

Better test doc

567c0a3

Fix actions not being scrolled

cc92ec1

bleroux reviewed Jun 24, 2024

View reviewed changes

packages/flutter/lib/src/cupertino/dialog.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/cupertino/dialog.dart Outdated Show resolved Hide resolved

dkwingsmt and others added 6 commits June 24, 2024 13:54

Apply suggestions from code review

8476965

Co-authored-by: Bruno Leroux <[email protected]>

Merge branch 'master' into as-priority-column

dbaf590

Merge branch 'master' into as-priority-column

f7f6720

Merge remote-tracking branch 'origin/main' into as-priority-column

951a9d1

Better test name

9cc8837

Merge branch 'as-priority-column' of github.com:dkwingsmt/flutter int…

04a9f44

…o as-priority-column

dkwingsmt commented Jun 26, 2024

View reviewed changes

dkwingsmt added 3 commits June 26, 2024 14:45

Merge branch 'master' into as-priority-column

24abdcf

Merge branch 'master' into as-priority-column

36e5c98

Merge branch 'master' into as-priority-column

fd8d333

chunhtai reviewed Jul 1, 2024

View reviewed changes

dkwingsmt added 2 commits July 1, 2024 13:51

minHeight -> maxHeight; typedef

00663af

Merge branch 'as-priority-column' of github.com:dkwingsmt/flutter int…

7225965

…o as-priority-column

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 1, 2024

dkwingsmt added 3 commits July 1, 2024 13:55

Clarify algorithm

48c3611

Fix algorithm explanation

c1c0a61

Revert "minHeight -> maxHeight; typedef"

893c62a

This reverts commit 00663af.

github-actions bot removed the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 1, 2024

Typedef

6904753

chunhtai approved these changes Jul 1, 2024

View reviewed changes

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 1, 2024

Merge branch 'master' into as-priority-column

a950490

auto-submit bot merged commit b616372 into flutter:master Jul 2, 2024

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to a052fb763814 (17 revisions) flutter/packages#7041

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

f60f1b6

…tions section is short (flutter/flutter#150708)

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 61326afdae44 (18 revisions) flutter/packages#7042

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

f8a8c81

…tions section is short (flutter/flutter#150708)

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 455be19d928f (20 revisions) flutter/packages#7043

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

49efdde

…tions section is short (flutter/flutter#150708)

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 12e3635e8e27 (27 revisions) flutter/packages#7045

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

ef53f1d

…tions section is short (flutter/flutter#150708)

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 9527259c705c (28 revisions) flutter/packages#7046

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

bb07c31

…tions section is short (flutter/flutter#150708)

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to af913a75b5d2 (30 revisions) flutter/packages#7047

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

451c237

…tions section is short (flutter/flutter#150708)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

b616372 Fix: CupertinoActionSheet should take up max height when ac…

947cef8

…tions section is short (flutter/flutter#150708)

Fix: CupertinoActionSheet should take up max height when actions section is short #150708

Fix: CupertinoActionSheet should take up max height when actions section is short #150708

Uh oh!

Conversation

dkwingsmt commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

bleroux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dkwingsmt Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

chunhtai Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

chunhtai Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix: `CupertinoActionSheet` should take up max height when actions section is short #150708

Fix: `CupertinoActionSheet` should take up max height when actions section is short #150708

dkwingsmt commented Jun 24, 2024 •

edited

Loading