Repository navigation
Only dismiss dropdowns if the orientation changes, not the size. - #42482
Conversation
fluttergithubbot
added
p: material_ui
There was a problem hiding this comment.
Could this be changed to the following instead:
| if (MediaQuery.of(context).orientation != _lastOrientation) { | |
| if (newOrientation != _lastOrientation) { |
If I understand ??= correctly, this would help us avoid looking up MediaQuery.of(context).orientation twice
There was a problem hiding this comment.
Oh, good point. Fixed, and I also made it so that if there wasn't a MediaQuery ancestor, it falls back to the window orientation.
There was a problem hiding this comment.
I think this seems to simulate a keyboard showing (shrinking MediaQuery.size), but not a keyboard hide (MediaQuery.size regrowing to Size(800, 600).
There was a problem hiding this comment.
It could be helpful to add comments to describe what behaviors are being simulated
There was a problem hiding this comment.
OK, added a grow test, and comments for each one.
There was a problem hiding this comment.
When would Navigator.userGestureInProgressNotifier end up as null?
There was a problem hiding this comment.
It doesn't, it's the widget.route.navigator that is sometimes null, like in a test, or before the route is pushed.
ce087eb to
622257f
Compare
…tter#42482) This changes the DropDownButton so that instead of dismissing itself when any metrics change occurs, it only dismisses itself when the orientation changes. This gets around the fact that we can't currently have a dropdown and a text field on the same page because the keyboard disappearing when the dropdown gets focus causes a metrics change, and the dropdown immediately disappears when activated. It still will cause the keyboard to jump up and down between controls, but that's a larger issue. At least now we can use the two together again.
| final Orientation newOrientation = _getOrientation(context); | ||
| _lastOrientation ??= newOrientation; | ||
| if (newOrientation != _lastOrientation) { | ||
| _removeDropdownRoute(); |
There was a problem hiding this comment.
This shouldn't be called from build. I am surprised that this is actually working and doesn't throw an "setState during build is illegal" exception.
Can you not just check in the didChangeMetrics if the orientation has changed from the last time and if it did remove the route there?
There was a problem hiding this comment.
Actually, looks like we have special code in place to allow removing an overlay (like this dropdown route) during build: https://github.com/flutter/flutter/blame/166d422b05e69ea140fcdc8968282b883aad0af5/packages/flutter/lib/src/widgets/overlay.dart#L128
@Hixie Do you know why we allow this? (you added the code :P)
| void initState() { | ||
| super.initState(); | ||
| _updateSelectedIndex(); | ||
| WidgetsBinding.instance.addObserver(this); |
There was a problem hiding this comment.
if we never add ourself as an observer we can also remove the removal in the dispose method (and stop implementing the WidgetsBindingObserver interface.
5 participants
Description
This changes the
DropDownButtonso that instead of dismissing itself when any metrics change occurs, it only dismisses itself when the orientation changes.This gets around the fact that we can't currently have a dropdown and a text field on the same page because the keyboard disappearing when the dropdown gets focus causes a metrics change, and the dropdown immediately disappears when activated.
It still will cause the keyboard to jump up and down between controls, but that's a larger issue. At least now we can use the two together again.
Related Issues
Fixes #41881
Tests
Breaking Change