Skip to content

Do not rely on Leader/Follower to position DropdownMenu menu #158930

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

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Nov 14, 2024

Description

This PR removes DropdownMenu usage of Leader/Follower.

Leader/Follower positioning was introduced in #154667 which uses Leader/Follower approach to fix some weird positioning issues (such as #149037).

Unfortunately, it also introduces some regressions, see:

Because #154667 is already included in the beta channel, cherry-picking this PR should be considered.

Context

This PR is not a full revert and keeps Leader/Follower usage in MenuAnchor because this usage is optional and doesn't cause any regression.
There are some ongoing work which might fix or mitigate this problem:

Related Issue

Fixes #157916
Fixes #158924

Reopens #123395
Reopens #149037
Reopens #151856

@bleroux bleroux requested a review from justinmc November 14, 2024 15:30
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 14, 2024
@bleroux bleroux requested a review from gspencergoog November 14, 2024 15:31
@bleroux bleroux mentioned this pull request Nov 14, 2024
9 tasks
@gspencergoog
Copy link
Contributor

Seems like there is a tradeoff here, and we need to decide which set of bugs is more detrimental to the user.

The regressions look like they are annoying, but the issues that this reopens are also pretty bad: they hide the dropdown on a mobile device because the keyboard covers up the menu, and the menu gets disconnected from the anchor when scrolling.

Are you expecting that the two other PRs referenced will fix the reopened issues, or are they just related?

Also, the "Fixes" section in this PR description seem to refer to the PRs, not the bugs, so you might want to update the numbers there.

@bleroux
Copy link
Contributor Author

bleroux commented Nov 14, 2024

Thanks for the feedback 🙏

(I have edited the description to fix the two links in the "fixes" section, thank for noticing this).

Seems like there is a tradeoff here, and we need to decide which set of bugs is more detrimental to the user.

Yes, there is a tradeoff. The main reason I filed this PR is that we are probably close to a stable release and I think this has an impact on the decision.

Are you expecting that the two other PRs referenced will fix the reopened issues, or are they just related?

One of the PR,, is a direct attempt to fix the first regression. But it is still a draft and It might take time to discuss and review this solution and it probably needs more work because being able to move the menu above the field is a good first step but not as smart as the current positioning logic that MenuAnchor offers when not using Leader/Follower.

The second PR introduces RawMenuAnchor in the widget library, I don't think it will fix the reopened issues. But it might help. It is a promising and huge PR so it might takes time to review it.

In summary, there are pending work that will help to fix the referenced regressions, but the corresponding PRs might not be ready for the next stable release.

@justinmc
Copy link
Contributor

Option 1: Close this PR

Downside: We'll be stuck with the following two issues. What would be the long term plan for addressing these if we go with this option?

#157916: DropdownMenu menu always appears below the TextField
A bad one because Dropdowns at or near the bottom of the screen are unusable on all platforms. The workaround would be to move the Dropdown.

#158924: Cannot scroll to the bottom of DropdownMenu
Also a bad one because latter menu items are not clickable. It seems to affect all platforms. Affects only longer lists, but that's probably common (in the issue, a Dropdown in the center of the screen with 8 items repros the bug). Workaround would just be to keep the list shorter.

Option 2: Merge this PR

The above two issues will be fixed, but the following ones will be reopened. They might be fixed with work in progress in #157921 and #158924, but those probably won't be done by the next release.

#123395: Menu appears behind the keyboard
Android only, doesn't happen all the time. The user can work around the problem by closing the keyboard. Admittedly though, it looks very bad.

#149037: DropdownMenu Entries Not Positioned Correctly on Keyboard Appearance
Looks to me like a duplicate of the previous issue.

#151856: DropdownMenu doesn't follow in scrollable
All platforms. Affects any Dropdown in a Scrollable, which is probably a good amount. The user can work around the problem by closing and reopening the menu and not scrolling. Looks bad.

So what should we do?

It looks to me like option 2 gets us in a better state. The issues we'd have are ugly but not app-breaking. Also, we seem to have a potential path forward to fixing them with #157921 and #158924. I say we merge this PR and then follow up on making sure those issues get fixed.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

See my thoughts in the previous comment. I'm definitely open to being corrected but I think merging this is the best of two imperfect options.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Works for me. I just wanted it to be a reasoned choice.

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 23, 2024
@auto-submit auto-submit bot added this pull request to the merge queue Nov 23, 2024
Merged via the queue into flutter:master with commit 773b42f Nov 23, 2024
79 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 23, 2024
@justinmc
Copy link
Contributor

@itsjustkevin The two PRs that this reverts (#157916 and #158924) have already rolled into beta, so this PR should be cherry picked into beta if possible.

@justinmc justinmc added the cp: beta cherry pick this pull request to beta release candidate branch label Nov 25, 2024
flutteractionsbot pushed a commit to flutteractionsbot/flutter that referenced this pull request Nov 25, 2024
…#158930)

## Description

This PR removes `DropdownMenu` usage of Leader/Follower.

Leader/Follower positioning was introduced in
flutter#154667 which uses
Leader/Follower approach to fix some weird positioning issues (such as
flutter#149037).

Unfortunately, it also introduces some regressions, see:
- flutter#157916
- flutter#158924

Because flutter#154667 is already
included in the beta channel, cherry-picking this PR should be
considered.

## Context

This PR is not a full revert and keeps Leader/Follower usage in
`MenuAnchor` because this usage is optional and doesn't cause any
regression.
There are some ongoing work which might fix or mitigate this problem:
- flutter#157921
- flutter#158255

## Related Issue

Fixes flutter#157916
Fixes flutter#158924

Reopens flutter#123395
Reopens flutter#149037
Reopens flutter#151856
@bleroux bleroux deleted the do_not_rely_on_leader_follower_to_position_dropdown_menu branch November 26, 2024 07:14
christopherfujino pushed a commit that referenced this pull request Dec 10, 2024
…#159436)

This pull request is created by [automatic cherry pick
workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request)
Please fill in the form below, and a flutter domain expert will evaluate
this cherry pick request.

### Issue Link:
What is the link to the issue this cherry-pick is addressing?

#157916 and
#158924

### Changelog Description:
Explain this cherry pick in one line that is accessible to most Flutter
developers. See [best
practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md)
for examples

Restore the previous dropdown menu positioning logic.

### Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot
ship an iOS app)? Does it impact development (ex. flutter doctor crashes
when Android Studio is installed), or the shipping production app (the
app crashes on launch)

DropdownMenu menu is misplaced and can be fully or partially hidden.
See
#158930 (comment)
for more context.

### Workaround:
Is there a workaround for this issue?

No

### Risk:
What is the risk level of this cherry-pick?

  - [x] Low
  - [ ] Medium
  - [ ] High

### Test Coverage:
Are you confident that your fix is well-tested by automated tests?

  - [x] Yes
  - [ ] No

### Validation Steps:
What are the steps to validate that this fix works?

< Replace with validation steps here >

Co-authored-by: Bruno Leroux <[email protected]>
@reidbaker reidbaker mentioned this pull request Dec 13, 2024
11 tasks
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp: beta cherry pick this pull request to beta release candidate branch f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
3 participants