Skip to content
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

Add a Focus node to the DropdownButton, and adds an activation action for it. #42811

Merged
merged 2 commits into from Oct 18, 2019

Conversation

@gspencergoog
Copy link
Contributor

gspencergoog commented Oct 16, 2019

Description

This adds a Focus node to the DropdownButton widget, allowing it to receive keyboard focus, and to show a focus highlight. In addition, I added the ability to activate the dropdown using the "enter" key binding (which is bound to ActivateAction in the WidgetsApp).

Related Issues

Tests

  • Added test for checking newly added parameters,
  • Added test to make sure it can't receive focus when not enabled.
  • Added test to check that keyboard bindings do what is expected.

Breaking Change

  • No, this is not a breaking change.
@Joao-b4

This comment has been minimized.

Copy link

Joao-b4 commented Oct 18, 2019

Any way to already apply this pull? I tried to replace with the code of dropdown.dart but accused errors "Compiler failed"

@gspencergoog

This comment has been minimized.

Copy link
Contributor Author

gspencergoog commented Oct 18, 2019

You can try going to https://patch-diff.githubusercontent.com/raw/flutter/flutter/pull/42811.diff, save it to a file, and the run git apply (whatever the filename is)

@gspencergoog

This comment has been minimized.

Copy link
Contributor Author

gspencergoog commented Oct 18, 2019

There's also probably a more git-like way by cloning my branch of the flutter repo at https://github.com/gspencergoog/flutter/tree/dropdown_focus . No guarantee that I won't change it out from under you though. Caveat emptor.

Copy link
Contributor

darrenaustin left a comment

Other than the border radius comment, it LGTM.

@@ -1112,6 +1166,10 @@ class _DropdownButtonState<T> extends State<DropdownButton<T>> with WidgetsBindi
Widget result = DefaultTextStyle(
style: _textStyle,
child: Container(
decoration: BoxDecoration(
color:_hasPrimaryFocus ? widget.focusColor ?? Theme.of(context).focusColor : null,
borderRadius: const BorderRadius.all(Radius.circular(4.0)),

This comment has been minimized.

Copy link
@darrenaustin

darrenaustin Oct 18, 2019

Contributor

Is this addition of a border radius intentional? Is there something about the focus changes that needs a rounded rect? Or did this get moved from somewhere else that I missed?

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Oct 18, 2019

Author Contributor

Yes, it's intentional: there's no spec for this, so I'm emulating the other defaults for highlights. It didn't get moved from anywhere because it didn't exist before, since we couldn't focus this component.

@gspencergoog gspencergoog merged commit df76354 into flutter:master Oct 18, 2019
78 of 79 checks passed
78 of 79 checks passed
flutter-build Flutter build is currently broken. Please do not merge this PR unless it contains a fix to the broken build.
Details
add2app-macos Task Summary
Details
bots_tests-linux Task Summary
Details
bots_tests-macos Task Summary
Details
bots_tests-windows Task Summary
Details
tool_coverage-linux Task Summary
Details
tool_tests_commands-linux Task Summary
Details
tool_tests_commands-macos Task Summary
Details
tool_tests_commands-windows Task Summary
Details
tool_tests_general-linux Task Summary
Details
tool_tests_general-macos Task Summary
Details
tool_tests_general-windows Task Summary
Details
tool_tests_integration-linux Task Summary
Details
tool_tests_integration-windows Task Summary
Details
WIP Ready for review
Details
add2app-macos
Details
analyze Task Summary
Details
analyze
Details
bots_tests-linux
Details
bots_tests-macos
Details
bots_tests-windows
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
build_tests-windows Task Summary
Details
build_tests-windows
Details
cla/google All necessary CLAs are signed
customer_testing-linux Task Summary
Details
customer_testing-linux
Details
customer_testing-macos Task Summary
Details
customer_testing-macos
Details
customer_testing-windows Task Summary
Details
customer_testing-windows
Details
deploy_gallery Task Summary
Details
deploy_gallery
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs Task Summary
Details
docs
Details
integration_tests-linux Task Summary
Details
integration_tests-linux
Details
integration_tests-windows Task Summary
Details
integration_tests-windows
Details
integration_tests_gradle1-linux Task Summary
Details
integration_tests_gradle1-linux
Details
integration_tests_gradle1-windows Task Summary
Details
integration_tests_gradle1-windows
Details
integration_tests_gradle2-linux Task Summary
Details
integration_tests_gradle2-linux
Details
integration_tests_gradle2-windows Task Summary
Details
integration_tests_gradle2-windows
Details
release_smoke_tests Task Summary
Details
release_smoke_tests
Details
tests_extras-linux Task Summary
Details
tests_extras-linux
Details
tests_extras-windows Task Summary
Details
tests_extras-windows
Details
tests_framework_other-linux Task Summary
Details
tests_framework_other-linux
Details
tests_framework_other-windows Task Summary
Details
tests_framework_other-windows
Details
tests_widgets-linux Task Summary
Details
tests_widgets-linux
Details
tests_widgets-windows Task Summary
Details
tests_widgets-windows
Details
tool_coverage-linux
Details
tool_tests_commands-linux
Details
tool_tests_commands-macos
Details
tool_tests_commands-windows
Details
tool_tests_general-linux
Details
tool_tests_general-macos
Details
tool_tests_general-windows
Details
tool_tests_integration-linux
Details
tool_tests_integration-windows
Details
web_tests-linux-shard-0 Task Summary
Details
web_tests-linux-shard-0
Details
web_tests-linux-shard-1 Task Summary
Details
web_tests-linux-shard-1
Details
web_tests-linux-shard-2 Task Summary
Details
web_tests-linux-shard-2
Details
@gspencergoog

This comment has been minimized.

Copy link
Contributor Author

gspencergoog commented Oct 18, 2019

@Joao-b4, no need to patch it in anymore: it's landed on master.

@Joao-b4

This comment has been minimized.

Copy link

Joao-b4 commented Oct 21, 2019

@gspencergoog , testing the update of the master branch, I realized that it is now possible to change the focus straight to the dropdown, not keeping the user stuck, as I show in the gif, but there is this small and fast bug, where clicking the dropdown it focuses on textFormField (maxlines: 5) for one second and focus returns to the dropdown then.
I haven't modified problem code #43008, but I don't know if there's anything related to the code.

ezgif com-video-to-gif (1)

@gspencergoog

This comment has been minimized.

Copy link
Contributor Author

gspencergoog commented Oct 21, 2019

@Joao-b4 sorry, maybe I'm just missing it in the animation, but I don't see what the bug is. Can you explain what you expected to happen, and what's happening instead?

@Joao-b4

This comment has been minimized.

Copy link

Joao-b4 commented Oct 21, 2019

@gspencergoog
I will list the gif steps that the problem occurs.

1.click on textFormField (maxLines: 5).
2.click on textFormField.
3.click done.
4.click on the dropdown.

The moment I click the dropdown, textFormField (maxLines: 5) gains focus for 1 second, and then the focus returns to the dropdown.

I believe that by clicking on the dropdown the focus should go directly to it.

@gspencergoog

This comment has been minimized.

Copy link
Contributor Author

gspencergoog commented Oct 21, 2019

Oh, I see: you mean that short (just a couple of frames) flash when the first text field appears to receive focus when you click on the dropdown, right? OK, can you file an issue for that and we'll take a look? Just copy what you posted above into an issue.

@escamoteur

This comment has been minimized.

Copy link
Contributor

escamoteur commented Oct 23, 2019

@gspencergoog I tried it today with my sample app and I have the effect that after selecting the DropDown box the focus jumps back to the last active Textfield which is wrong IMHO.
The DropDown should keep the focus.

@gspencergoog gspencergoog deleted the gspencergoog:dropdown_focus branch Nov 13, 2019
Inconnu08 added a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
… for it. (flutter#42811)

This adds a Focus node to the DropdownButton widget, allowing it to receive keyboard focus, and to show a focus highlight. In addition, I added the ability to activate the dropdown using the "enter" key binding (which is bound to ActivateAction in the WidgetsApp).

Related Issues
Fixes flutter#42646
Fixes flutter#43008
Fixes flutter#42511
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.