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

FocusableActionDetector widget #44867

Merged
merged 4 commits into from Nov 19, 2019

Conversation

@gspencergoog
Copy link
Contributor

gspencergoog commented Nov 14, 2019

Description

This adds a FocusableActionDetector, a widget which combines the functionality of Actions, Shortcuts, MouseRegion and a Focus widget to create a detector that defines actions and key bindings, and will notify that the focus or hover highlights should be shown or not. This widget can be used to give a control the required detection modes for focus and hover handling on desktop and web platforms.

I replaced a bunch of similar code in many of our widgets with this widget, and found that pretty much any control that wants to be focusable wants all of these features as well: focus highlights, hover highlights, and actions to activate it.

Also eliminated an extra _hasFocus variable in FocusState that wasn't being used.

Tests

  • Added tests for the new widget.

Breaking Change

  • No, this is not a breaking change.
@gspencergoog gspencergoog requested a review from darrenaustin Nov 14, 2019
@googlebot googlebot added the cla: yes label Nov 14, 2019
@gspencergoog gspencergoog requested a review from HansMuller Nov 14, 2019
@gspencergoog gspencergoog changed the title Add FocusableActionDetector FocusableActionDetector widget Nov 14, 2019
Copy link
Contributor

darrenaustin left a comment

Couple of minor comments, but LGTM.

packages/flutter/lib/src/widgets/actions.dart Outdated Show resolved Hide resolved
/// provides focus and hover capabilities.
///
/// It hosts its own [FocusNode] or uses [focusNode], if given.
class FocusableActionDetector extends StatefulWidget {

This comment has been minimized.

Copy link
@darrenaustin

darrenaustin Nov 14, 2019

Contributor

I like the concept, but don't love the name FocusableActionDetector. However, as all the suggestions I could come up with are worse, perhaps I should just keep this to myself :).

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Nov 14, 2019

Author Contributor

Yeah, I don't love the name either. It's the best I could come up with, though: it has to do with focus, actions, shortcuts, and hover, and it doesn't have a visual representation. FocusableShortcutActionsAndHoverDetector seemed like a mouthful. :-)

@gspencergoog gspencergoog force-pushed the gspencergoog:focusable_action_detector branch from 2d99ada to d50a945 Nov 14, 2019
@gspencergoog gspencergoog requested a review from goderbauer Nov 14, 2019
Copy link
Member

goderbauer left a comment

LGTM with some doc nits.

/// and key bindings, and provides callbacks for handling focus and hover
/// highlights.
///
/// This widget can be used to give a control the required detection modes for

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

"... can be used to give a control the required ..."

Something seems to be missing here...

assert(child != null),
super(key: key);

/// Is this widget enabled or not.

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

nit: I believe we usually say "Whether this widget is enabled or not"


/// Is this widget enabled or not.
///
/// If disabled, will not send any notifications needed to update highlight or

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

This sentence may miss one or two "it" to be a complete sentence?

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

Same in the next paragraph?

assert(widget.onShowHoverHighlight != null);
if (!_hovering) {
// TODO(gspencergoog): remove scheduleMicrotask once MouseRegion event timing has changed.
scheduleMicrotask(() { setState(() { _hovering = true; _handleShowHoverHighlight(); }); });

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

nit: maybe format this in multiple lines to make it more readable? Same below.

child: widget.child,
),
);
if (widget.enabled && widget.actions != null && widget.actions.isNotEmpty) {

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

Changing the shape of the widget tree (e.g. adding/removing a widget in the middle) is somewhat expansive. Is there a way to always include the Actions and Shortcuts widget, but have it "disabled" if its functionality is not required?

/// highlights.
///
/// This widget can be used to give a control the required detection modes for
/// focus and hover handling.

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

I wish this doc would say a little more about when and how to use this widget.

SemanticsFlag.hasCheckedState,
SemanticsFlag.hasEnabledState,
SemanticsFlag.isFocusable,

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

Uh, we were marking a disabled radio button as focusable before? It is correct, that a disabled element is not focusable, right?

},
),
),
return FocusableActionDetector(

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

Nice! The new Widget makes this code cleaner!


/// A function that will be called when the focus changes.
///
/// Called with true if the [focusNode] has primary focus.

This comment has been minimized.

Copy link
@goderbauer

goderbauer Nov 18, 2019

Member

Can this link to something that explains what "primary focus" is?

gspencergoog added 4 commits Nov 1, 2019
@gspencergoog gspencergoog force-pushed the gspencergoog:focusable_action_detector branch from d50a945 to 64ade3f Nov 19, 2019
@gspencergoog gspencergoog merged commit 6e10719 into flutter:master Nov 19, 2019
63 checks passed
63 checks passed
WIP Ready for review
Details
analyze-linux Task Summary
Details
analyze-linux
Details
build_tests-linux Task Summary
Details
build_tests-linux
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-linux Task Summary
Details
deploy_gallery-linux
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs-linux Task Summary
Details
docs-linux
Details
firebase_test_lab_tests-linux Task Summary
Details
firebase_test_lab_tests-linux
Details
flutter-build
Details
framework_tests-libraries-linux Task Summary
Details
framework_tests-libraries-linux
Details
framework_tests-libraries-macos Task Summary
Details
framework_tests-libraries-macos
Details
framework_tests-libraries-windows Task Summary
Details
framework_tests-libraries-windows
Details
framework_tests-misc-linux Task Summary
Details
framework_tests-misc-linux
Details
framework_tests-misc-macos Task Summary
Details
framework_tests-misc-macos
Details
framework_tests-misc-windows Task Summary
Details
framework_tests-misc-windows
Details
framework_tests-widgets-linux Task Summary
Details
framework_tests-widgets-linux
Details
framework_tests-widgets-macos Task Summary
Details
framework_tests-widgets-macos
Details
framework_tests-widgets-windows Task Summary
Details
framework_tests-widgets-windows
Details
hostonly_devicelab_tests-0-linux Task Summary
Details
hostonly_devicelab_tests-0-linux
Details
hostonly_devicelab_tests-1-linux Task Summary
Details
hostonly_devicelab_tests-1-linux
Details
hostonly_devicelab_tests-2-linux Task Summary
Details
hostonly_devicelab_tests-2-linux
Details
hostonly_devicelab_tests-3_last-linux Task Summary
Details
hostonly_devicelab_tests-3_last-linux
Details
web_tests-0-linux Task Summary
Details
web_tests-0-linux
Details
web_tests-1-linux Task Summary
Details
web_tests-1-linux
Details
web_tests-2-linux Task Summary
Details
web_tests-2-linux
Details
web_tests-3-linux Task Summary
Details
web_tests-3-linux
Details
web_tests-4-linux Task Summary
Details
web_tests-4-linux
Details
web_tests-5-linux Task Summary
Details
web_tests-5-linux
Details
web_tests-6-linux Task Summary
Details
web_tests-6-linux
Details
web_tests-7_last-linux Task Summary
Details
web_tests-7_last-linux
Details
@gspencergoog gspencergoog deleted the gspencergoog:focusable_action_detector branch Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.