[flutter] when primary mouse pointers don't contact a focused node, reset the focus by jonahwilliams · Pull Request #82575 · flutter/flutter

jonahwilliams · 2021-05-14T21:30:23Z

Introduces a widget that checks if a pointer down event has hit the currently focused nodes' render object. This leads to odd behavior with text field decorations due to the focus being only owned by the inner editable text widget.

The modal route's focus scoped node is used to "reset" the focus when pointer did not contact either.

This is limited to mouse pointers on desktop and mobile web.

jonahwilliams · 2021-05-14T21:31:25Z

This currently only applies for keyboard events, I'd need to do more research into how touch events should behave

skia-gold · 2021-05-14T21:36:18Z

Gold has detected about 2 untriaged digest(s) on patchset 2.
View them at https://flutter-gold.skia.org/cl/github/82575

jonahwilliams · 2021-05-14T21:47:46Z

adding another widget here seems to be changing the layout slightly, which is unfortunate...

flutter-dashboard · 2021-05-14T21:49:38Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai · 2021-05-14T22:36:09Z

                            child: Builder(
                              builder: (BuildContext context) {
-                                return widget.route.buildPage(
+                                return _FocusTrap(child: widget.route.buildPage(


any reason we scope at route level?

i scoped it at the route level so that I can have this and the shortcuts widget used by the SelectionArea widget use the same focus node.

chunhtai · 2021-05-14T22:37:08Z

+  FocusNode get focusNode => widget.focusNode ?? _focusNode;
+
+  void _stealFocus(TapDownDetails details) {
+    if (details.kind != PointerDeviceKind.mouse)


do you know what is the native behavior for mobile if you have an external mouse?

i don't have access to the hardware to test this. we could definitely just disable the behavior on iOS/android until we could test it

(disabled for non-desktop platforms for now)

MarcusWichelmann · 2021-05-19T09:27:10Z

Just an outsiders thought:

This looks great and all, but should it really be necessary to add a FocusTrap widget to the widget tree just to get a behaviour which one might expect to be there by default? Of course, this is a good first step, but maybe this functionallity should also be included into other widgets like e.g. MaterialApp so programmers get the behaviour they know from the web out of the box without having to google for it and adding the FocusTrap widget theirself?

jonahwilliams · 2021-05-19T16:50:46Z

I agree, I'm not sure that this is the right solution exactly but i'm not sure how else you would do it. Widgets can't unfocus themselves if they aren't hit 🤔

Wissperwind · 2021-05-19T21:46:26Z

In HTML Web every element can be focused. Even if it is not editable, correct?
Can't we make every widget fucusable? This would change the focus away from a text field if you click for example on the background scaffold or the scroll bar.

I am not a flutter developer. So this is only an idea from outside but I wanted to post it.

For me it is rather important to get some working solution in the next two weeks than to get a really good solution.

jonahwilliams · 2021-05-19T22:25:09Z

An html element isn't equivalent to a flutter widget. A widget not have any UI, only configuring other widgets themselves. Even making every widget with uI focusable would make pure focus traversal painful.

jonahwilliams · 2021-05-20T01:00:05Z

Based on some discussion with @gspencergoog , I've updated this to use the unfocus functionality of the modal route's FocusScopeNode. Of course, using the gesture recognizer the way that I am is breaking the world, so I'll need to find a way to make this change not compete with any other recognizers somehow...

skia-gold · 2021-05-20T01:03:12Z

Gold has detected about 3 untriaged digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/82575

jonahwilliams · 2021-05-20T22:21:50Z

  /// Creates a tap gesture recognizer.
-  BaseTapGestureRecognizer({ Object? debugOwner })
-    : super(deadline: kPressTimeout , debugOwner: debugOwner);
+  BaseTapGestureRecognizer({ Object? debugOwner, Set<PointerDeviceKind>? supportedDevices })


Will revert, unrelated

jonahwilliams · 2021-05-20T22:22:07Z

-                _requestKeyboard();
-              },
-              onDidGainAccessibilityFocus: handleDidGainAccessibilityFocus,
+    return FocusArea(


Because we want clicks on the decorations of at text field to retain focus

jonahwilliams · 2021-05-20T22:22:30Z

PTAL at the current approach

chunhtai

LGTM except for the share focusScopeNode in two widgets.

chunhtai · 2021-06-04T17:03:13Z

-                                    ignoring: ignoreEvents,
-                                    child: child,
+                      child: _FocusTrap(
+                        focusScopeNode: focusScopeNode,


The same focusnode get passed into two widgets. not sure if this is a concern cc @gspencergoog

If it is, we can avoid this by merging the FocusScope and _focusTrap into one widget.

Usually sharing a FocusNode (or FocusScopeNode) between two widgets is a concern because they'll fight over the attachment. If the focus node is only used to call requestFocus(), or something else that doesn't require attachment, in one of the widgets, then it's technically OK.

Basically, the rule is: Don't pass the same focus node to two widgets that will attach it to the focus tree. Only one widget can "own" it.

You might also have lifetime issues when it comes to disposing them. Again, only one may dispose of the focus node, and so the widget that it is passed to that doesn't own it needs to have a shorter lifetime than the one that does own it.

I think what I'm doing here is safe, no stack traces or bugs so far. What would you like me to change about the current structure?

I don't have a good suggestion neither, if @gspencergoog is ok with the current structure, I am fine with it as well

I think I'm OK with the current structure: the widget using the node is a private class, so we have control over its lifetime, and it doesn't appear to be doing anything that would attach the node to the focus tree.

gspencergoog

gspencergoog · 2021-06-08T17:28:56Z

+      || event.buttons != kPrimaryButton
+      || event.kind != PointerDeviceKind.mouse
+      || _shouldIgnoreEvents
+      || currentFocus == null)


nit: Can you put braces on this conditional block? Having a really long multi-line conditional just reads weird without the braces.

gspencergoog · 2021-06-08T17:30:56Z

-                                    ignoring: ignoreEvents,
-                                    child: child,
+                      child: _FocusTrap(
+                        focusScopeNode: focusScopeNode,


I think I'm OK with the current structure: the widget using the node is a private class, so we have control over its lifetime, and it doesn't appear to be doing anything that would attach the node to the focus tree.

jonahwilliams · 2021-06-09T00:05:29Z

FYI @renyou , not sure what is up with the google testing on this PR

renyou · 2021-06-09T00:20:30Z

It seems some google internal tests may be very slow and I am going to increase the timeout threshold. Hopefully it can resolve this.

renyou · 2021-06-09T00:25:43Z

@jonahwilliams Did you trigger a full test on this PR?

jonahwilliams · 2021-06-09T00:30:41Z

yes, since it is a potentially a substantial change I wanted to be sure we ran as many tests as possible...

renyou · 2021-06-09T00:33:24Z

Yes! The tests are actually all done, just took longer to analyze all the test results. I manually set the results. Everything looks good!

jonahwilliams · 2021-06-09T00:35:36Z

🎊

HansMuller · 2021-06-10T17:19:04Z

@jonahwilliams - does this PR address #74354?

jonahwilliams · 2021-06-10T17:33:35Z

This only handles clicks via mouse on desktop and mobile web, not taps - though the PR also mentions clicks. I'm not sure if that is trying to fix the same issue as this or a different input connection based focus issue

jpnurmi · 2021-07-01T22:17:34Z

On Linux, native GNOME/GTK apps don't reset focus when clicking outside text fields.
The focused render object inside TextField doesn't cover the entire TextField? Please check TextField temporarily loses focus on mouse press outside text #85732
Is it not possible to have buttons that don't reset focus? [text_input] introduce TextInputControl #76072 (comment)

Hixie · 2021-07-05T21:50:50Z

If we want this behaviour, why not just put a Focus widget around the route?

...but I'm not convinced we want this behaviour. It's not what happens on Windows and macOS as far as I can tell.

Hixie · 2021-07-05T21:52:16Z

I think this is just something we want on Web, and on Web the behaviour is just that the root of the tree is itself focusable, no need to check whether a tap would have focused a node and all that.

jonahwilliams added 2 commits May 13, 2021 21:00

[flutter] allow de-focusing of text fields

7fef2cf

make widget private and add unit test

0b2e07b

flutter-dashboard Bot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels May 14, 2021

google-cla Bot added the cla: yes label May 14, 2021

jonahwilliams marked this pull request as draft May 14, 2021 21:46

chunhtai reviewed May 14, 2021

View reviewed changes

jonahwilliams added 2 commits May 19, 2021 17:19

Merge branch 'master' of github.com:flutter/flutter into de_focus

a4e21d2

maybe this work better

52d40b3

jonahwilliams force-pushed the de_focus branch from c43c24c to 52d40b3 Compare May 20, 2021 00:57

jonahwilliams added 2 commits May 20, 2021 11:35

getting there...

842e88b

it works

84f21d6

jonahwilliams marked this pull request as ready for review May 20, 2021 22:21

jonahwilliams commented May 20, 2021

View reviewed changes

jonahwilliams requested review from chunhtai and gspencergoog May 20, 2021 22:22

jonahwilliams changed the title ~~[flutter] introduce a focus trap at the page level~~ [flutter] when primary mouse pointers don't contact a focused node, reset the focus May 20, 2021

fix analysis errors

f0bfaf4

chunhtai approved these changes Jun 4, 2021

View reviewed changes

gspencergoog approved these changes Jun 8, 2021

View reviewed changes

Update routes.dart

6f41377

jonahwilliams added the waiting for tree to go green label Jun 9, 2021

fluttergithubbot merged commit 85be4f6 into flutter:master Jun 9, 2021

jonahwilliams mentioned this pull request Jun 9, 2021

Selection toolbar/handler should disappear when it is opened and user clicks outside of it #72029

Open

jonahwilliams deleted the de_focus branch June 10, 2021 03:58

This was referenced Jun 10, 2021

TextFormField does not get blurred when tapped outside of the widget. #74354

Closed

[Web][Desktop]: The dropdown of an AutoComplete widget should be dismissed by an ESC hit or by clicking outside area. #84416

Closed

This was referenced Jul 1, 2021

[text_input] introduce TextInputControl #76072

Merged

TextField temporarily loses focus on mouse press outside text #85732

Closed

jpnurmi mentioned this pull request Jul 9, 2021

[desktop/web] TextField loses focus when pressing prefix or suffix button #86154

Closed

Hixie mentioned this pull request Jul 23, 2021

Consider simplifying FocusTrap? #86972

Closed

justinmc mentioned this pull request Sep 17, 2021

(WIP) Proposal to simplify FocusTrap #90240

Closed

elliette mentioned this pull request Jan 27, 2022

Focus moves to ModalScopeState instead of an app’s FocusScope when you click outside of a TextField #97389

Closed

justinmc mentioned this pull request Mar 3, 2022

(WIP) Proposal to simplify FocusTrap, revived #99434

Closed

bleroux mentioned this pull request Apr 2, 2022

When changing focus between inputs, focus goes through ModalScopeState first #97013

Closed

gspencergoog mentioned this pull request Oct 10, 2023

MenuAnchor-based menus pass through clicks that close the menu #135327

Closed

Conversation

jonahwilliams commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented May 14, 2021

Uh oh!

skia-gold commented May 14, 2021

Uh oh!

jonahwilliams commented May 14, 2021

Uh oh!

flutter-dashboard Bot commented May 14, 2021

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!

MarcusWichelmann commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented May 19, 2021

Uh oh!

Wissperwind commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented May 19, 2021

Uh oh!

jonahwilliams commented May 20, 2021

Uh oh!

skia-gold commented May 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented May 20, 2021

Uh oh!

chunhtai 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!

gspencergoog 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!

jonahwilliams commented Jun 9, 2021

Uh oh!

renyou commented Jun 9, 2021

Uh oh!

renyou commented Jun 9, 2021

Uh oh!

jonahwilliams commented Jun 9, 2021

Uh oh!

renyou commented Jun 9, 2021

Uh oh!

jonahwilliams commented Jun 9, 2021

Uh oh!

HansMuller commented Jun 10, 2021

Uh oh!

jonahwilliams commented Jun 10, 2021

Uh oh!

jonahwilliams commented May 14, 2021 •

edited

Loading

MarcusWichelmann commented May 19, 2021 •

edited

Loading

Wissperwind commented May 19, 2021 •

edited

Loading