Fix DropdownButton extra padding when alignDropdown: true #131183

OctavianMihaila · 2023-07-24T10:53:05Z

Before:

After:

Removed that unnecessary padding from dropdown button's label by chaning this line in dropdown.dart:
const EdgeInsetsGeometry _kAlignedButtonPadding = EdgeInsetsDirectional.only(start: 16.0, end: 4.0);
to:
const EdgeInsets _kAlignedButtonPadding = EdgeInsets.zero;

Also, there was a test that verifies this padding's presence. I decided to modify it so that now it verifies if there is no padding (not sure if this was a good approach).

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.

…cific test updated

google-cla · 2023-07-24T10:53:08Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

QuncCccccc · 2023-08-08T17:57:34Z

Hello! Thanks a lot for the contribution:) Seems there are some test failures in the presubmit checks. Could you help fix this?

OctavianMihaila · 2023-08-09T13:47:08Z

Hello! Thanks a lot for the contribution:) Seems there are some test failures in the presubmit checks. Could you help fix this?

Hello :), I've investigated the test failures and solved some of them. There is still one that causes all the failings due to the fact that it is checking if the DropdownButton's label is aligned with the first item from the DropdownMenuItem. When that extra padding was present, it was aligned. So I think that after removal of the padding, parts of the test are no longer required. But I'm not sure if this test modification approach is good. Shall I do it ?

Those are the lines in the test that check the alignment:

flutter/packages/flutter/test/material/button_theme_test.dart

Line 154 in d78b3ac

expect(tester.getRect(fooText.at(0)), tester.getRect(fooText.at(1)));

flutter/packages/flutter/test/material/button_theme_test.dart

Line 172 in d78b3ac

expect(tester.getRect(fooText.at(0)), tester.getRect(fooText.at(1)));

Also, the differences:

The following TestFailure was thrown running a test:
Expected: Rect:<Rect.fromLTRB(316.0, 288.0, 365.0, 312.0)>
Actual: Rect:<Rect.fromLTRB(300.0, 288.0, 349.0, 312.0)>

flutter-dashboard · 2023-08-29T13:56:21Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Hixie · 2023-11-14T23:56:29Z

Hello :), I've investigated the test failures and solved some of them. There is still one that causes all the failings due to the fact that it is checking if the DropdownButton's label is aligned with the first item from the DropdownMenuItem. When that extra padding was present, it was aligned. So I think that after removal of the padding, parts of the test are no longer required. But I'm not sure if this test modification approach is good. Shall I do it ?

Sorry for the delay responding, we seem to have dropped the ball on this PR.

Fundamentally, for each failing test we (you) have to carefully examine it and decide whether it's a bug in the test (i.e. the expectation was wrong and only passed because the old code was wrong, which the PR is fixing), or a regression caused by the PR (i.e. the expectation is correct, the old code was correct, and the PR is introducing a bug). One way to do this is to use flutter run to run the tests, to see what they look like.

QuncCccccc · 2023-11-15T01:30:48Z

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

 const double _kDenseButtonHeight = 24.0;
 const EdgeInsets _kMenuItemPadding = EdgeInsets.symmetric(horizontal: 16.0);
-const EdgeInsetsGeometry _kAlignedButtonPadding = EdgeInsetsDirectional.only(start: 16.0, end: 4.0);
+const EdgeInsets _kAlignedButtonPadding = EdgeInsets.zero;


Hi! Just take another look, if we change this to EdgeInsets.zero. There will be no difference between _kAlignedButtonPadding and _kUnalignedButtonPadding . And with this change, it seems the padding would be useless in the Container on line 1481 because it will always be EdgeInsets.zero. Maybe we can just remove them all (The padding constants, padding variable and the padding in the Container).

I see another closed PR(#125193) by @Renzo-Olivares, so just wonder if he has any insights about this issue.

Hi I took a quick look. I think completely removing the padding breaks alignedDropdown for something like this

ButtonTheme( alignedDropdown: true, child: DropdownButton<String>( onChanged: (String? value) { }, value: 'foo', items: const <DropdownMenuItem<String>>[ DropdownMenuItem<String>( value: 'foo', child: Text('foo'), ), DropdownMenuItem<String>( value: 'bar', child: Text('bar'), ), ], ), ),

I haven't been able to figure out why, but I haven't noticed any differences when turning this padding off specifically for DropdownButtonFormField like in my pr #125193 . I think this might be the safest route if we want to fix it this way since it won't affect the other DropdownButton.

I see alignedDropdown was introduced in #14849 by @HansMuller, so maybe he has more insight on this.

It's true that I introduced #14849 five years ago :-). Although the after screenshot in the description does look like an improvement, changing the padding now would likely break many golden image tests and some layouts.

I see. Turning off the padding only for DropdownButtonFormField makes sense to me. @OctavianMihaila Maybe we can fix the issue in this way?

bambinoua · 2023-12-15T11:30:23Z

What about this PR? When do you think to merge it?

Renzo-Olivares · 2024-02-28T01:02:11Z

Hi @OctavianMihaila do you still plan on working on this PR? I made a suggestion regarding disabling the padding only in the case of DropdownButtonFormField, #131183 (comment) which may help clear up some of the failing tests / lessen the amount of layouts and Golden's affected.

goderbauer · 2024-04-30T22:50:22Z

(triage) I am going to close this one since there was no follow up. Feel free to reopen it if you find the time to work on this again. Thank you.

dropdown padding removed in case of alignedDropdown: true and the spe…

9d5c8b8

…cific test updated

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 24, 2023

HansMuller requested a review from QuncCccccc July 28, 2023 22:23

OctavianMihaila added 2 commits August 8, 2023 22:12

Removed trailing spaces in dropdown_test.dart

c9cf046

Merge remote-tracking branch 'upstream/master' into fix131055

82bd5db

TahaTesser mentioned this pull request Aug 17, 2023

The behaviour of DropdownButtonFormField padding is weird depending of settings #123783

Closed

QuncCccccc reviewed Nov 15, 2023

View reviewed changes

goderbauer closed this Apr 30, 2024

bleroux mentioned this pull request Feb 6, 2025

Fix DropdownButtonFormField padding when ButtonTheme.alignedDropdown is true #162810

Merged

ahyangnb mentioned this pull request Feb 7, 2025

[new] Fix DropdownButton extra padding when alignDropdown: true #162860

Closed

8 tasks

Fix DropdownButton extra padding when alignDropdown: true #131183

Fix DropdownButton extra padding when alignDropdown: true #131183

Uh oh!

Conversation

OctavianMihaila commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

google-cla bot commented Jul 24, 2023

Uh oh!

QuncCccccc commented Aug 8, 2023

Uh oh!

OctavianMihaila commented Aug 9, 2023

Uh oh!

flutter-dashboard bot commented Aug 29, 2023

Uh oh!

Hixie commented Nov 14, 2023

Uh oh!

QuncCccccc Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Nov 16, 2023 • edited by HansMuller Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

bambinoua commented Dec 15, 2023

Uh oh!

Renzo-Olivares commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

OctavianMihaila commented Jul 24, 2023 •

edited

Loading

Renzo-Olivares Nov 16, 2023 •

edited by HansMuller

Loading

Renzo-Olivares commented Feb 28, 2024 •

edited

Loading