Add MediaQueryData.navigationMode and allow controls to be focused when disabled. by gspencergoog · Pull Request #54919 · flutter/flutter

gspencergoog · 2020-04-16T01:48:13Z

Description

This adds a new navigationMode to the MediaQueryData that indicates how focusable controls should behave under different navigation modes, currently with two modes: NavigationMode.traditional, and NavigationMode.directional.

It may seem like focusing a disabled control is not desirable, but this is useful for user interfaces that use DPAD navigation because if a control gets disabled, losing focus is disruptive to the user, and it is difficult to control where the focus will end up unless it is done explicitly.

Related Issues

Fixes Allow controls to remain focusable while in disabled state #54877

Tests

Added tests for IconButton, InkResponse, PopupMenuButton, TextField, and FocusableActionDetector.

Breaking Change

No, this is not a breaking change.

packages/flutter/lib/src/widgets/basic.dart

goderbauer · 2020-04-29T18:18:37Z

packages/flutter/lib/src/material/input_decorator.dart

I don't fully understand the changes in this file. If the widget is focused, there's no longer a way to tell from its visuals whether it's enabled or not? Is this in line with material spec?

/cc @HansMuller

No, there's definitely no Material spec for this.

That's something I think I'll be asking the customer to push for, since this is a use case that probably needs some design work.

But you can usually tell if it is disabled: all the hints and labels are shown as "disabled", and it acts like a read-only field. Of course, if you don't have any hints or labels, then no, you can't tell.

In order to provide that, we'll have to add a disabledFocusColor or something. Waiting for some more spec guidance before doing that seems like a good idea.

goderbauer · 2020-04-29T18:19:20Z

packages/flutter/lib/src/material/popup_menu.dart

(side node: I really wish we would have called this property "focusable" instead of "canRequestFocus")

Yeah, me to. It's already in our "I wish we could make a breaking change for this" issue. I mean, I'm happy to make a breaking change if we think it is important enough, but it would be just a name change.

packages/flutter/lib/src/material/text_field.dart

goderbauer · 2020-04-29T19:00:06Z

What's the reason to go with the NavigationModality over a new TargetPlatform now?

Also, my understanding from the discord discussion with @Hixie was that if we do an inherited widget, widgets that use it should have a flag to allow overriding the inherited behavior, similarly to the various textDirection flags.

justinmc

LGTM from a TextField point of view.

packages/flutter/lib/src/material/input_decorator.dart

packages/flutter/lib/src/widgets/basic.dart

gspencergoog · 2020-04-29T21:17:55Z

What's the reason to go with the NavigationModality over a new TargetPlatform now?

Also, my understanding from the discord discussion with @Hixie was that if we do an inherited widget, widgets that use it should have a flag to allow overriding the inherited behavior, similarly to the various textDirection flags.

My reasoning for having this new inherited widget was to provide for a layered design that would avoid complecting the idea of a navigation mode with a target platform. If we did decide to add a new target platform, then this would be one aspect of implementing the default for that platform.

That said, I spoke with @Hixie some more, and he said that we need to decide whether this navigation paradigm is an attribute of the platform, an attribute of the design language, or just a mode that we enter when people do any kind of directional navigation.

My thought in this PR was to make it be a design decision for the developer: you apply this inherited widget when you design an app (or part of an app) to use directional navigation, making it a conscious design choice, but it could be that we want to make the default sensitive to the platform.

In our discussion, he also pointed out that if it's a platform attribute that can be overridden, that the MediaQuery inherited widget serves this purpose for other platform attributes already.

I'm going to talk to the customer and see if they can describe more about their use case, and see if they can get some Material Design guidance for us.

gspencergoog · 2020-05-20T18:29:39Z

After speaking with the customer, it seems that this is a design choice that they would like to make on multiple platforms, even ones where directional isn't the primary navigation method.

I need to get more details from the Material Design team still about whether or not we need an entirely new mode for controls that can be focused while disabled or not, how that modifies controls like text fields and sliders, and how it affects semantics of those widgets.

In the meantime, in order to allow the customer to make progress, I'd like to continue with this PR.

I've moved the navigationMode setting to be an attribute on MediaQuery, and set the default to traditional. Developers can override it to directional on apps where they would like to use directional navigation this way, and if/when we add engine support for platforms where directional navigation is the primary mode, it can default to directional on those platforms.

gspencergoog · 2020-05-20T18:32:46Z

One thing that might make sense is to move the NavigationMode enum to dart:ui in an engine PR before landing this, since we anticipate setting the default from the platform in the future, it might be nice to avoid the migration needed to get it there that would be necessary if we put it here initially.

goderbauer

The implementation LGTM from a technical point of view. I am unsure if we are given the material widgets the right visual treatment in this new mode when they are disabled. Did you get some guidance from the Material team on that? Changing this later may be an annoying breaking change (for customers that use screenshot tests).

goderbauer · 2020-05-26T22:41:38Z

packages/flutter/lib/src/material/ink_well.dart

material cannot be imported from within material.

Stupid IntelliJ! I've tried turning it off a bunch of times, but it insists on inserting imports when it shouldn't (I didn't add this import manually).

goderbauer · 2020-05-26T22:49:30Z

packages/flutter/lib/src/widgets/media_query.dart

nit: Maybe add a one-sentence example describing what you would do differently depending on the mode. I think that would increase the understanding of what this property should be used for.

Good idea. I added a small paragraph about this.

goderbauer · 2020-05-26T22:50:07Z

packages/flutter/lib/src/widgets/media_query.dart

update comment to remove reference to NavigationModality?

Whoops. Fixed.

goderbauer · 2020-05-26T22:53:37Z

Also: the analyzer is unhappy.

goderbauer · 2020-05-26T22:54:23Z

Can you maybe also update the PR description for future archaeologists? I was at first confused when I read about a NavigationModality widget in there and then didn't find it in the code :)

Update other widgets to remain focused when disabled if navigation mode is NavigationMode.directional.

…on MediaQuery

Hixie · 2020-05-28T19:57:18Z

packages/flutter/lib/src/widgets/media_query.dart

+  /// Some behaviors are also affected by this mode. For instance, disabled
+  /// controls will retain focus when disabled, and will be able to receive
+  /// focus (although they remain disabled) when traversed.
+  directional,


we should probably mention that this is for TVs.

gspencergoog added framework flutter/packages/flutter repository. See also f: labels. customer: fun (g3) labels Apr 16, 2020

fluttergithubbot added f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Apr 16, 2020

googlebot added the cla: yes label Apr 16, 2020

gspencergoog removed cla: yes labels Apr 16, 2020

googlebot added the cla: yes label Apr 16, 2020

gspencergoog added work in progress; do not review and removed cla: yes f: material design flutter/packages/flutter/material repository. labels Apr 16, 2020

googlebot added the cla: yes label Apr 16, 2020

gspencergoog removed the c: contributor-productivity Team-specific productivity, code health, technical debt. label Apr 16, 2020

fluttergithubbot added f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 16, 2020

gspencergoog changed the title ~~[WIP] Adding a ThemeData option to allow controls to be focused when disabled.~~ [WIP] Adding a NavigationModality InheritedWidget to allow controls to be focused when disabled. Apr 16, 2020

gspencergoog force-pushed the focus_disabled branch 3 times, most recently from 91781b1 to dae6393 Compare April 17, 2020 01:22

gspencergoog marked this pull request as ready for review April 17, 2020 01:23

gspencergoog removed c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Apr 17, 2020

gspencergoog changed the title ~~[WIP] Adding a NavigationModality InheritedWidget to allow controls to be focused when disabled.~~ Adding a NavigationModality InheritedWidget to allow controls to be focused when disabled. Apr 17, 2020

gspencergoog requested a review from goderbauer April 17, 2020 01:25

flutter deleted a comment from skia-gold Apr 17, 2020

gspencergoog changed the title ~~Adding a NavigationModality InheritedWidget to allow controls to be focused when disabled.~~ Add NavigationModality widget and allow controls to be focused when disabled. Apr 17, 2020

gspencergoog force-pushed the focus_disabled branch from dae6393 to c791b07 Compare April 17, 2020 20:58

gspencergoog force-pushed the focus_disabled branch from c791b07 to 67d03fd Compare April 29, 2020 16:52

goderbauer reviewed Apr 29, 2020

View reviewed changes

justinmc reviewed Apr 29, 2020

View reviewed changes

packages/flutter/lib/src/material/input_decorator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/basic.dart Outdated Show resolved Hide resolved

gspencergoog force-pushed the focus_disabled branch from 67d03fd to d54b948 Compare May 1, 2020 21:00

gspencergoog force-pushed the focus_disabled branch 2 times, most recently from 522cca0 to 21cc0d4 Compare May 20, 2020 18:15

gspencergoog force-pushed the focus_disabled branch from 21cc0d4 to 3767961 Compare May 20, 2020 18:30

gspencergoog mentioned this pull request May 20, 2020

Allow controls to remain focusable while in disabled state #54877

Closed

gspencergoog changed the title ~~Add NavigationModality widget and allow controls to be focused when disabled.~~ Add MediaQueryData attribute and allow controls to be focused when disabled. May 20, 2020

gspencergoog changed the title ~~Add MediaQueryData attribute and allow controls to be focused when disabled.~~ Add MediaQueryData.navigationMode and allow controls to be focused when disabled. May 20, 2020

gspencergoog force-pushed the focus_disabled branch from 3767961 to 2b2658a Compare May 21, 2020 19:34

goderbauer reviewed May 26, 2020

View reviewed changes

gspencergoog added 5 commits May 26, 2020 17:37

Add NavigationModality widget, and update other widgets

9075ef3

Update other widgets to remain focused when disabled if navigation mode is NavigationMode.directional.

Review changes

20f7fd6

Remove NavigationModality widget in favor of putting navigation mode …

01e30ab

…on MediaQuery

Handle missing MediaQuery, use switch statements for enum

5152468

Review changes

4a82c57

gspencergoog force-pushed the focus_disabled branch from 2b2658a to 4a82c57 Compare May 27, 2020 00:46

gspencergoog merged commit 8ef2915 into flutter:master May 27, 2020

gspencergoog deleted the focus_disabled branch May 27, 2020 22:30

Hixie reviewed May 28, 2020

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Conversation

gspencergoog commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking Change

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer commented Apr 29, 2020

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gspencergoog commented Apr 29, 2020

Uh oh!

gspencergoog commented May 20, 2020

Uh oh!

gspencergoog commented May 20, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented May 26, 2020

Uh oh!

goderbauer commented May 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gspencergoog commented Apr 16, 2020 •

edited

Loading