[flutter] prevent errant text field clicks from losing focus by jonahwilliams · Pull Request #86041 · flutter/flutter

jonahwilliams · 2021-07-07T22:03:51Z

Without making dramatic adjustments to the focus system, attempts to support the text field / editable text nesting while still preventing clicks from text field decoration from removing focus from the editable text.

Fixes #86154

jonahwilliams · 2021-07-07T22:47:56Z

@gspencergoog @chunhtai this is an attempt to fix one of the side effects from the focus removal change; since the "focused" part of the text field is just the tiny editable text portion it is too easy to lose focus by clicking on the decorative parts. This introduces another context that can be attached to a focus node to detect this.

jonahwilliams · 2021-07-07T22:48:18Z

-    focusScopeNode.addListener(_currentFocusListener);
-  }
-
-  void _currentFocusListener() {


For some reason this wasn't giving up to date results when using tab navigation

I am not so certain about why we need to remove this code. If this is indeed a bug fixes, we should have a test for this.

flutter-dashboard · 2021-07-07T22:48:33Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

jonahwilliams · 2021-07-07T23:18:34Z

ahh, need to port over my old tests

chunhtai · 2021-07-07T22:53:05Z

  BuildContext? _context;

+  /// An optional parent context that determines whether or not a primary pointer
+  /// gesture that missed the actual [context] should remove focus from the node.


Can you also document on when will this focus manager will remove focus from the node? will it do it when the parent context = null or will it do it when the parent context != null?

Also where the focus will go if this happen

expanded the comments here a bit

chunhtai · 2021-07-07T23:31:12Z

    _clipboardStatus?.addListener(_onChangedClipboardStatus);
    widget.controller.addListener(_didChangeTextEditingValue);
-    _focusAttachment = widget.focusNode.attach(context);
+    _focusAttachment = widget.focusNode.attach(context, contextBounds: widget.parentContext);


It looks like the hittest prioritizes context bounds to determine hittest rect, I wonder can it directly attach to the widget.parentContext if it is not null?

Thinking about this more, we might have some issues if editable text migrates to use a regular focus node since we won't have the attachment available.

Attaching to the parent context might work, but thinking about it more I'm not sure if we need to worry about reparenting. Surely you'd never re-parent the text field decoration apart from an editable text right?

third party libraries does crazy thing around editable text, I wouldn't be surprised if the editabletext gets reparented. but I think we should be able to reparent the attachment in didUpdateWidget?

Currently focus will be lost (at least according to my test cases)

yes, focusAttachment.detach will call unfocus on the focusnode. I think this is probably fine because this will be an edgy case where people keep swapping out the parent focus. We can fix it later if that actually cause issue.

…_node

chunhtai · 2021-07-08T18:51:38Z

-    focusScopeNode.addListener(_currentFocusListener);
-  }
-
-  void _currentFocusListener() {


I am not so certain about why we need to remove this code. If this is indeed a bug fixes, we should have a test for this.

chunhtai · 2021-07-08T18:52:24Z

  /// than 1.
  final ScrollBehavior? scrollBehavior;

+  /// Optionally, the parent build context if this editable text was built as part of


Suggested change

/// Optionally, the parent build context if this editable text was built as part of

/// The optional parent build context if this editable text was built as part of

chunhtai · 2021-07-08T18:53:28Z

+  /// a larger text field.
+  ///
+  /// If this is provided then focus will be attached to the parent context instead of
+  /// the editable text. If the [parentContext] is later modified while the editable text


Maybe also explain what's the different consequences to attach to this context vs parent context, and why developer may want to do that

chunhtai · 2021-07-08T18:54:46Z

The overall approach looks good to me, just some nits

jonahwilliams · 2021-07-08T18:56:39Z

Good point, i need to investigate a bit more. i might be working around a focus bug, so finding the root cause first is probably ideal.

jonahwilliams · 2021-07-09T01:22:14Z

moved the listener change to #86141

jonahwilliams · 2021-07-11T21:09:08Z

I wasn't able to make the parent context work - it changed how actions/shortcuts were looked up in some non-obvious ways. I've gone back to tagging render objects with the focus node.

jonahwilliams · 2021-07-12T20:48:49Z

This is ready for review

gspencergoog · 2021-07-12T20:55:48Z

+/// This area is conceptually part of the editable text, but not attached to the
+/// focus context. The [FocusTrapArea] is used to inform the framework of this
+/// relationship, so that primary pointer contact inside of this region but above
+/// the editable text focus will not trigger loss of focus.


You might add a reference/description here for the focus trap and when it comes into play, etc. If I pretend that I just happened upon the API docs page for this widget, it feels like if I wasn't writing my own text widget I wouldn't quite know when to use it, or what it affects.

Since _FocusTrap isn't a public class, there's probably not much mention of it in other docs.

I could make it public, no need to keep it private really

that would require me to document it a bit more, but no harm there

The other option would be to make FocusTrapArea private, if you don't think anyone will need it outside of the text field (but it seems like they might).

probably the cupertino text field needs it as well, as would other like the macos ui package - depending on whether or not they are editable text widget based

jonahwilliams · 2021-07-13T18:47:31Z

I've updated the documentation and made focus trap/ focus area stuff public. ptAL @gspencergoog

gspencergoog

chunhtai

LGTM

[flutter] prevent errant text field clicks from losing focus

76377b6

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

google-cla Bot added the cla: yes label Jul 7, 2021

jonahwilliams commented Jul 7, 2021

View reviewed changes

jonahwilliams requested review from chunhtai and gspencergoog July 7, 2021 22:48

jonahwilliams marked this pull request as ready for review July 7, 2021 22:48

update test case

2ba04af

chunhtai reviewed Jul 7, 2021

View reviewed changes

jonahwilliams added 2 commits July 8, 2021 11:24

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

6649413

…_node

use parentContext for focus attachment

ed7a6a1

jonahwilliams requested a review from chunhtai July 8, 2021 18:44

chunhtai reviewed Jul 8, 2021

View reviewed changes

jonahwilliams mentioned this pull request Jul 9, 2021

[flutter] tab navigation does not notify the focus scope node #86141

Merged

jonahwilliams added 2 commits July 11, 2021 12:40

merge with upstream

f3a9fd2

Remove parent context

feca1c4

Update editable_text.dart

01315ce

gspencergoog reviewed Jul 12, 2021

View reviewed changes

make public and add more docs

61001ea

gspencergoog approved these changes Jul 13, 2021

View reviewed changes

Jonah Williams added 3 commits July 13, 2021 12:58

Update routes.dart

b35ead1

Update editable_text.dart

ea073ea

Update editable_text.dart

b9feaec

chunhtai approved these changes Jul 13, 2021

View reviewed changes

update test expectations

e6d934a

jonahwilliams added the waiting for tree to go green label Jul 13, 2021

fluttergithubbot merged commit 2900347 into flutter:master Jul 13, 2021

jpnurmi mentioned this pull request Jul 14, 2021

TextField temporarily loses focus on mouse press outside text #85732

Closed

jonahwilliams deleted the merged_focus_node branch July 14, 2021 04:45

justinmc mentioned this pull request Sep 17, 2021

(WIP) Proposal to simplify FocusTrap #90240

Closed

justinmc mentioned this pull request Mar 3, 2022

(WIP) Proposal to simplify FocusTrap, revived #99434

Closed

	/// Optionally, the parent build context if this editable text was built as part of
	/// The optional parent build context if this editable text was built as part of

Conversation

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

Uh oh!

Uh oh!

jonahwilliams commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard Bot commented Jul 7, 2021

Uh oh!

jonahwilliams commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jul 8, 2021

Uh oh!

jonahwilliams commented Jul 8, 2021

Uh oh!

jonahwilliams commented Jul 9, 2021

Uh oh!

jonahwilliams commented Jul 11, 2021

Uh oh!

jonahwilliams commented Jul 12, 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!

jonahwilliams commented Jul 13, 2021

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahwilliams commented Jul 7, 2021 •

edited

Loading

chunhtai Jul 7, 2021 •

edited

Loading