-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add an ActivateAction to controls that use InkWell. #41220
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
4a75801 to
e20b130
Compare
429315b to
2019bff
Compare
06429d0 to
b9070e2
Compare
b9070e2 to
e284296
Compare
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.
This override isn't needed anymore.
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 point. Removed!
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.
[we talked this over and decided to leave the API as is]
Just FTR: there's tension here between convenience and separation of concerns. We're exposing the most useful Focus widget parameters here and in other components for the sake of convenience. It's for the not uncommon case where the developer wants to be able to drive the focus directly to this inkwell (or button etc). Unfortunately that means blending the FocusWidget's parameters into other widget configurations. There doesn't seem to be a clean way to avoid this:
- A TransitionBuilder valued
buildFocusparameter could be used instead. It would be easy enough to have it return a widget that DTRT, but somewhat clunkier. - The developer could just wrap the component's child with a Focus widget. But that would create a nested Focus widget (under the one created by the component by default) which might not be what's wanted (Semantics).
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.
Using inheritFromWidgetOfExactType because we context to rebuilt if Actions changes?
If that's true then Actions probably needs an operator== and hashCode overloads so the framework can judge if the inherited widget has changed.
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.
Yeah, specifically if the action dispatcher that is set on the Actions widget has changed, or the actions list, I suppose.
I thought that was what updateShouldNotify was for, so I didn't define them.
Added.
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.
widgets should never have ==s
dee806e to
c2f72a2
Compare
HansMuller
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
…t the Focus, and add the top level binding to the ENTER key.
9758af3 to
370a125
Compare
This adds the default shortcuts and actions for keyboard-based focus traversal of apps. This list of shortcuts includes shortcuts for TAB, SHIFT TAB, RIGHT_ARROW, LEFT_ARROW, UP_ARROW, DOWN_ARROW, and the four DPAD keys for game controllers (because the DPAD produces arrow key events). It doesn't yet include functionality for triggering a control (e.g. SPACE, ENTER, or controller buttons), because that involves restructuring some of the Flutter controls to trigger animations differently, and so will be done in another PR (#41220)
This attempts to reland #40186 and #41220, that were reverted in #41945. The main modifications from the original PRs are that I predefine the shortcuts and actions maps instead of defining them inline in the build function, and I use a new mapEquals to do a deep comparison so that we don't rebuild modified things if the contents of the map haven't changed. I also eliminated an operator== and hashCode that were defined on the Actions widget, since widgets shouldn't have those. (it's too bad though: I get an 85% speedup if we leave this in! Too bad it prevents rebuilding of the children...) Fixes #40101
Adds an ActivateAction to controls that use InkWell. Make InkWell host the Focus for those controls, and add the top level binding to the ENTER key. This will make it possible to trigger a button using the enter key, and to get an ink response when the button is triggered. This is a breaking change because it moves the Focus widget into the InkWell. If you have a component that uses the InkWell directly and you used to wrap that InkWell in a Focus widget to give it its notion of focus, it will now not look for that Focus ancestor for its focus state anymore. In order to control focus on the InkWell, you need to give it a FocusNode directly, via the new focusNode parameter. This should not affect users of widgets like OutlineButton or FloatingActionButton and the like, since those have been modified in this PR.
This adds the default shortcuts and actions for keyboard-based focus traversal of apps. This list of shortcuts includes shortcuts for TAB, SHIFT TAB, RIGHT_ARROW, LEFT_ARROW, UP_ARROW, DOWN_ARROW, and the four DPAD keys for game controllers (because the DPAD produces arrow key events). It doesn't yet include functionality for triggering a control (e.g. SPACE, ENTER, or controller buttons), because that involves restructuring some of the Flutter controls to trigger animations differently, and so will be done in another PR (flutter#41220)
This attempts to reland flutter#40186 and flutter#41220, that were reverted in flutter#41945. The main modifications from the original PRs are that I predefine the shortcuts and actions maps instead of defining them inline in the build function, and I use a new mapEquals to do a deep comparison so that we don't rebuild modified things if the contents of the map haven't changed. I also eliminated an operator== and hashCode that were defined on the Actions widget, since widgets shouldn't have those. (it's too bad though: I get an 85% speedup if we leave this in! Too bad it prevents rebuilding of the children...) Fixes flutter#40101
Description
Adds an
ActivateActionto controls that useInkWell. MakeInkWellhost theFocusfor those controls, and add the top level binding to the ENTER key. This will make it possible to trigger a button using the enter key, and to get an ink response when the button is triggered.This is a breaking change because it moves the
Focuswidget into theInkWell. If you have a component that uses theInkWelldirectly and you used to wrap thatInkWellin aFocuswidget to give it its notion of focus, it will now not look for thatFocusancestor for its focus state anymore. In order to control focus on theInkWell, you need to give it aFocusNodedirectly, via the newfocusNodeparameter. This should not affect users of widgets likeOutlineButtonorFloatingActionButtonand the like, since those have been modified in this PR.So, if before you had:
You would now write:
Related Issues
Addresses #31946, #13264, #1608
Tests
Added tests that make sure sending the ENTER key triggers animations and the
onTapfor the control.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?