Disable arrow key focus navigation for text fields

[gspencergoog]

Description

This disables the arrow key focus navigation for text fields. This was non-standard behavior for text fields, although it remains useful for other kinds of controls.

Related Issues

Fixes #42259

Tests

• Added tests for Material and Cupertino text fields to make sure that arrow keys don't change focus.

Breaking Change

• No, this is not a breaking change.

[gspencergoog]

Actually, this exposed an issue in the focus subsystem. I have a fix here: #42554. Once that lands, this PR will pass tests.

[mdebbar]

Thanks for fixing this. The change looks good.

My only concern with this approach is that it disables arrow-key navigation in text fields with no way to re-enable it. Ideally, arrow-key navigation in text fields would be off by default. But if a developer decides that they want this behavior in their app, they should be able to do so.

[gspencergoog]

Yes, I share that concern about not being able to re-define them, but I'm not sure how to implement that, other than adding an explicit flag to the text field to not re-map them. We already have too many flags, and this one seems oddly specific.

The other way I could block it would be to bind the DirectionalFocusAction to do nothing, but then the developer wouldn't be able to bind other keys to that action (e.g. maybe they want shift-left-arrow to navigate left).

Being able to re-bind the arrow keys to something is probably not as useful as it seems, however, since the _Editable will eat the left/right arrow keys most of the time before the shortcut gets activated, and when it's a multiline field, it'll eat the up/down arrow keys too. Not that it wouldn't be useful sometimes, but only rarely.

[gspencergoog]

@mdebbar, can I take that as an "LGTM", or do you want someone else to review it?

[mdebbar]

Thanks for the detailed reply.

LGTM!

[gspencergoog]

Another way this could be implemented is to check the current focus inside the action when the action is invoked, and have the action do nothing if it's a text field. Then if someone really wanted to remap the keys, they could just replace the default action with another one that doesn't do that check.

The down side of that approach is that it would only work for text fields that wrapped EditableText, but that's a pretty good bet, since writing your own text widget is very hard. And if you're up for doing that, then you can easily define your own action that does what you want for your text fields.

Would that be a better implementation, do you think?

[mdebbar]

Yeah I think I prefer that approach. It also removes the duplication of code in material and cupertino text fields.

[gspencergoog]

Oh, crap, I didn't see your comment until after I submitted. I'll correct it tomorrow.

[technolion]

I was happy to see that focus traversal was implemented by using the arrow keys and very sad that it was removed again. I am developing a desktop app for macOS and Windows.
I there more info why you consider focus traversal with arrow keys as non-standard behaviour?

[gspencergoog]

It's actually only removed for text fields. Other controls will continue to have arrow key focus traversal. We removed it for text fields because it is non-standard for both mobile and desktop, and so was surprising to users. If you open most desktop apps, focus a text field, and try and move it to another field with the arrow keys, it moves the cursor, and not the input focus. Spreadsheets are an exception, but most other apps (web browsers, preference panels, file dialogs, etc) don't allow arrow key traversal between fields. On macOS, even having arrow key traversal on non-list/non-grid primitives is non standard, although not as surprising.

If you still want focus traversal for text fields in your app, you will be able to add it back in again once I implement the change I alluded to in #42533 (comment) (which I'm going to attempt today). Once that exists, you will be able to bind the arrow keys to a directional focus action that doesn't check for text fields, it just won't be the default.