Dismiss Autocomplete with ESC #97790

markusaksli-nc · 2022-02-04T12:19:48Z

Allows the Autocomplete options to be hidden by pressing ESC. As for revealing the options again here's how it should work:

When the text is edited (but not when moving the caret around or selecting text)
When the field is re-focused
When up or down arrows are pressed for navigating autocomplete options

This is mostly based off of the angular implementation of material autocomplete but there are some discrepancies:

Due to _selection == null in

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

Line 294 in fd8d035

return _focusNode.hasFocus && _selection == null && _options.isNotEmpty;

once a selection has been made the autocomplete options will not appear to show the only completed option.
Options will not be shown on empty input by default (well it's up to optionsBuilder).

Another interesting detail is that in the angular material implementation the highlighted selection index gets reset when the options are hidden. This is different from something like the search bar in YouTube, where the highlighted option remains the same when the options are shown again by pressing the up or down arrow. The latter is how it works in the current version of the PR.

Basic autocomplete sample

import 'package:flutter/material.dart';

void main() => runApp(const AutocompleteExampleApp());

class AutocompleteExampleApp extends StatelessWidget {
  const AutocompleteExampleApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(
          title: const Text('Autocomplete Basic'),
        ),
        body: const Center(
          child: AutocompleteBasicExample(),
        ),
      ),
    );
  }
}

class AutocompleteBasicExample extends StatelessWidget {
  const AutocompleteBasicExample({Key? key}) : super(key: key);

  static const List<String> _kOptions = <String>[
    'aardvark',
    'bobcat',
    'chameleon',
  ];

  @override
  Widget build(BuildContext context) {
    return Autocomplete<String>(
      optionsBuilder: (TextEditingValue textEditingValue) {
        if (textEditingValue.text == '') {
          return const Iterable<String>.empty();
        }
        return _kOptions.where((String option) {
          return option.contains(textEditingValue.text.toLowerCase());
        });
      },
      onSelected: (String selection) {
        debugPrint('You just selected $selection');
      },
    );
  }
}

Fixes #84416

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

flutter-dashboard · 2022-02-04T12:19:51Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

markusaksli-nc · 2022-02-04T12:55:55Z

cc @jonahwilliams @gspencergoog

justinmc

Thanks for taking this on, I have multiple times been using an Autocomplete and instinctively tried to dismiss the options with escape, only to be disappointed. We should support this feature.

The biggest point that I brought up in my comments is that we might be able to use Shortcuts instead of onKeyDown. Otherwise my comments are pretty minor and I like the approach.

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

justinmc · 2022-02-04T23:08:52Z

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

Missing a space after if here, and in a few other places in this PR.

So the point of _lastText is (tell me if I'm wrong):

Say you're typing into the field with the menu visible, and then you hit escape to close it. It will only reopen again if you modify the text. If you just change the selection then the menu will stay hidden.

Do you need to set _lastText when _onKeyEvent hides the menu too?

As a nit, maybe add a comment at the definition of _lastText and/or here to explain this stuff a bit.

You understood correctly but I decided to remove the check for _hidden here to remove any doubts about this being checked against an outdated value.

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

justinmc · 2022-02-04T23:20:16Z

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

I trust your judgement on this. Both seem like reasonable choices to me. We can keep an eye out for any issues in the future in case users disagree (as well as for the other inconsistencies with Angular Material that you pointed out).

Maybe if we use Shortcuts, as mentioned below, then users could do whichever behavior they want here.

justinmc · 2022-02-05T00:02:08Z

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

Nit: I feel like this _hidden piece of state can be implemented in a way that's more clear. My problems with it are:

It's called _hidden, but it's not a direct indication of whether the options are currently hidden or not. It's possible that _hidden is false but the options are not shown.

It encompasses both whether the user has recently pressed escape and the current focus, which is already used elsewhere as _focusNode.hasFocus.

What if we call it something like _userHidOptions and don't change it in _onChangedFocus? Just use _focusNode.hasFocus when you need to know that.

Then we know what it means when it's true (the user hid the options recently and that hasn't yet been reset) and we have a better idea of what implications it has (the options may be hidden, but check _shouldShowOptions to be sure).

The fact that this piece of state also encompasses if the field has focus is just a byproduct of the fact that it needs to be reset when the field is un-focused and re-focused. It might be more clear if the bit in _onChangedFocus is changed to something like

// Called when the field's FocusNode changes. void _onChangedFocus() { // Options should no longer be hidden when the field is re-focused. if(_userHidOptions && _focusNode.hasFocus) { _userHidOptions = false; } _updateOverlay(); }

but I just figured this is more concise. If you think it would help readability I'm not against this change.

I see you went with my idea, it's not perfect as you pointed out but hopefully it helps improve readability a bit. I'll try to keep an eye out in case anyone is confused by this and we can reconsider.

markusaksli-nc · 2022-02-05T19:33:25Z

Thanks for the fast review @justinmc. I think the Shortcuts solution also makes a lot more sense because the _focusNode can refer to widget.focusNode so it might be unexpectedly overriding onKeyEvent.

One slight snag I ran into was that shortcuts need to be disabled and enabled according to the old _shouldShowOptions without !_userHidOptions. This left me with this silly choice of copying everything from there to _setActionsEnabled in _updateOverlay or changing its definition to something like _canShowOptions and having _canShowOptions && !_userHidOptions in _updateOverlay.

Either way it looks a bit silly with _shouldShowOptions now being used in just one spot.

Also added a test, I just left in a bunch of hiding with the escape key but it could maybe be made more concise somehow?

markusaksli-nc · 2022-02-15T15:53:02Z

Just noticed that this could be using the DismissIntent instead of a custom one (not sure which is preferable).

gspencergoog · 2022-02-15T17:31:01Z

I'd say DismissIntent is preferable.

justinmc

LGTM 👍

Just nits otherwise (DismissIntent and a few other small things).

justinmc · 2022-02-16T00:04:40Z

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

+1 to @gspencergoog's suggestion to use DismissIntent instead of this new one.

justinmc · 2022-02-16T00:07:39Z

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

Good call saving this value locally, I guess it could change after the await.

justinmc · 2022-02-16T00:08:13Z

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

Nit: "gets changes" => "gets changed" or "changes"? Or maybe I'm reading that wrong...

Nit: The styleguide says never use "we" (though that's mainly for public docs).

justinmc · 2022-02-16T00:18:18Z

packages/flutter/test/widgets/autocomplete_test.dart

This test is super thorough and easy to follow 👍

markusaksli-nc · 2022-02-16T16:54:03Z

Fixed the nit, going to merge and check that this plays well with #98511

markusaksli-nc · 2022-02-16T18:52:10Z

Update: they did not play nice together at all, see #98600

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 4, 2022

markusaksli-nc mentioned this pull request Feb 4, 2022

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

Closed

HansMuller requested a review from justinmc February 4, 2022 22:53

justinmc reviewed Feb 5, 2022

View reviewed changes

markusaksli-nc requested a review from justinmc February 9, 2022 13:31

justinmc approved these changes Feb 16, 2022

View reviewed changes

markusaksli-nc added 6 commits February 16, 2022 18:08

Dismiss autocomplete with ESC

82a529b

shortcut checking fix

24eb170

added commented hilight reset

a808474

switched to shortcuts

dcd69d7

added test

4ea6d23

switch to DismissIntent, fix doc typo

6eeec63

markusaksli-nc force-pushed the dismiss-autocomplete branch from ff79efe to 6eeec63 Compare February 16, 2022 16:37

markusaksli-nc added the waiting for tree to go green label Feb 16, 2022

fluttergithubbot merged commit 919d205 into flutter:master Feb 16, 2022

engine-flutter-autoroll mentioned this pull request Feb 16, 2022

Roll Flutter from 9514106108d1 to 919d20511cf6 (4 revisions) flutter/packages#859

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2022

919d205 Dismiss Autocomplete with ESC (flutter/flutter#97790)

b462f7f

markusaksli-nc mentioned this pull request Feb 16, 2022

Revert "Dismiss text selection toolbar with ESC" #98600

Merged

engine-flutter-autoroll mentioned this pull request Feb 16, 2022

Roll Flutter from b62327903d31 to 919d20511cf6 (2 revisions) flutter/plugins#4871

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 16, 2022

919d205 Dismiss Autocomplete with ESC (flutter/flutter#97790)

74ed9a1

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

Dismiss Autocomplete with ESC (flutter#97790)

9af67fa

TahaTesser deleted the dismiss-autocomplete branch September 23, 2022 20:00

Dismiss Autocomplete with ESC #97790

Dismiss Autocomplete with ESC #97790

Uh oh!

Conversation

markusaksli-nc commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 4, 2022

Uh oh!

markusaksli-nc commented Feb 4, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

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!

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!

markusaksli-nc commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusaksli-nc commented Feb 15, 2022

Uh oh!

gspencergoog commented Feb 15, 2022

Uh oh!

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

markusaksli-nc commented Feb 16, 2022

Uh oh!

markusaksli-nc commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markusaksli-nc commented Feb 4, 2022 •

edited

Loading

markusaksli-nc commented Feb 5, 2022 •

edited

Loading

markusaksli-nc commented Feb 16, 2022 •

edited

Loading