Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Sep 24, 2019

Description

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.

So, if before you had:

  Focus(
    focusNode: focusNode,
    child: Container(
      width: 100,
      height: 100,
      child: InkWell(
        onTap: () {},
      ),
    ),
  ),

You would now write:

  Container(
    width: 100,
    height: 100,
    child: InkWell(
      focusNode: focusNode,
      onTap: () {},
    ),
  ),

Related Issues

Addresses #31946, #13264, #1608

Tests

Added tests that make sure sending the ENTER key triggers animations and the onTap for 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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.
  • I checked all the boxes!

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Sep 24, 2019
@gspencergoog gspencergoog marked this pull request as ready for review September 25, 2019 20:01
@gspencergoog gspencergoog force-pushed the activate_action branch 3 times, most recently from 429315b to 2019bff Compare September 25, 2019 21:08
@gspencergoog gspencergoog added c: API break Backwards-incompatible API changes and removed work in progress; do not review labels Sep 26, 2019
@gspencergoog gspencergoog force-pushed the activate_action branch 2 times, most recently from 06429d0 to b9070e2 Compare September 26, 2019 17:28
@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 27, 2019
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Removed!

Copy link
Contributor

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 buildFocus parameter 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).

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

@gspencergoog gspencergoog merged commit 4512a1c into flutter:master Oct 2, 2019
gspencergoog added a commit that referenced this pull request Oct 2, 2019
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)
@Hixie
Copy link
Contributor

Hixie commented Oct 3, 2019

#41919

@gspencergoog gspencergoog deleted the activate_action branch October 9, 2019 18:05
gspencergoog added a commit that referenced this pull request Oct 10, 2019
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
@Hixie Hixie mentioned this pull request Oct 14, 2019
54 tasks
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
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.
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
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)
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: API break Backwards-incompatible API changes c: contributor-productivity Team-specific productivity, code health, technical debt. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants