Skip to content

Replace FocusTrap with TapRegionSurface - #107262

Merged
auto-submit[bot] merged 17 commits into
flutter:masterfrom
gspencergoog:focus_trap_fix
Jul 29, 2022
Merged

Replace FocusTrap with TapRegionSurface#107262
auto-submit[bot] merged 17 commits into
flutter:masterfrom
gspencergoog:focus_trap_fix

Conversation

@gspencergoog

@gspencergoog gspencergoog commented Jul 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

A PR to address #86972

The code derives largely from the suggestion in this comment, as well as some explorations I had while working on the MenuBar widget. It differs from Venkat's code in that there is also a grouping mechanism for the regions, so that multiple regions can act as one, allowing things like taps on the text field toolbars to not unfocus the text field.

The way TapRegionSurface is supposed to work is that it wraps the whole app area, and triggers any onTapOutside callbacks for regions or groups of regions when a tap occurs.

This is a shift from how FocusTrap works, in that widgets that want to have the focus disappear will need to do some setup instead of getting it by default. Since TextField is the main widget that wants to do this, that shouldn't really be too hard. I added an onTapOutside and tapRegionGroupId attributes to TextField that if unset, will do what FocusTrap used to do (only lose focus on desktop and web platforms). But you can also set it if you want to override that behavior, or disable it, and place the TextField inside of a TapRegion group that includes, for instance, value spinners or a "copy to clipboard" button so that they won't affect the focus.

Usage looks something like this:

Widget build(BuildContext context) {
  return Column(
    children: <Widget>[
      const Text('Outside Surface'),  // Taps on this would not be considered.
      TapRegionSurface( // This would normally be at the top of the app.
        child: Row(
          children: <Widget>[
            const Text('Outside'),
            TapRegion(
              onTapOutside: () {
                clickedOutside.add('No Group');
              },
              child: const Text('No Group'),
            ),
            TapRegion(
              groupId: 1,
              onTapOutside: () {
                clickedOutside.add('Group 1 A');
              },
              child: const Text('Group 1 A'),
            ),
            TapRegion(
              groupId: 1,
              onTapOutside: () {
                clickedOutside.add('Group 1 B');
              },
              child: const Text('Group 1 B'),
            ),
          ],
        ),
      ),
    ],
  );
}

cc @justinmc @venkatd @jpnurmi

@flutter-dashboard flutter-dashboard Bot added a: text input Entering text in a text field or keyboard related problems p: material_ui material_ui package in flutter/packages f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. labels Jul 7, 2022
@gspencergoog gspencergoog mentioned this pull request Jul 7, 2022
Closed
@gspencergoog

gspencergoog commented Jul 7, 2022

Copy link
Copy Markdown
Contributor Author

In looking at the test failures, it's clear that we'd have to figure something out for the cut/copy/paste toolbar so that clicking on it doesn't cause the field to be unfocused.

@gspencergoog gspencergoog changed the title Replace FocusTrap with CompositeTapRegion Replace FocusTrap with TapRegionSurface Jul 8, 2022
@flutter-dashboard flutter-dashboard Bot added the f: focus Focus traversal, gaining or losing focus label Jul 8, 2022
@flutter-dashboard flutter-dashboard Bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jul 8, 2022
@gspencergoog

gspencergoog commented Jul 8, 2022

Copy link
Copy Markdown
Contributor Author

OK, after a few false starts, this is pretty much exactly what @venkatd had, with the addition of groups.

I've also integrated groups into the selection dialogs so that taps on the overlay dialogs don't unfocus the text field.

@gspencergoog
gspencergoog force-pushed the focus_trap_fix branch 4 times, most recently from 53224d4 to 632edbe Compare July 13, 2022 18:52
@flutter-dashboard flutter-dashboard Bot added p: cupertino_ui cupertino_ui package in flutter/packages f: scrolling Viewports, list views, slivers, etc. labels Jul 13, 2022
@gspencergoog
gspencergoog marked this pull request as ready for review July 13, 2022 18:52
@gspencergoog

gspencergoog commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

There's still some documentation that needs to be written for this, but does this seem like a reasonable replacement for FocusTrap?

@gspencergoog

gspencergoog commented Jul 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

And probably some name refinement. I went with TapRegion instead of TapOutsideDetector, since I was hoping to also allow non-gesture-arena "onTapInside" callbacks. Those aren't terribly useful, but it might be interesting to know that there was a tap on something inside your "group", and not just inside your own rect.

@gspencergoog

gspencergoog commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

And it needs more tests too.

Switching back to Draft PR for now.

@gspencergoog
gspencergoog marked this pull request as draft July 13, 2022 21:42
@flutter-dashboard

flutter-dashboard Bot commented Jul 13, 2022

Copy link
Copy Markdown

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.

@justinmc justinmc mentioned this pull request Jul 19, 2022
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 30, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 30, 2022
@engine-flutter-autoroll
f5e4d2b Replace FocusTrap with TapRegionSurface (flutter/flutter#107262)
3b13d13
@gspencergoog

gspencergoog commented Aug 1, 2022

Copy link
Copy Markdown
Contributor Author

Hi @Eerey

Will this also work for a desktop environment (windows) when using a virtual keyboard in combination with a WebView inside flutter?

To be clear: you're talking about the case of building your own virtual keyboard, right? Not a system built-in keyboard?

You should be able to do that, but you might need to wrap the web view in a TapRegion widget, and do the same to your virtual keyboard (using the same groupId for both) to keep the web view from losing focus when you tap on the keyboard buttons. If you're mixing Flutter text fields and web view text fields in the same UI, you might need to use a TextFieldTapRegion around them instead, to let Flutter think that the web view and your keyboard are both part of the text field group.

With this change, building a virtual keyboard should be a lot easier, however.

@gspencergoog gspencergoog mentioned this pull request Aug 1, 2022
Closed
@Eerey

Eerey commented Aug 1, 2022 •
edited
Loading

Copy link
Copy Markdown

Good evening @gspencergoog

To be clear: you're talking about the case of building your own virtual keyboard, right? Not a system built-in keyboard?

Yes, correct.

With this change, building a virtual keyboard should be a lot easier, however.

That sounds amazing. I am very much looking forward to it.

@justinmc justinmc mentioned this pull request Aug 1, 2022
Closed
gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Aug 2, 2022
@gspencergoog
Revert "Replace FocusTrap with TapRegionSurface (flutter#107262)"
759e099 
This reverts commit f5e4d2b because it causes Google test failures.
This was referenced Aug 2, 2022
Closed
Merged
@justinmc justinmc mentioned this pull request Aug 8, 2022
Open
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@gspencergoog @camsim99
Replace FocusTrap with TapRegionSurface (flutter#107262)
3c0e4d8
@Tkko Tkko mentioned this pull request Aug 11, 2022
Closed
@gspencergoog gspencergoog mentioned this pull request Aug 16, 2022
Closed
This was referenced Aug 23, 2022
Closed
Open
@bleroux bleroux mentioned this pull request Aug 29, 2022
Closed
t123yh pushed a commit to SmartCross/flutter that referenced this pull request Sep 17, 2022
@gspencergoog @t123yh
Replace FocusTrap with TapRegionSurface (flutter#107262)
fe31410
@Eerey

Eerey commented Oct 20, 2022 •
edited
Loading

Copy link
Copy Markdown

Hi, where can we observe if this feature is currently present? When will this be available in the master channel? :) currently running flutter 3.3.4

@gspencergoog

gspencergoog commented Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

@Eerey It is currently present in the master and beta channels. It will be in stable with the next stable release.

Just for future reference, the master channel is the first place where things are added, so something new is always available in the master channel. There is only a delay for things going into the more stable channels (beta and stable). See https://github.com/flutter/flutter/wiki/Flutter-build-release-channels for more information.

@Jjagg

Jjagg commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

@gspencergoog Should this not have gone through the breaking change policy by including a design doc and migration guide for FocusTrap/FocusTrapArea?
I had to dig a little to figure out how to migrate.

@gspencergoog

gspencergoog commented Nov 2, 2022

Copy link
Copy Markdown
Contributor Author

@Jjagg I'm sorry it was hard to find. Hopefully the migration wasn't too hard, and that it meets your needs better than FocusTrap did. And no, this change didn't break any of our contributed tests, so it wasn't considered a breaking change for the purposes of the policy.

@Jjagg

Jjagg commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

@gspencergoog It wasn't too bad and this is a way more flexible solution, so I'm very happy with this change :)
Thank you for your work on it.

Ah sorry, I thought public API changes fell under the policy. I'll have to remember to submit some stuff to the contributed tests sometimes.

@AurelVU AurelVU mentioned this pull request Jan 25, 2023
Closed
@Eerey Eerey mentioned this pull request Feb 15, 2023
Open
@CillianMyles CillianMyles mentioned this pull request Mar 11, 2023
Closed
@justinmc justinmc mentioned this pull request Mar 15, 2023
Open
@bleroux bleroux mentioned this pull request May 9, 2023
Closed
@bleroux bleroux mentioned this pull request Mar 4, 2024
Closed
@friebetill friebetill mentioned this pull request May 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes
+1 more reviewer
@jonahwilliams jonahwilliams jonahwilliams left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems c: contributor-productivity Team-specific productivity, code health, technical debt. f: focus Focus traversal, gaining or losing focus f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gspencergoog @jonahwilliams @Eerey @Jjagg @justinmc