-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Hide autocomplete menu on select. #100251
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
Hide autocomplete menu on select. #100251
Conversation
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| return; | ||
| } | ||
|
|
||
| _floatingOptions?.remove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call pulling this out of the conditionals.
| // Hide or show the options overlay, if needed. | ||
| void _updateOverlay() { | ||
| _setActionsEnabled(_focusNode.hasFocus && _selection == null && _options.isNotEmpty); | ||
| if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.persistentCallbacks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the problem was that _updateOverlay was called on a tap callback, in the persistentCallbacks phase, and so the OverlayEntry was updated for the previous frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tap callbacks should never be called in persistent callbacks.
The problem was if the text isn't changed by the tap, it's not going to call _updateOverlay so that shouldShowOptions isn't getting re-evaluated.
This is just a small change to make sure the caller of _updateOverlay doesn't have to know whether it's safe to do an overlay update right away or it has to schedule an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, sounds good.
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.