Skip to content

Conversation

@markusaksli-nc
Copy link
Contributor

@markusaksli-nc markusaksli-nc commented Feb 23, 2022

Relands #98511

Currently still broken but looking for some hints on how to fix this.

Demo
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: Center(
          child: Column(
            children: [
              AutocompleteBasicExample(),
              TextField()
            ],
          ),
        ),
      ),
    );
  }
}

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 #98163

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 flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Feb 23, 2022
@markusaksli-nc
Copy link
Contributor Author

@justinmc do you know why this doesn't work? I'm not sure how else to disable the action so it doesn't interfere with other ESC to dismiss behavior.

@goderbauer goderbauer requested a review from justinmc February 23, 2022 22:35
@justinmc
Copy link
Contributor

Sorry, I think I was mistaken with the advice I gave in #98600 (comment).

Instead of using isEnabled and consumesKey, I think you should check toolbarIsVisible in _hideToolbarAction's onInvoke. If it is, hide it. If not, reinvoke DismissIntent using Actions.invoke.

It seems like isEnabled and consumesKey don't actually allow an Intent to pass through. See the example below and let me know if this doesn't make sense.

CC @gspencergoog Am I right that a disabled Action will not let its Intent pass through to a higher Actions, and the correct way to do that is to reinvoke Actions.invoke, not to use isEnabled? See the example below.

Example of reinvoking an Action. isEnabled/consumesKey don't work.
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() => runApp(MyApp());

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      debugShowCheckedModeBanner: false,
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatelessWidget {
  final String title;

  MyHomePage({
    Key? key,
    required this.title,
  }) : super(key: key);

  final FocusNode _focusNode = FocusNode();

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Flutter Demo'),
      ),
      body: Center(
        child: Shortcuts(
          shortcuts: const <ShortcutActivator, Intent>{
            SingleActivator(LogicalKeyboardKey.escape): _MyIntent(),
          },
          child: Actions(
            actions: <Type, Action<Intent>>{
              _MyIntent: CallbackAction<_MyIntent>(onInvoke: (_) {
                print('Outer Actions called for _MyIntent');
              }),
            },
            child: Builder(
              builder: (BuildContext context) {
                return Actions(
                  actions: <Type, Action<Intent>>{
                    _MyIntent: CallbackAction<_MyIntent>(onInvoke: (_MyIntent intent) {
                      print('Inner Actions called for _MyIntent');
                      return Actions.invoke(context, intent);
                    }),
                    // The below will not work. I believe it passes the key
                    // through, but not the _MyIntent.
                    /*
                    _MyIntent: _TransparentCallbackAction<_MyIntent>(onInvoke: (_) {
                      print('justin _TransparentCallbackAction invoked?');
                    }),
                    */
                  },
                  child: Column(
                    mainAxisAlignment: MainAxisAlignment.center,
                    children: [            
                      const TextField(
                        decoration: InputDecoration(
                          hintText: 'Tap to focus the field, then press escape.',
                        ),
                      ),
                      TextButton(
                        focusNode: _focusNode,
                        onPressed: () {
                          _focusNode.requestFocus();
                        },
                        child: const Text('Or tap to focus the button before pressing escape.'),
                      ),
                    ],
                  ),
                );
              },
            ),
          ),
        ),
      ),
    );
  }
}

class _MyIntent extends Intent {
  const _MyIntent();
}

// An attempt at creating an Action that passes its Intent through, but it
// doesn't work.  I believe isEnabled and consumesKey are operating on the key
// itself, which is handled by Shortcuts, and no the Intent.
/*
class _TransparentCallbackAction<T extends Intent> extends CallbackAction<T> {
  _TransparentCallbackAction({
    required OnInvokeCallback<T> onInvoke,
  }) : super(onInvoke: onInvoke);

  @override
  bool isEnabled(covariant T intent) => false;

  @override
  bool consumesKey(covariant T intent) => false;
}
*/

@markusaksli-nc markusaksli-nc force-pushed the dimiss-text-selection-toolbar branch from 3b5320a to 1e1693d Compare March 1, 2022 13:14
@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Mar 1, 2022

Thanks for the advice @justinmc. I was coming at this from the wrong angle thinking about the key event being consumed but rather the issue was that the DismissIntent fired from the app was being consumed at the first depth and without being re-invoked it would obviously not do anything further down the tree.

This got rid of the issues I was having, let me know if there is anything that should be changed about the added method. I was wondering if this could also cause issues down the line in the autocomplete dismiss PR since it doesn't re-invoke? Maybe it would be a good idea to always pass the DismissIntent further down if it is ignored.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I think you're right, your autocomplete dismiss PR should reinvoke the intent like this. Good catch. Do you want to open a quick PR for that? I can do it if you don't have time.

);

final EditableTextState state =
tester.state<EditableTextState>(find.byType(EditableText));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: indent this.

@markusaksli-nc
Copy link
Contributor Author

Filed #99403 for the autocomplete fix

@fluttergithubbot fluttergithubbot merged commit 03ed3c9 into flutter:master Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 2, 2022
mdebbar added a commit that referenced this pull request Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 12, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
@TahaTesser TahaTesser deleted the dimiss-text-selection-toolbar branch April 26, 2022 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The escape key should close the right click menu on desktop

3 participants