Skip to content

Conversation

@ahmedsameha1
Copy link
Contributor

This is my attempt to handle #6537 for the DropdownMenu widget.

@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 2, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a regression test to ensure that DropdownMenu does not crash when it is rendered in a zero-sized environment. The test correctly sets up the scenario and verifies the widget's size. I have suggested an improvement to make the test more robust by also checking for crashes when attempting to open the menu, as this could reveal further layout-related issues.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

Checklist:

  • The test is in the correct file
  • The test name goes “does not crash at zero area”
  • The target widget is wrapped by Center (or is fullscreen)
  • The target widget is not a overlay
  • Expect the target widget has a size of exactly Size.zero

@dkwingsmt
Copy link
Contributor

I've discussed with @LongCatIsLooong, who proposed that we could also set the view size to be zero.
This also reflects the actual goal of the original issue, that is to ensure widgets to not crash in a 0x0 view (the warmup frame). Specifically for DropdownMenu, it would be something like this:

  testWidgets('DropdownMenu does not crash at zero area', (WidgetTester tester) async {
    tester.view.physicalSize = Size.zero;
    final TextEditingController controller = TextEditingController(text: 'I');
    addTearDown(controller.dispose);

    await tester.pumpWidget(
      MaterialApp(
        home: Scaffold(
          body: Center(
            child: DropdownMenu<TestMenu>(
              dropdownMenuEntries: menuChildren,
              controller: controller,
            ),
          ),
        ),
      ),
    );
    expect(tester.getSize(find.byType(DropdownMenu<TestMenu>)), Size.zero);
    controller.selection = const TextSelection.collapsed(offset: 0);
    await tester.pump();
    expect(find.byType(MenuItemButton), findsWidgets);
  });

Notable points:

  • This snippet now set the physicalSize to be zero to align more closely to the original issue.
  • The controller is needed for this widget specifically because a zero-sized widget can't be tapped.
  • The target widget should have a 0x0 size even without the SizedBox.shrink. Nevertheless we can still verify its size.
  • This snippet does not verify the overlay's size, partly because I find no easy way (no public accessor to the overlay layer), but also that the overlay stack is not surrounded by anything there should be minimum possibility of human error; being able to render it in a 0x0 view should be enough.

Either this way or the previous way (with SizedBox.shrink) should be ok for simpler widgets without overlays (so no need to change existing tests or PRs), but this one is probably better for overlays. What do you think?

@ahmedsameha1
Copy link
Contributor Author

If I understand correctly, it is meaningless to make any spatial interaction with a widget that occupies a zero area of the screen, and people cannot make any spatial interaction with the widget when starting up the framework in a 0x0 environment, as the original purpose of the issue had been stated in #6537 (comment).

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

(Edited)

@dkwingsmt dkwingsmt self-requested a review September 18, 2025 04:47
@dkwingsmt
Copy link
Contributor

dkwingsmt commented Sep 18, 2025

If I understand correctly, it is meaningless to make any spatial interaction with a widget that occupies a zero area of the screen, and people cannot make any spatial interaction with the widget when starting up the framework in a 0x0 environment, as the original purpose of the issue had been stated in #6537 (comment).

The idea is not to test whether a zero-size widget is interactable, but to verify that the content in the pop-up can also be rendered in a zero-size constraints, which matters because:

  • Since a controller option is publicly available, it is legal and possible for developers to show a pop up in the same way as our test, i.e. the pop up doesn't have to be triggered by a gesture, which is worth verifying.
  • Although the 0x0 case isn't interactable, constraints of non-zero small constraints (such as 4x4 pixels) are interactable, which are worth verifying.

@ahmedsameha1
Copy link
Contributor Author

@dkwingsmt I see that you approved my changes. Do you want me to apply these changes: #174809 (comment)? Or what?

@dkwingsmt
Copy link
Contributor

@ahmedsameha1 Sorry I didn't meant to approve it. Yes, can you apply the change in the comment?

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@dkwingsmt dkwingsmt requested a review from victorsanni October 8, 2025 23:20
@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 10, 2025

autosubmit label was removed for flutter/flutter/174809, because - The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Mac framework_tests_impeller has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2025
@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2025
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2025
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 16, 2025

autosubmit label was removed for flutter/flutter/174809, because - The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Mac framework_tests_impeller has failed. Please fix the issues identified (or deflake) before re-applying this label.

@victorsanni victorsanni self-requested a review October 17, 2025 06:52
@victorsanni
Copy link
Contributor

Hi @ahmedsameha1, can you resolve the merge conflicts?

Copy link
Contributor

@victorsanni victorsanni left a comment

Choose a reason for hiding this comment

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

Can you ensure the view resets to it's normal default value after this test? Otherwise other tests will be run with a zero-sized view which can cause unrelated test failures.

@ahmedsameha1 ahmedsameha1 force-pushed the handle#6537-DropdownMenu branch from e9bbe56 to e8a9959 Compare October 20, 2025 22:41
@victorsanni victorsanni added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 25, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Oct 25, 2025
Merged via the queue into flutter:master with commit 673023b Oct 25, 2025
77 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 27, 2025
flutter/flutter@cb18290...4c91098

2025-10-26 [email protected] Roll ICU from ff35c4f9df23 to f27805b7d7d8 (1 revision) (flutter/flutter#177558)
2025-10-26 [email protected] Roll Skia from b85c7ae19092 to de52b3a7585a (1 revision) (flutter/flutter#177550)
2025-10-25 [email protected] Make sure that a DropdownMenu doesn't crash in 0x0 environment (flutter/flutter#174809)
2025-10-25 [email protected] Fix DropdownMenu filtering is broken (flutter/flutter#177450)
2025-10-25 [email protected] Roll Skia from f352da2d607f to b85c7ae19092 (1 revision) (flutter/flutter#177539)
2025-10-25 [email protected] Roll Skia from e91d238ce638 to f352da2d607f (4 revisions) (flutter/flutter#177525)
2025-10-24 [email protected] Add more docs to TextBaseline (flutter/flutter#177507)
2025-10-24 [email protected] Bump Templates To Correct Versions (flutter/flutter#177416)
2025-10-24 [email protected] [web] Use SkPathBuilder because SkPath is becoming immutable (flutter/flutter#177343)

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] 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
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants