Skip to content

Commit

Permalink
Reland "Fix Backbutton is not displayed when there is a endDrawer (#1… (
Browse files Browse the repository at this point in the history
flutter#104110)

* Reland "Fix Backbutton is not displayed when there is a endDrawer (flutter#102093)"

This reverts commit a4a8e73.

* add todos
  • Loading branch information
chunhtai authored May 24, 2022
1 parent c29a7a2 commit bd7d34f
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 10 deletions.
8 changes: 5 additions & 3 deletions packages/flutter/lib/src/material/app_bar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -970,9 +970,11 @@ class _AppBarState extends State<AppBar> {
onPressed: _handleDrawerButton,
tooltip: MaterialLocalizations.of(context).openAppDrawerTooltip,
);
} else {
if (!hasEndDrawer && canPop)
leading = useCloseButton ? const CloseButton() : const BackButton();
// TODO(chunhtai): remove (!hasEndDrawer && canPop) once internal tests
// are migrated.
// https://github.com/flutter/flutter/issues/80256.
} else if ((!hasEndDrawer && canPop) || (parentRoute?.impliesAppBarDismissal ?? false)) {
leading = useCloseButton ? const CloseButton() : const BackButton();
}
}
if (leading != null) {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter/lib/src/material/drawer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ class DrawerControllerState extends State<DrawerController> with SingleTickerPro
if (_historyEntry == null) {
final ModalRoute<dynamic>? route = ModalRoute.of(context);
if (route != null) {
_historyEntry = LocalHistoryEntry(onRemove: _handleHistoryEntryRemoved);
_historyEntry = LocalHistoryEntry(onRemove: _handleHistoryEntryRemoved, impliesAppBarDismissal: false);
route.addLocalHistoryEntry(_historyEntry!);
FocusScope.of(context).setFirstFocus(_focusScopeNode);
}
Expand Down
48 changes: 42 additions & 6 deletions packages/flutter/lib/src/widgets/routes.dart
Original file line number Diff line number Diff line change
Expand Up @@ -455,13 +455,21 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
/// An entry in the history of a [LocalHistoryRoute].
class LocalHistoryEntry {
/// Creates an entry in the history of a [LocalHistoryRoute].
LocalHistoryEntry({ this.onRemove });
///
/// The [impliesAppBarDismissal] defaults to true if not provided.
LocalHistoryEntry({ this.onRemove, this.impliesAppBarDismissal = true });

/// Called when this entry is removed from the history of its associated [LocalHistoryRoute].
final VoidCallback? onRemove;

LocalHistoryRoute<dynamic>? _owner;

/// Whether an [AppBar] in the route this entry belongs to should
/// automatically add a back button or close button.
///
/// Defaults to true.
final bool impliesAppBarDismissal;

/// Remove this entry from the history of its associated [LocalHistoryRoute].
void remove() {
_owner?.removeLocalHistoryEntry(this);
Expand All @@ -482,7 +490,7 @@ class LocalHistoryEntry {
/// is removed from the list and its [LocalHistoryEntry.onRemove] is called.
mixin LocalHistoryRoute<T> on Route<T> {
List<LocalHistoryEntry>? _localHistory;

int _entriesImpliesAppBarDismissal = 0;
/// Adds a local history entry to this route.
///
/// When asked to pop, if this route has any local history entries, this route
Expand Down Expand Up @@ -620,7 +628,12 @@ mixin LocalHistoryRoute<T> on Route<T> {
_localHistory ??= <LocalHistoryEntry>[];
final bool wasEmpty = _localHistory!.isEmpty;
_localHistory!.add(entry);
if (wasEmpty)
bool internalStateChanged = false;
if (entry.impliesAppBarDismissal) {
internalStateChanged = _entriesImpliesAppBarDismissal == 0;
_entriesImpliesAppBarDismissal += 1;
}
if (wasEmpty || internalStateChanged)
changedInternalState();
}

Expand All @@ -632,10 +645,15 @@ mixin LocalHistoryRoute<T> on Route<T> {
assert(entry != null);
assert(entry._owner == this);
assert(_localHistory!.contains(entry));
_localHistory!.remove(entry);
bool internalStateChanged = false;
if (_localHistory!.remove(entry) && entry.impliesAppBarDismissal) {
_entriesImpliesAppBarDismissal -= 1;
internalStateChanged = _entriesImpliesAppBarDismissal == 0;
}
entry._owner = null;
entry._notifyRemoved();
if (_localHistory!.isEmpty) {
if (_localHistory!.isEmpty || internalStateChanged) {
assert(_entriesImpliesAppBarDismissal == 0);
if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.persistentCallbacks) {
// The local history might be removed as a result of disposing inactive
// elements during finalizeTree. The state is locked at this moment, and
Expand Down Expand Up @@ -663,7 +681,12 @@ mixin LocalHistoryRoute<T> on Route<T> {
assert(entry._owner == this);
entry._owner = null;
entry._notifyRemoved();
if (_localHistory!.isEmpty)
bool internalStateChanged = false;
if (entry.impliesAppBarDismissal) {
_entriesImpliesAppBarDismissal -= 1;
internalStateChanged = _entriesImpliesAppBarDismissal == 0;
}
if (_localHistory!.isEmpty || internalStateChanged)
changedInternalState();
return false;
}
Expand Down Expand Up @@ -697,6 +720,7 @@ class _ModalScopeStatus extends InheritedWidget {
const _ModalScopeStatus({
required this.isCurrent,
required this.canPop,
required this.impliesAppBarDismissal,
required this.route,
required super.child,
}) : assert(isCurrent != null),
Expand All @@ -706,12 +730,14 @@ class _ModalScopeStatus extends InheritedWidget {

final bool isCurrent;
final bool canPop;
final bool impliesAppBarDismissal;
final Route<dynamic> route;

@override
bool updateShouldNotify(_ModalScopeStatus old) {
return isCurrent != old.isCurrent ||
canPop != old.canPop ||
impliesAppBarDismissal != old.impliesAppBarDismissal ||
route != old.route;
}

Expand All @@ -720,6 +746,7 @@ class _ModalScopeStatus extends InheritedWidget {
super.debugFillProperties(description);
description.add(FlagProperty('isCurrent', value: isCurrent, ifTrue: 'active', ifFalse: 'inactive'));
description.add(FlagProperty('canPop', value: canPop, ifTrue: 'can pop'));
description.add(FlagProperty('impliesAppBarDismissal', value: impliesAppBarDismissal, ifTrue: 'implies app bar dismissal'));
}
}

Expand Down Expand Up @@ -822,6 +849,7 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
route: widget.route,
isCurrent: widget.route.isCurrent, // _routeSetState is called if this updates
canPop: widget.route.canPop, // _routeSetState is called if this updates
impliesAppBarDismissal: widget.route.impliesAppBarDismissal,
child: Offstage(
offstage: widget.route.offstage, // _routeSetState is called if this updates
child: PageStorage(
Expand Down Expand Up @@ -1561,6 +1589,14 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T
/// notified.
bool get canPop => hasActiveRouteBelow || willHandlePopInternally;

/// Whether an [AppBar] in the route should automatically add a back button or
/// close button.
///
/// This getter returns true if there is at least one active route below it,
/// or there is at least one [LocalHistoryEntry] with `impliesAppBarDismissal`
/// set to true
bool get impliesAppBarDismissal => hasActiveRouteBelow || _entriesImpliesAppBarDismissal > 0;

// Internals

final GlobalKey<_ModalScopeState<T>> _scopeKey = GlobalKey<_ModalScopeState<T>>();
Expand Down
38 changes: 38 additions & 0 deletions packages/flutter/test/material/app_bar_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3267,6 +3267,44 @@ void main() {
});
});

// Regression test for https://github.com/flutter/flutter/issues/80256
testWidgets('The second page should have a back button even it has a end drawer', (WidgetTester tester) async {
final Page<void> page1 = MaterialPage<void>(
key: const ValueKey<String>('1'),
child: Scaffold(
key: const ValueKey<String>('1'),
appBar: AppBar(),
endDrawer: const Drawer(),
)
);
final Page<void> page2 = MaterialPage<void>(
key: const ValueKey<String>('2'),
child: Scaffold(
key: const ValueKey<String>('2'),
appBar: AppBar(),
endDrawer: const Drawer(),
)
);
final List<Page<void>> pages = <Page<void>>[ page1, page2 ];
await tester.pumpWidget(
MaterialApp(
home: Navigator(
pages: pages,
onPopPage: (Route<Object?> route, Object? result) => false,
),
),
);

// The page2 should have a back button.
expect(
find.descendant(
of: find.byKey(const ValueKey<String>('2')),
matching: find.byType(BackButton),
),
findsOneWidget
);
});

testWidgets('AppBar.preferredHeightFor', (WidgetTester tester) async {
late double preferredHeight;
late Size preferredSize;
Expand Down

0 comments on commit bd7d34f

Please sign in to comment.