-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Reland "Dismiss text selection toolbar with ESC" #98995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reland "Dismiss text selection toolbar with ESC" #98995
Conversation
|
@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. |
|
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;
}
*/ |
3b5320a to
1e1693d
Compare
|
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 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. |
justinmc
left a comment
There was a problem hiding this 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: indent this.
|
Filed #99403 for the autocomplete fix |
Relands #98511
Currently still broken but looking for some hints on how to fix this.
Demo
Fixes #98163
Pre-launch Checklist
///).