Overlay supports unconstrained environments #139513

goderbauer · 2023-12-04T22:44:20Z

Unfortunately, we cannot auto-detect which OverlayEntry should be sizing the Overlay in unconstrained environment. So, this PR adds a special flag to annotate the Overlay Entry that should be used.

yjbanov · 2023-12-09T00:19:07Z

packages/flutter/lib/src/widgets/overlay.dart

+  ///
+  /// Setting this to true has no effect if the OverlayEntry uses a [Positioned]
+  /// widget to position itself in the Overlay.
+  final bool canSizeOverlay;


nit: I'd drop the "can" and just start with "sizes". The word "can" instills doubt. I'm guessing you added it because there's no guarantee that this overlay is the top-most one and so it will not necessarily be used to determine the size of the view. However, that's just a precedence rule or a tie-breaker. It's still OK to say that it sizes the view.

The word "can" instills doubt.

That's exactly the doubt I want to install because in the vast majority of cases (the Overlay is getting non-infinite constraints) it does not size the overlay. The cases where it does size it are very, very, very rare. I want people to see this property, pause, and read the documentation to learn when it will size the overlay.

yjbanov · 2023-12-09T00:23:12Z

packages/flutter/lib/src/widgets/overlay.dart

+  /// [BoxConstraints].
+  ///
+  /// Setting this to true has no effect if the OverlayEntry uses a [Positioned]
+  /// widget to position itself in the Overlay.


nit: it's pretty clear what "Overlay" and "OverlayEntries" are referring to, so I'd just use the plain English phrases "overlay" and "overlay entries" to refer to them. Otherwise, without the [...] they are kind of in limbo state, neither proper symbol links, nor proper nouns.

good point, updated.

yjbanov · 2023-12-09T00:24:58Z

packages/flutter/lib/src/widgets/overlay.dart

-    // Same BoxConstraints as used by RenderStack for StackFit.expand.
-    final BoxConstraints nonPositionedChildConstraints = BoxConstraints.tight(constraints.biggest);
+  void layoutChild(RenderBox child, BoxConstraints nonPositionedChildConstraints) {
+    final StackParentData childParentData = child.parentData! as StackParentData;


Has it been asserted elsewhere that child.parentData vends StackParentData? If not, consider asserting with a descriptive error message prior to unboxing.

It's guaranteed to be StackParentData here and using it this way is the established pattern in the framework.

yjbanov · 2023-12-09T00:27:21Z

packages/flutter/lib/src/widgets/overlay.dart

+        'itself to one of its children, but no suitable non-positioned child that has '
+        'OverlayEntry.canSizeOverlay set to true could be found.',
+      ),
+      ErrorHint('Try wrapping the Overlay in a SizedBox to give it a finite size.'),


Or adding a overlay entry with canSizeOverlay set to true?

yjbanov · 2023-12-09T00:30:53Z

packages/flutter/lib/src/widgets/overlay.dart

+    if (constraints.biggest.isFinite) {
+      size = constraints.biggest;
+    } else {
+      sizeDeterminingChild = _findSizeDeterminingChild();


Should we memoize the sizeDeterminingChild from the computeDryLayout invocation? I can imagine two issues with the code as is:

Due to a bug _findSizeDeterminingChild() returns different children and the code will keep on trucking, sizing itself incorrectly.

It looks up the same child twice, while a single look-up would suffice. Of course, in debug mode we could assert that the child hasn't changed since the last look-up.

If we go with memoization, we'd probably want to null out the memoized child after the layout is done for this frame, because it should be legal to change the child in the next frame.

The "dry" part in computeDryLayout means that the layout should be computed without changing any internal state. There is no guarantee that performLayout is called after computeDryLayout or that it is called with the same constraints or that the RenderObject is still in the same state when it is called and the memoized child is still valid. Of course, we could add caching logic that is invalidated when necessary, but let's defer adding more complexity to this until we see an actual performance problem with this (given that there are only every very few children in a overlay I doubt that there is much performance gain to be had here).

yjbanov · 2023-12-09T00:32:21Z

packages/flutter/test/material/app_test.dart

@@ -1602,6 +1602,20 @@ void main() {
    expect(find.byType(AnimatedTheme), findsNothing);
    expect(find.byType(Theme), findsOneWidget);
  });
+
+  testWidgets('MaterialApp works in an unconstrained environment', (WidgetTester tester) async {
+    // Regression test for https://github.com/flutter/flutter/issues/137875.


nit: I'd move this above the testWidgets and reduce the number of empty lines in the body of the test.

yjbanov · 2023-12-09T00:33:12Z

packages/flutter/test/widgets/overlay_test.dart

@@ -1593,6 +1593,177 @@ void main() {
    expect(find.text('Bye, bye'), findsOneWidget);
    expect(tester.state(find.byType(Overlay)), same(overlayState));
  });
+
+  testWidgets('Overlay.wrap works in an unconstrained environment', (WidgetTester tester) async {


Consider being more specific than "works". For example, "sizes to its child".

yjbanov · 2023-12-09T00:33:36Z

packages/flutter/test/widgets/overlay_test.dart

+    expect(tester.getSize(find.byType(Overlay)), const Size(123, 456));
+  });
+
+  testWidgets('Overlay works in an unconstrained environment', (WidgetTester tester) async {


ditto re "works"

yjbanov · 2023-12-09T00:33:59Z

packages/flutter/test/widgets/overlay_test.dart

+    expect(find.text('World'), findsNothing);
+  });
+
+  testWidgets('Overlay throws if unconstrained and no child', (WidgetTester tester) async {


nit: "and has no"

yjbanov · 2023-12-09T00:42:22Z

packages/flutter/lib/src/widgets/overlay.dart

+        'The constraints given to the overlay ($constraints) would result in an illegal '
+        'infinite size (${constraints.biggest}). To avoid that, the Overlay tried to size '
+        'itself to one of its children, but no suitable non-positioned child that has '
+        'OverlayEntry.canSizeOverlay set to true could be found.',


The last part of the sentence was confusing to me. "child that has OverlayEntry.canSizeOverlay set to true" sounds wrong because the positioned child is not an OverlayEntry so it can't have OverlayEntry.canSizeOverlay set to true. How about "but the overlay did not contain an overlay entry with a non-positioned child"?

Rephrased this a bit to hopefully remove that confusion. Just having a non-positioned child is not enough, though, it has to belong to an OverlayEntry that also sets canSizeOverlay. Hope that's clearer now.

LongCatIsLooong · 2023-12-09T01:40:23Z

packages/flutter/lib/src/widgets/overlay.dart

+  /// an infinite size, it has to rely on one of its children to size itself. In
+  /// this situation, the Overlay will consult the topmost [OverlayEntry] that
+  /// has this property set to true, lay it out with unconstrained
+  /// [BoxConstraints], and force all other OverlayEntries to have the same


Unless the OverlayEntry is positioned?

Right, updated this to make that clearer.

LongCatIsLooong · 2023-12-09T03:55:02Z

packages/flutter/lib/src/widgets/overlay.dart

+  /// [BoxConstraints], and force all other OverlayEntries to have the same
+  /// size.
+  ///
+  /// OverlayEntries that set this to true must be able to handle unconstrained


question: determining whether an OverlayEntry can handle unconstrained BoxConstraints sounds a bit challenging in general? Right now it seems it's always set to true for opaque routes,
so I guess the idea is to trust developers to not put widgets that want to take the full width/height, into a dynamically sized view, without applying extra constraints? If they did then RenderBox.setter should be able to provide actionable feedback?

Yes, all of this is true, assuming you mean the RenderBox.size setter.

LongCatIsLooong · 2023-12-09T04:04:45Z

packages/flutter/lib/src/widgets/overlay.dart

+  /// constraints to be as large as possible. However, if that would result in
+  /// an infinite size, it has to rely on one of its children to size itself. In
+  /// this situation, the Overlay will consult the topmost [OverlayEntry] that
+  /// has this property set to true, lay it out with unconstrained


nit: not necessarily completely unconstrained I think? Maybe it's just infinite maxWidth or infinite maxHeight or infinite both? I feel unconstrained [BoxConstraints] sounded like we are going to throw the incoming constraints away.

(Can a dynamically sized view give you constrained width but unconstrained height?)

I feel unconstrained [BoxConstraints] sounded like we are going to throw the incoming constraints away.

Oh, good catch. I'll update the docs to make it clear that we just pass on the incoming constraints in that case.

(Can a dynamically sized view give you constrained width but unconstrained height?)

Yes, this is allowed.

LongCatIsLooong · 2023-12-09T04:24:46Z

packages/flutter/lib/src/widgets/overlay.dart

+  /// In most situations the Overlay sizes itself based on its incoming
+  /// constraints to be as large as possible. However, if that would result in
+  /// an infinite size, it has to rely on one of its children to size itself. In
+  /// this situation, the Overlay will consult the topmost [OverlayEntry] that


Should non-positioned be mentioned here too?

I added it for clarity.

yjbanov · 2023-12-11T21:18:13Z

packages/flutter/lib/src/widgets/overlay.dart

+  @override
+  void performLayout() {
+    RenderBox? sizeDeterminingChild;
+    if (constraints.biggest.isFinite) {


Should this be constraints.isTight? Otherwise, will this allow the user to give the view the flexibility to size itself, but also set a limit to make sure the size doesn't blow up? If I'm reading it correctly, if the constraints are limited in any way then we'll force it to take up all the available space, even if biggest is overkill for the content that's shown.

If I'm reading it correctly, if the constraints are limited in any way then we'll force it to take up all the available space, even if biggest is overkill for the content that's shown.

Yes, that's existing behavior and a lot of things would likely break if we change that :)

…r#139513)

flutter/flutter@11a9cb7...a51e33a 2023-12-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 223f4b2465dd to caf33276468b (1 revision) (flutter/flutter#140156) 2023-12-14 engine-flutter-autoroll@skia.org Roll Packages from b5958e2 to 1151191 (10 revisions) (flutter/flutter#140154) 2023-12-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from a3f9393f9591 to 223f4b2465dd (1 revision) (flutter/flutter#140151) 2023-12-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 913446eca57c to a3f9393f9591 (2 revisions) (flutter/flutter#140144) 2023-12-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 923f9e29d4b5 to 913446eca57c (1 revision) (flutter/flutter#140132) 2023-12-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9f7004e3e30e to 923f9e29d4b5 (7 revisions) (flutter/flutter#140130) 2023-12-14 magder@google.com Add self back to CODEOWNERS (flutter/flutter#140080) 2023-12-14 git@reb0.org Adapt wording for required Android SDK for plugins (flutter/flutter#140043) 2023-12-14 andrewrkolos@gmail.com [reland] Support conditional bundling of assets based on `--flavor` (flutter/flutter#139834) 2023-12-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 9f7004e3e30e to 45b95f264d63 (1 revision)" (flutter/flutter#140123) 2023-12-14 dacoharkes@google.com [deps] update Android SDK to 34 (flutter/flutter#138183) 2023-12-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9f7004e3e30e to 45b95f264d63 (1 revision) (flutter/flutter#140108) 2023-12-14 31859944+LongCatIsLooong@users.noreply.github.com Catch `Stopwatch` with static analysis (flutter/flutter#140019) 2023-12-14 goderbauer@google.com Overlay supports unconstrained environments (flutter/flutter#139513) 2023-12-14 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.10 to 3.22.11 (flutter/flutter#140087) 2023-12-14 rmolivares@renzo-olivares.dev Remove deprecated `ThemeData.selectedRowColor` (flutter/flutter#139080) 2023-12-14 15619084+vashworth@users.noreply.github.com Unpin mac_toolchain version (flutter/flutter#139938) 2023-12-14 chingjun@google.com Optimize file transfer when using proxied devices. (flutter/flutter#139968) 2023-12-14 38378650+hgraceb@users.noreply.github.com Add commonly used parameter names (flutter/flutter#140027) 2023-12-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc3267724e1b to 9f7004e3e30e (4 revisions) (flutter/flutter#140090) 2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Remove header guard from generated key map (flutter/flutter#140082) 2023-12-13 reidbaker@google.com Do not use project in do last (flutter/flutter#139325) 2023-12-13 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#140024) 2023-12-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Warn when Gradle plugins are applied using the legacy "apply script method" way" (flutter/flutter#140102) 2023-12-13 58190796+MitchellGoodwin@users.noreply.github.com Swap iOS back button icon in Material app bar (flutter/flutter#134754) 2023-12-13 engine-flutter-autoroll@skia.org Roll Packages from 80aa46a to b5958e2 (10 revisions) (flutter/flutter#140069) 2023-12-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9039ac78cf03 to fc3267724e1b (26 revisions) (flutter/flutter#140084) 2023-12-13 barpac02@gmail.com Warn when Gradle plugins are applied using the legacy "apply script method" way (flutter/flutter#139690) 2023-12-13 magder@google.com Add self as bundler dependabot reviewer (flutter/flutter#140081) 2023-12-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal" (flutter/flutter#140085) 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 dit@google.com,rmistry@google.com,stuartmorgan@google.com 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

Fixes flutter#137875. Unfortunately, we cannot auto-detect which OverlayEntry should be sizing the Overlay in unconstrained environment. So, this PR adds a special flag to annotate the Overlay Entry that should be used.

…r#139513)

goderbauer mentioned this pull request Dec 4, 2023

MaterialApp should support unconstrained environments #137875

Closed

goderbauer force-pushed the dynamically-sized-overlay branch from b0ca136 to c1795fb Compare December 8, 2023 18:21

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Dec 8, 2023

goderbauer force-pushed the dynamically-sized-overlay branch from c1795fb to a55a128 Compare December 8, 2023 22:18

github-actions bot added the f: material design flutter/packages/flutter/material repository. label Dec 8, 2023

goderbauer force-pushed the dynamically-sized-overlay branch from a55a128 to bb10e86 Compare December 8, 2023 22:38

goderbauer marked this pull request as ready for review December 8, 2023 22:42

goderbauer requested review from LongCatIsLooong and ditman December 9, 2023 00:03

goderbauer changed the title ~~Overlay supports unconstraind environments~~ Overlay supports unconstrained environments Dec 9, 2023

yjbanov reviewed Dec 9, 2023

View reviewed changes

LongCatIsLooong reviewed Dec 9, 2023

View reviewed changes

goderbauer added 7 commits December 11, 2023 11:05

Overlay supports unconstraind environments

284ce4a

++

16179ac

++

aa78c8e

++

29853a5

more tests

b4130aa

whitespace

f747c74

reviews

b01fc46

goderbauer force-pushed the dynamically-sized-overlay branch from bb10e86 to b01fc46 Compare December 11, 2023 19:32

goderbauer requested review from LongCatIsLooong and yjbanov December 11, 2023 19:33

yjbanov reviewed Dec 11, 2023

View reviewed changes

LongCatIsLooong approved these changes Dec 11, 2023

View reviewed changes

yjbanov approved these changes Dec 12, 2023

View reviewed changes

goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023

auto-submit bot merged commit 4162272 into flutter:master Dec 14, 2023
68 checks passed

goderbauer deleted the dynamically-sized-overlay branch December 14, 2023 00:25

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter from 11a9cb7029d7 to e1f954f4fd51 (27 revisions) flutter/packages#5676

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023

416227259 Overlay supports unconstrained environments (flutter/flutte…

c70c62a

…r#139513)

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter from 11a9cb7029d7 to 625b8017cda3 (28 revisions) flutter/packages#5678

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023

416227259 Overlay supports unconstrained environments (flutter/flutte…

3d1a416

…r#139513)

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter from 11a9cb7029d7 to 3a20f9ca2ac4 (29 revisions) flutter/packages#5679

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023

416227259 Overlay supports unconstrained environments (flutter/flutte…

53483fb

…r#139513)

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter from 11a9cb7029d7 to a51e33ac76e4 (30 revisions) flutter/packages#5680

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023

416227259 Overlay supports unconstrained environments (flutter/flutte…

c3f777d

…r#139513)

HansMuller mentioned this pull request Dec 15, 2023

Flutter Framework Team: Notable Commits #121415

Open

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

416227259 Overlay supports unconstrained environments (flutter/flutte…

93d56d9

…r#139513)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overlay supports unconstrained environments #139513

Overlay supports unconstrained environments #139513

goderbauer commented Dec 4, 2023 •

edited

yjbanov Dec 9, 2023

goderbauer Dec 11, 2023

yjbanov Dec 9, 2023

goderbauer Dec 11, 2023

yjbanov Dec 9, 2023

goderbauer Dec 11, 2023

yjbanov Dec 9, 2023

goderbauer Dec 11, 2023

yjbanov Dec 9, 2023

goderbauer Dec 11, 2023

yjbanov Dec 9, 2023

yjbanov Dec 9, 2023

yjbanov Dec 9, 2023

yjbanov Dec 9, 2023

yjbanov Dec 9, 2023

goderbauer Dec 11, 2023

LongCatIsLooong Dec 9, 2023

goderbauer Dec 11, 2023

LongCatIsLooong Dec 9, 2023

goderbauer Dec 11, 2023

LongCatIsLooong Dec 9, 2023

LongCatIsLooong Dec 9, 2023

goderbauer Dec 11, 2023

LongCatIsLooong Dec 9, 2023

goderbauer Dec 11, 2023

yjbanov Dec 11, 2023

goderbauer Dec 11, 2023

Overlay supports unconstrained environments #139513

Overlay supports unconstrained environments #139513

Conversation

goderbauer commented Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer commented Dec 4, 2023 •

edited