From dfb36f032e6d3c2a0102f469c32ddce45c8a74ce Mon Sep 17 00:00:00 2001 From: Victor Sanni Date: Tue, 4 Feb 2025 17:49:25 -0800 Subject: [PATCH] Make CupertinoSheetRoute usable with Cupertino(Sliver)NavigationBar (#162181) Working on the cupertino nav bars recently gave me some context on those widgets, so when I saw this [comment](https://github.com/flutter/flutter/pull/157568#issuecomment-2590955571) I was inspired to add a fix :) Thanks @MaherSafadii for [starting the exploration](https://github.com/flutter/flutter/pull/157568#issuecomment-2592535332) and also for the very helpful [screenshots](https://github.com/flutter/flutter/issues/162021#issuecomment-2608430023). Removes the following when CupertinoNavigationBar/CupertinoSliverNavigationBar is used in a CupertinoSheetRoute: - Unneeded back button - Superfluous top padding in CupertinoNavigationBar - Full page route transitions ## Before: https://github.com/user-attachments/assets/a6da3957-0cff-4491-9380-bbc676ac799d ## After: https://github.com/user-attachments/assets/37cd1628-a47e-44aa-85c7-abceda6e7944
Sample code ```dart // Copyright 2014 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. import 'package:flutter/cupertino.dart'; /// Flutter code sample for [CupertinoSheetRoute]. class CupertinoSheetApp extends StatelessWidget { const CupertinoSheetApp({super.key}); @override Widget build(BuildContext context) { return const CupertinoApp(title: 'Cupertino Sheet', home: HomePage()); } } class HomePage extends StatelessWidget { const HomePage({super.key}); @override Widget build(BuildContext context) { return CupertinoPageScaffold( navigationBar: const CupertinoNavigationBar( middle: Text('Sheet Example'), automaticBackgroundVisibility: false, ), child: Center( child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ CupertinoButton.filled( onPressed: () { Navigator.of(context).push( CupertinoSheetRoute( builder: (BuildContext context) => const _SheetScaffold(), ), ); }, child: const Text('Open Bottom Sheet'), ), ], ), ), ); } } class _SheetScaffold extends StatelessWidget { const _SheetScaffold(); @override Widget build(BuildContext context) { return CupertinoPageScaffold( navigationBar: CupertinoNavigationBar( backgroundColor: CupertinoColors.systemGrey.withOpacity(0.5), middle: const Text('CupertinoNavigationBar Sample'), automaticBackgroundVisibility: false, ), child: Column( children: [ Container(height: 50, color: CupertinoColors.systemRed), Container(height: 50, color: CupertinoColors.systemGreen), Container(height: 50, color: CupertinoColors.systemBlue), Container(height: 50, color: CupertinoColors.systemYellow), Center( child: CupertinoButton.filled( onPressed: () { Navigator.of(context).push( CupertinoSheetRoute( builder: (BuildContext context) => const SliverNavBarExample(), ), ); }, child: const Text('Open Bottom Sheet'), ), ) ], ), ); } } class SliverNavBarExample extends StatelessWidget { const SliverNavBarExample({super.key}); @override Widget build(BuildContext context) { return CupertinoPageScaffold( child: CustomScrollView( slivers: [ const CupertinoSliverNavigationBar( leading: Icon(CupertinoIcons.person_2), largeTitle: Text('Contacts'), trailing: Icon(CupertinoIcons.add_circled), ), SliverFillRemaining( child: Padding( padding: const EdgeInsets.symmetric(horizontal: 10.0), child: Column( mainAxisAlignment: MainAxisAlignment.spaceEvenly, children: [ const Text('Drag me up', textAlign: TextAlign.center), CupertinoButton.filled( onPressed: () {}, child: const Text('Bottom Automatic mode'), ), CupertinoButton.filled( onPressed: () {}, child: const Text('Bottom Always mode'), ), ], ), ), ), ], ), ); } } ```
Fixes [Cupertino navbars apply too much top padding within a sheet](https://github.com/flutter/flutter/issues/162021). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --- .../flutter/lib/src/cupertino/nav_bar.dart | 16 ++++-- .../flutter/test/cupertino/nav_bar_test.dart | 54 +++++++++++++++++-- .../cupertino/nav_bar_transition_test.dart | 47 ++++++++++++++++ 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/nav_bar.dart b/packages/flutter/lib/src/cupertino/nav_bar.dart index 283cc8058cb..488c62b2e34 100644 --- a/packages/flutter/lib/src/cupertino/nav_bar.dart +++ b/packages/flutter/lib/src/cupertino/nav_bar.dart @@ -20,6 +20,7 @@ import 'icons.dart'; import 'page_scaffold.dart'; import 'route.dart'; import 'search_field.dart'; +import 'sheet.dart'; import 'theme.dart'; /// Modes that determine how to display the navigation bar's bottom in relation to scroll events. @@ -212,7 +213,9 @@ bool _isTransitionable(BuildContext context) { // Fullscreen dialogs never transitions their nav bar with other push-style // pages' nav bars or with other fullscreen dialog pages on the way in or on // the way out. - return route is PageRoute && !route.fullscreenDialog; + return route is PageRoute && + !route.fullscreenDialog && + !CupertinoSheetRoute.hasParentSheet(context); } /// An iOS-styled navigation bar. @@ -1571,7 +1574,10 @@ class _PersistentNavigationBar extends StatelessWidget { final Widget? backChevron = components.backChevron; final Widget? backLabel = components.backLabel; - if (leading == null && backChevron != null && backLabel != null) { + if (leading == null && + backChevron != null && + backLabel != null && + !CupertinoSheetRoute.hasParentSheet(context)) { leading = CupertinoNavigationBarBackButton._assemble(backChevron, backLabel); } @@ -1591,7 +1597,11 @@ class _PersistentNavigationBar extends StatelessWidget { return SizedBox( height: _kNavBarPersistentHeight + MediaQuery.paddingOf(context).top, - child: SafeArea(bottom: false, child: paddedToolbar), + child: SafeArea( + top: !CupertinoSheetRoute.hasParentSheet(context), + bottom: false, + child: paddedToolbar, + ), ); } } diff --git a/packages/flutter/test/cupertino/nav_bar_test.dart b/packages/flutter/test/cupertino/nav_bar_test.dart index c8fd39155de..676faecc529 100644 --- a/packages/flutter/test/cupertino/nav_bar_test.dart +++ b/packages/flutter/test/cupertino/nav_bar_test.dart @@ -461,8 +461,6 @@ void main() { testWidgets('Media padding is applied to CupertinoSliverNavigationBar', ( WidgetTester tester, ) async { - final ScrollController scrollController = ScrollController(); - addTearDown(scrollController.dispose); final Key leadingKey = GlobalKey(); final Key middleKey = GlobalKey(); final Key trailingKey = GlobalKey(); @@ -475,7 +473,6 @@ void main() { ), child: CupertinoPageScaffold( child: CustomScrollView( - controller: scrollController, slivers: [ CupertinoSliverNavigationBar( leading: Placeholder(key: leadingKey), @@ -797,6 +794,57 @@ void main() { expect(find.text('Home page'), findsOneWidget); }); + testWidgets('Navigation bars in a CupertinoSheetRoute have no back button', ( + WidgetTester tester, + ) async { + await tester.pumpWidget( + const CupertinoApp(home: CupertinoNavigationBar(middle: Text('Home page'))), + ); + + expect(find.byType(CupertinoButton), findsNothing); + + tester + .state(find.byType(Navigator)) + .push( + CupertinoSheetRoute( + builder: (BuildContext context) { + return const CupertinoPageScaffold( + navigationBar: CupertinoNavigationBar(middle: Text('Page 2')), + child: Placeholder(), + ); + }, + ), + ); + + await tester.pump(); + await tester.pump(const Duration(milliseconds: 600)); + + // No back button is found. + expect(find.byType(CupertinoButton), findsNothing); + expect(find.text(String.fromCharCode(CupertinoIcons.back.codePoint)), findsNothing); + + tester + .state(find.byType(Navigator)) + .push( + CupertinoSheetRoute( + builder: (BuildContext context) { + return const CupertinoPageScaffold( + child: CustomScrollView( + slivers: [CupertinoSliverNavigationBar(largeTitle: Text('Page 3'))], + ), + ); + }, + ), + ); + + await tester.pump(); + await tester.pump(const Duration(milliseconds: 600)); + + // No back button is found. + expect(find.byType(CupertinoButton), findsNothing); + expect(find.text(String.fromCharCode(CupertinoIcons.back.codePoint)), findsNothing); + }); + testWidgets('Long back label turns into "back"', (WidgetTester tester) async { await tester.pumpWidget(const CupertinoApp(home: Placeholder())); diff --git a/packages/flutter/test/cupertino/nav_bar_transition_test.dart b/packages/flutter/test/cupertino/nav_bar_transition_test.dart index 0d1ff01918d..48d5e13aa1b 100644 --- a/packages/flutter/test/cupertino/nav_bar_transition_test.dart +++ b/packages/flutter/test/cupertino/nav_bar_transition_test.dart @@ -343,6 +343,53 @@ void main() { expect(() => flying(tester, find.text('Page 2')), throwsAssertionError); }); + testWidgets('Navigation bars in a CupertinoSheetRoute have no hero transitions', ( + WidgetTester tester, + ) async { + await tester.pumpWidget( + CupertinoApp( + builder: (BuildContext context, Widget? navigator) { + return navigator!; + }, + home: const Placeholder(), + ), + ); + + tester + .state(find.byType(Navigator)) + .push( + CupertinoSheetRoute( + builder: + (BuildContext context) => + scaffoldForNavBar(const CupertinoNavigationBar(middle: Text('Page 1')))!, + ), + ); + + await tester.pump(); + await tester.pump(const Duration(milliseconds: 600)); + + tester + .state(find.byType(Navigator)) + .push( + CupertinoSheetRoute( + builder: + (BuildContext context) => + scaffoldForNavBar( + const CupertinoSliverNavigationBar(largeTitle: Text('Page 2')), + )!, + ), + ); + + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + + expect(find.byType(Hero), findsNothing); + + // No Hero transition happened. + expect(() => flying(tester, find.text('Page 1')), throwsAssertionError); + expect(() => flying(tester, find.text('Page 2')), throwsAssertionError); + }); + testWidgets('Popping mid-transition is symmetrical', (WidgetTester tester) async { await startTransitionBetween(tester, fromTitle: 'Page 1');