-
Notifications
You must be signed in to change notification settings - Fork 6k
Support accessibility back gesture to pop route from view controller #51241
Conversation
09ab397
to
4262a45
Compare
@@ -50,7 +50,6 @@ | |||
0AC232F724BA71D300A85907 /* FlutterEngineTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEngineTest.mm; sourceTree = "<group>"; }; | |||
0AC2330324BA71D300A85907 /* accessibility_bridge_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = accessibility_bridge_test.mm; sourceTree = "<group>"; }; | |||
0AC2330B24BA71D300A85907 /* FlutterTextInputPluginTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterTextInputPluginTest.mm; sourceTree = "<group>"; }; | |||
0AC2330F24BA71D300A85907 /* FlutterBinaryMessengerRelayTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterBinaryMessengerRelayTest.mm; sourceTree = "<group>"; }; |
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.
FlutterBinaryMessengerRelayTest
is no longer in the iOS directory and is tested in common
#44395
@@ -72,7 +71,6 @@ | |||
3DD7D38C27D2B81000DA365C /* FlutterUndoManagerPluginTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterUndoManagerPluginTest.mm; sourceTree = "<group>"; }; | |||
689EC1E2281B30D3008FEB58 /* FlutterSpellCheckPluginTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterSpellCheckPluginTest.mm; sourceTree = "<group>"; }; | |||
68B6091227F62F990036AC78 /* VsyncWaiterIosTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = VsyncWaiterIosTest.mm; sourceTree = "<group>"; }; | |||
73F12C22288F92FF00AFC3A6 /* FlutterViewControllerTest_mrc.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterViewControllerTest_mrc.mm; sourceTree = "<group>"; }; |
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.
Removed in #48162
@@ -1,3 +1,3 @@ | |||
FLUTTER_ENGINE[arch=x86_64]=ios_debug_sim_unopt | |||
FLUTTER_ENGINE[arch=arm64]=ios_debug_sim_unopt_arm64 | |||
FLUTTER_ENGINE=ios_debug_sim_unopt | |||
FLUTTER_ENGINE=ios_debug_sim_unopt_arm64 |
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.
There are more folks running these tests on ARM Macs now than x64.
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.
does the previous 2 line do anything? is it supposed to pick the right engine based on which mac you are running on?
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.
The previous two lines were my attempt at picking the right engine for you #39213, but it doesn't work because at one of the point it's used the ARCH
is unknown
😞
@Hangyujin are you the best person to review this since @chunhtai is out? Or should I add someone else? |
@@ -2104,6 +2104,15 @@ - (int32_t)accessibilityFlags { | |||
return flags; | |||
} | |||
|
|||
- (BOOL)accessibilityPerformEscape { | |||
FlutterMethodChannel* navigationChannel = [_engine.get() navigationChannel]; | |||
if (navigationChannel != nil) { |
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: just if (navigationChannel)
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.
I don't think we're consistent in the engine, I don't have a preference though.
@@ -1,3 +1,3 @@ | |||
FLUTTER_ENGINE[arch=x86_64]=ios_debug_sim_unopt | |||
FLUTTER_ENGINE[arch=arm64]=ios_debug_sim_unopt_arm64 | |||
FLUTTER_ENGINE=ios_debug_sim_unopt | |||
FLUTTER_ENGINE=ios_debug_sim_unopt_arm64 |
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.
does the previous 2 line do anything? is it supposed to pick the right engine based on which mac you are running on?
Did you also try dismissing a dialog (example: https://api.flutter.dev/flutter/material/showDialog.html)? Seems to me it should probably work too |
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
Confirmed that worked on main and also on this branch. |
Routes and modal combined, showing the modal is dismissed before the route is popped import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';
void main() {
runApp(const MaterialApp(
title: 'Navigation Basics',
home: FirstRoute(),
));
}
class FirstRoute extends StatelessWidget {
const FirstRoute({super.key});
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
title: const Text('First Route'),
),
body: Center(
child: ElevatedButton(
child: const Text('Open route'),
onPressed: () {
Navigator.push(
context,
MaterialPageRoute(builder: (context) => const SecondRoute()),
);
},
),
),
);
}
}
class SecondRoute extends StatelessWidget {
const SecondRoute({super.key});
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
title: const Text('Second Route'),
),
body: Center(
child: ElevatedButton(
onPressed: () {
Navigator.of(context).restorablePush(_modalBuilder);
},
child: const Text('Open Modal'),
),
),
);
}
@pragma('vm:entry-point')
static Route<void> _modalBuilder(BuildContext context, Object? arguments) {
return CupertinoModalPopupRoute<void>(
builder: (BuildContext context) {
return CupertinoActionSheet(
title: const Text('Title'),
message: const Text('Message'),
actions: <CupertinoActionSheetAction>[
CupertinoActionSheetAction(
child: const Text('Action One'),
onPressed: () {
Navigator.pop(context);
},
),
CupertinoActionSheetAction(
child: const Text('Action Two'),
onPressed: () {
Navigator.pop(context);
},
),
],
);
},
);
}
} (gesture done at :09 and :13) |
…145345) flutter/engine@90c4d64...016206d 2024-03-18 [email protected] Support accessibility back gesture to pop route from view controller (flutter/engine#51241) 2024-03-18 [email protected] [web] remove Tappable from basic set of a11y roles; add it case by case (flutter/engine#51466) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Adapted suggestion from flutter/flutter#74246 (comment) to pop the route if the view controller route if
accessibilityPerformEscape
is called at that level in the responder chain.Ideally this could know if the route was successfully popped, and only then return
YES
(halting propagation through the responder chain).I confirmed by editing https://docs.flutter.dev/cookbook/navigation/navigation-basics#interactive-example as a demo that that the two-finger scrub (move two fingers back and forth three times quickly, making a "z") navigates back to the last route, and that the
SementicObject
scrub, which already worked, continued to work.I'm not super familiar with this code, hopefully there aren't further gotchas I'm not thinking of.
Fixes flutter/flutter#74246
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.