From 9c76c1e68d6677e144d004610719980c826d66b2 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 26 Nov 2024 16:52:35 -0800 Subject: [PATCH] [SuperEditor] - Fix: When navigating from screen with keyboard open to screen with no IME connection, KeyboardScaffoldSafeArea pushes the content up above the keyboard even though its closed. (Resolves #2419) (#2421) --- .../example/lib/main_super_editor_chat.dart | 37 +++- .../keyboard_panel_scaffold.dart | 44 +++- .../keyboard_panel_scaffold_test.dart | 190 ++++++++++++++++-- 3 files changed, 243 insertions(+), 28 deletions(-) diff --git a/super_editor/example/lib/main_super_editor_chat.dart b/super_editor/example/lib/main_super_editor_chat.dart index 22e26f6804..16d7fd21b7 100644 --- a/super_editor/example/lib/main_super_editor_chat.dart +++ b/super_editor/example/lib/main_super_editor_chat.dart @@ -25,10 +25,39 @@ void main() { runApp( MaterialApp( - home: Scaffold( - resizeToAvoidBottomInset: false, - body: MobileChatDemo(), - ), + routes: { + "/": (context) => Scaffold( + appBar: AppBar( + actions: [ + IconButton( + onPressed: () { + Navigator.of(context).pushNamed("/second"); + }, + icon: Icon(Icons.settings), + ), + ], + ), + resizeToAvoidBottomInset: false, + body: MobileChatDemo(), + ), + // We include a 2nd screen with navigation so that we can verify + // what happens to the keyboard safe area when navigating from an + // open editor to another screen with a safe area, but no keyboard + // scaffold. See issue #2419 + "/second": (context) => Scaffold( + appBar: AppBar(), + resizeToAvoidBottomInset: false, + body: KeyboardScaffoldSafeArea( + child: ListView.builder( + itemBuilder: (context, index) { + return ListTile( + title: Text("Item $index"), + ); + }, + ), + ), + ), + }, debugShowCheckedModeBanner: false, ), ); diff --git a/super_editor/lib/src/infrastructure/keyboard_panel_scaffold.dart b/super_editor/lib/src/infrastructure/keyboard_panel_scaffold.dart index 7da87e0134..441e53f128 100644 --- a/super_editor/lib/src/infrastructure/keyboard_panel_scaffold.dart +++ b/super_editor/lib/src/infrastructure/keyboard_panel_scaffold.dart @@ -707,6 +707,7 @@ class _KeyboardScaffoldSafeAreaState extends State KeyboardSafeAreaGeometry? _keyboardSafeAreaData; KeyboardScaffoldSafeAreaMutator? _ancestorSafeArea; + bool _isSafeAreaFromMediaQuery = false; @override void didChangeDependencies() { @@ -723,14 +724,46 @@ class _KeyboardScaffoldSafeAreaState extends State // of the editor, not a direct ancestor or descendant. So we need to be able to coordinate // the safe area across independent trees by sharing an ancestor. // - // If there's no existing ancestor KeyboardScaffoldSafeArea, then defer to whatever + // Example: + // KeyboardScaffoldSafeArea + // |- Stack + // |- KeyboardScaffoldSafeArea + // |- Content + // |- SuperEditor + // + // Second, if there's no existing ancestor KeyboardScaffoldSafeArea, then defer to whatever // MediaQuery reports. We only do this for the very first frame because we don't yet // know what our values should be (because that's reported by descendants in the tree). _ancestorSafeArea = KeyboardScaffoldSafeArea.maybeOf(context); - _keyboardSafeAreaData ??= KeyboardSafeAreaGeometry( - bottomInsets: _ancestorSafeArea?.geometry.bottomInsets ?? MediaQuery.viewInsetsOf(context).bottom, - bottomPadding: _ancestorSafeArea?.geometry.bottomPadding ?? MediaQuery.paddingOf(context).bottom, - ); + + if (_keyboardSafeAreaData == null) { + // This is the first call to didChangeDependencies. Initialize our safe area. + _keyboardSafeAreaData = KeyboardSafeAreaGeometry( + bottomInsets: _ancestorSafeArea?.geometry.bottomInsets ?? MediaQuery.viewInsetsOf(context).bottom, + bottomPadding: _ancestorSafeArea?.geometry.bottomPadding ?? MediaQuery.paddingOf(context).bottom, + ); + + // We track whether our safe area is from MediaQuery (instead of an another KeyboardSafeAreaGeometry). + // We do this in case the MediaQuery value changes when we don't have any descendant + // KeyboardPanelScaffold. + // + // For example, you're on Screen 1 with the keyboard up. You navigate to Screen 2, which closes the keyboard. When + // Screen 2 first pumps, it sees that the keyboard is up, so it configures a keyboard safe area. But the keyboard + // immediately closes. Screen 2 is then stuck with a keyboard safe area that never goes away. + // + // By tracking when our safe area comes from MediaQuery, we can continue to honor changing + // MediaQuery values until a descendant explicitly sets our `geometry`. + _isSafeAreaFromMediaQuery = _ancestorSafeArea == null; + } + + if (_isSafeAreaFromMediaQuery) { + // Our current safe area came from MediaQuery, not a descendant. Therefore, + // we want to continue blindly honoring the MediaQuery. + _keyboardSafeAreaData = KeyboardSafeAreaGeometry( + bottomInsets: MediaQuery.viewInsetsOf(context).bottom, + bottomPadding: MediaQuery.paddingOf(context).bottom, + ); + } } @override @@ -738,6 +771,7 @@ class _KeyboardScaffoldSafeAreaState extends State @override set geometry(KeyboardSafeAreaGeometry geometry) { + _isSafeAreaFromMediaQuery = false; if (geometry == _keyboardSafeAreaData) { return; } diff --git a/super_editor/test/infrastructure/keyboard_panel_scaffold_test.dart b/super_editor/test/infrastructure/keyboard_panel_scaffold_test.dart index 652eabaefd..2663dd72e1 100644 --- a/super_editor/test/infrastructure/keyboard_panel_scaffold_test.dart +++ b/super_editor/test/infrastructure/keyboard_panel_scaffold_test.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:flutter_test_runners/flutter_test_runners.dart'; +import 'package:golden_toolkit/golden_toolkit.dart'; import 'package:super_editor/src/test/super_editor_test/supereditor_robot.dart'; import 'package:super_editor/super_editor.dart'; @@ -10,7 +11,7 @@ void main() { group('Keyboard panel scaffold >', () { group('phones >', () { testWidgetsOnMobilePhone('does not show toolbar upon initialization when IME is disconnected', (tester) async { - await _pumpTestApp(tester); + await _pumpTestAppWithTabs(tester); // Ensure the toolbar isn't visible. expect(find.byKey(_aboveKeyboardToolbarKey), findsNothing); @@ -20,7 +21,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -41,7 +42,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -67,7 +68,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -100,7 +101,7 @@ void main() { ); testWidgetsOnMobilePhone('does not show keyboard panel upon keyboard appearance', (tester) async { - await _pumpTestApp(tester); + await _pumpTestAppWithTabs(tester); // Place the caret at the beginning of the document to show the software keyboard. await tester.placeCaretInParagraph('1', 0); @@ -113,7 +114,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -134,7 +135,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -168,7 +169,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -206,7 +207,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -243,7 +244,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -275,7 +276,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -315,7 +316,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -340,7 +341,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -398,7 +399,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -423,7 +424,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -481,7 +482,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -506,7 +507,7 @@ void main() { final softwareKeyboardController = SoftwareKeyboardController(); final controller = KeyboardPanelController(softwareKeyboardController); - await _pumpTestApp( + await _pumpTestAppWithTabs( tester, controller: controller, softwareKeyboardController: softwareKeyboardController, @@ -536,16 +537,65 @@ void main() { // Ensure that the account tab's content is full height (isn't restricted by safe area). expect(tester.getSize(find.byKey(_accountPageKey)).height, contentHeightWithNoKeyboard); }); + + testWidgetsOnMobilePhone('does not retain bottom insets when closing keyboard during navigation', (tester) async { + final navigatorKey = GlobalKey(); + final softwareKeyboardController = SoftwareKeyboardController(); + final controller = KeyboardPanelController(softwareKeyboardController); + + await _pumpTestAppWithNavigationScreens( + tester, + navigatorKey: navigatorKey, + controller: controller, + softwareKeyboardController: softwareKeyboardController, + simulatedKeyboardHeight: _expandedPhoneKeyboardHeight, + ); + + // Record the height of the content when no keyboard is open. + final contentHeightWithNoKeyboard = tester.getSize(find.byKey(_chatPageKey)).height; + + // Show the keyboard. Don't show the toolbar because it's irrelevant for this test. + controller.toolbarVisibility = KeyboardToolbarVisibility.hidden; + controller.showSoftwareKeyboard(); + await tester.pumpAndSettle(); + + // Record the height of the content now that the keyboard is open. + final contentHeightWithKeyboardPanelOpen = tester.getSize(find.byKey(_chatPageKey)).height; + + // Ensure that the content is pushed up above the keyboard. + expect(contentHeightWithNoKeyboard - contentHeightWithKeyboardPanelOpen, _expandedPhoneKeyboardHeight); + + // Navigate to screen 2, while simultaneously closing the keyboard (which is what + // happens when navigating to a new screen without an IME connection). + navigatorKey.currentState!.pushNamed("/second"); + + // CRITICAL: The reason navigation is a problem is because the first pump of a new + // screen happens before the keyboard starts to close (in a real app). Therefore, we + // pump one frame here to create the new screen and THEN we close the keyboard. + await tester.pump(); + + // Close the keyboard now that the new screen is starting to navigate in. + softwareKeyboardController.close(); + + // Pump and settle to let the navigation animation play. + await tester.pumpAndSettle(); + + // Ensure that the second page body takes up all available space. + expect(tester.getSize(find.byKey(_screen2BodyKey)).height, contentHeightWithNoKeyboard); + }); }); }); } -/// Pumps a tree that displays a panel at the software keyboard position. +/// Pumps a tree that displays a two tab UI, one tab has an editor that shows +/// a keyboard panel, and the other tab has no editor at all, which can be +/// used to verify what happens when navigating from an open editor to another +/// tab. /// /// Simulates the software keyboard appearance and disappearance by animating /// the `MediaQuery` view insets when the app communicates with the IME to show/hide /// the software keyboard. -Future _pumpTestApp( +Future _pumpTestAppWithTabs( WidgetTester tester, { KeyboardPanelController? controller, SoftwareKeyboardController? softwareKeyboardController, @@ -675,6 +725,108 @@ const _chatPageKey = ValueKey("chat_content"); const _accountTabKey = ValueKey("account_tab_button"); const _accountPageKey = ValueKey("account_content"); +const _screen2BodyKey = ValueKey("screen_2_body"); + +/// Pumps a tree that can display two screens - the first screen has an editor +/// with a keyboard panel scaffold, the second screen has a keyboard safe area +/// but no editor or keyboard panel scaffold. +/// +/// Simulates the software keyboard appearance and disappearance by animating +/// the `MediaQuery` view insets when the app communicates with the IME to show/hide +/// the software keyboard. +Future _pumpTestAppWithNavigationScreens( + WidgetTester tester, { + required GlobalKey navigatorKey, + KeyboardPanelController? controller, + SoftwareKeyboardController? softwareKeyboardController, + ValueNotifier? isImeConnected, + double simulatedKeyboardHeight = _expandedPhoneKeyboardHeight, +}) async { + final keyboardController = softwareKeyboardController ?? SoftwareKeyboardController(); + final keyboardPanelController = controller ?? KeyboardPanelController(keyboardController); + final imeConnectionNotifier = isImeConnected ?? ValueNotifier(false); + + await tester // + .createDocument() + .withLongDoc() + .withSoftwareKeyboardController(keyboardController) + .withImeConnectionNotifier(imeConnectionNotifier) + .simulateSoftwareKeyboardInsets( + true, + simulatedKeyboardHeight: simulatedKeyboardHeight, + ) + .withCustomWidgetTreeBuilder( + (superEditor) => MaterialApp( + navigatorKey: navigatorKey, + routes: { + '/': (context) { + return _Screen1( + keyboardPanelController: keyboardPanelController, + imeConnectionNotifier: imeConnectionNotifier, + superEditor: superEditor, + ); + }, + '/second': (context) { + return const _Screen2(); + }, + }, + ), + ) + .pump(); +} + +class _Screen1 extends StatelessWidget { + const _Screen1({ + required this.keyboardPanelController, + required this.imeConnectionNotifier, + required this.superEditor, + }); + + final KeyboardPanelController keyboardPanelController; + final ValueNotifier imeConnectionNotifier; + final Widget superEditor; + + @override + Widget build(BuildContext context) { + return Scaffold( + resizeToAvoidBottomInset: false, + body: KeyboardScaffoldSafeArea( + // ^ This safe area is needed to receive the bottom insets from the + // bottom mounted editor, and then make it available to the subtree + // with the content behind the chat. + child: _buildChatPage( + keyboardPanelController, + imeConnectionNotifier, + superEditor, + ), + ), + ); + } +} + +class _Screen2 extends StatelessWidget { + const _Screen2(); + + @override + Widget build(BuildContext context) { + return Scaffold( + resizeToAvoidBottomInset: false, + body: KeyboardScaffoldSafeArea( + child: Builder(builder: (context) { + return ListView.builder( + key: _screen2BodyKey, + itemBuilder: (context, index) { + return ListTile( + title: Text("Item $index"), + ); + }, + ); + }), + ), + ); + } +} + void testWidgetsOnMobilePhone( String description, WidgetTesterCallback test, {