[web] Popping a nameless route should preserve the correct route name (flutter#173652)

Fixes flutter#173356

Author: mdebbar

engine/src/flutter/lib/web_ui/lib/src/engine/navigation/history.dart

@@ -6,7 +6,6 @@ import 'package:meta/meta.dart';
 import 'package:ui/ui.dart' as ui;
 import 'package:ui/ui_web/src/ui_web.dart' as ui_web;
 
-import '../dom.dart';
 import '../platform_dispatcher.dart';
 import '../services/message_codec.dart';
 import '../services/message_codecs.dart';
@@ -257,20 +256,30 @@ class SingleEntryBrowserHistory extends BrowserHistory {
 
     _setupStrategy(strategy);
 
-    final String path = currentPath;
-    if (!_isFlutterEntry(domWindow.history.state)) {
+    _currentRouteName = currentPath;
+    if (!_isFlutterEntry(currentState)) {
       // An entry may not have come from Flutter, for example, when the user
       // refreshes the page. They land directly on the "flutter" entry, so
       // there's no need to set up the "origin" and "flutter" entries, we can
       // safely assume they are already set up.
       _setupOriginEntry(strategy);
-      _setupFlutterEntry(strategy, path: path);
+      _setupFlutterEntry(strategy);
     }
   }
 
   @override
   final ui_web.UrlStrategy? urlStrategy;
 
+  /// The route name of the current page.
+  ///
+  /// This is updated whenever the framework calls `setRouteName`. This is then
+  /// used when the user hits the back button to pop a nameless route, to restore
+  /// the route name from before the nameless route was pushed.
+  ///
+  /// This is also used to track the user-provided url when they change it
+  /// directly in the address bar.
+  String _currentRouteName = '/';
+
   static const MethodCall _popRouteMethodCall = MethodCall('popRoute');
   static const String _kFlutterTag = 'flutter';
   static const String _kOriginTag = 'origin';
@@ -303,11 +312,11 @@ class SingleEntryBrowserHistory extends BrowserHistory {
   @override
   void setRouteName(String? routeName, {Object? state, bool replace = false}) {
     if (urlStrategy != null) {
-      _setupFlutterEntry(urlStrategy!, replace: true, path: routeName);
+      _currentRouteName = routeName ?? currentPath;
+      _setupFlutterEntry(urlStrategy!, replace: true);
     }
   }
 
-  String? _userProvidedRouteName;
   @override
   void onPopState(Object? state) {
     if (_isOriginEntry(state)) {
@@ -323,17 +332,13 @@ class SingleEntryBrowserHistory extends BrowserHistory {
       // We get into this scenario when the user changes the url manually. It
       // causes a new entry to be pushed on top of our "flutter" one. When this
       // happens it first goes to the "else" section below where we capture the
-      // path into `_userProvidedRouteName` then trigger a history back which
+      // path into `_currentRouteName` then trigger a history back which
       // brings us here.
-      assert(_userProvidedRouteName != null);
-
-      final String newRouteName = _userProvidedRouteName!;
-      _userProvidedRouteName = null;
 
       // Send a 'pushRoute' platform message so the app handles it accordingly.
       EnginePlatformDispatcher.instance.invokeOnPlatformMessage(
         'flutter/navigation',
-        const JSONMethodCodec().encodeMethodCall(MethodCall('pushRoute', newRouteName)),
+        const JSONMethodCodec().encodeMethodCall(MethodCall('pushRoute', _currentRouteName)),
         (_) {},
       );
     } else {
@@ -342,7 +347,7 @@ class SingleEntryBrowserHistory extends BrowserHistory {
       // example.
 
       // 1. We first capture the user's desired path.
-      _userProvidedRouteName = currentPath;
+      _currentRouteName = currentPath;
 
       // 2. Then we remove the new entry.
       // This will take us back to our "flutter" entry and it causes a new
@@ -360,13 +365,9 @@ class SingleEntryBrowserHistory extends BrowserHistory {
 
   /// This method is used manipulate the Flutter Entry which is always the
   /// active entry while the Flutter app is running.
-  void _setupFlutterEntry(ui_web.UrlStrategy strategy, {bool replace = false, String? path}) {
-    path ??= currentPath;
-    if (replace) {
-      strategy.replaceState(_flutterState, 'flutter', path);
-    } else {
-      strategy.pushState(_flutterState, 'flutter', path);
-    }
+  void _setupFlutterEntry(ui_web.UrlStrategy strategy, {bool replace = false}) {
+    final updateState = replace ? strategy.replaceState : strategy.pushState;
+    updateState(_flutterState, 'flutter', _currentRouteName);
   }
 
   @override

engine/src/flutter/lib/web_ui/lib/src/engine/test_embedding.dart

@@ -15,7 +15,7 @@ const bool _debugLogHistoryActions = false;
 class TestHistoryEntry {
   const TestHistoryEntry(this.state, this.title, this.url);
 
-  final dynamic state;
+  final Object? state;
   final String? title;
   final String url;
 
@@ -45,7 +45,7 @@ class TestUrlStrategy implements ui_web.UrlStrategy {
   String getPath() => currentEntry.url;
 
   @override
-  dynamic getState() => currentEntry.state;
+  Object? getState() => currentEntry.state;
 
   int _currentEntryIndex;
   int get currentEntryIndex => _currentEntryIndex;

engine/src/flutter/lib/web_ui/test/engine/history_test.dart

@@ -203,6 +203,8 @@ void testMain() {
       expect(spy.messages[0].channel, 'flutter/navigation');
       expect(spy.messages[0].methodName, 'popRoute');
       expect(spy.messages[0].methodArguments, isNull);
+      // The framework responds by updating to the most current route name.
+      await routeUpdated('/home');
       // We still have 2 entries.
       expect(strategy.history, hasLength(2));
       expect(strategy.currentEntryIndex, 1);
@@ -293,7 +295,7 @@ void testMain() {
       expect(spy.messages[0].methodName, 'pushRoute');
       expect(spy.messages[0].methodArguments, '/page3');
       spy.messages.clear();
-      // 2. The framework sends a `routePushed` platform message.
+      // 2. The framework sends a `routeUpdated` platform message.
       await routeUpdated('/page3');
       // 3. The history state should reflect that /page3 is currently active.
       expect(strategy.history, hasLength(3));
@@ -309,9 +311,9 @@ void testMain() {
       expect(spy.messages[0].methodName, 'popRoute');
       expect(spy.messages[0].methodArguments, isNull);
       spy.messages.clear();
-      // 2. The framework sends a `routePopped` platform message.
+      // 2. The framework sends a `routeUpdated` platform message.
       await routeUpdated('/home');
-      // 3. The history state should reflect that /page1 is currently active.
+      // 3. The history state should reflect that /home is currently active.
       expect(strategy.history, hasLength(2));
       expect(strategy.currentEntryIndex, 1);
       expect(strategy.currentEntry.state, flutterState);
@@ -341,6 +343,35 @@ void testMain() {
       expect(strategy.currentEntry.state, flutterState);
       expect(strategy.currentEntry.url, '/home');
     });
+
+    test('popping a nameless route does not change url', () async {
+      final TestUrlStrategy strategy = TestUrlStrategy.fromEntry(
+        const TestHistoryEntry(null, null, '/home'),
+      );
+      await implicitView.debugInitializeHistory(strategy, useSingle: true);
+
+      // Go to a named route.
+      await routeUpdated('/named-route');
+      expect(strategy.currentEntry.url, '/named-route');
+
+      // Now, push a nameless route. The url shouldn't change.
+      // In a real app, this would be `Navigator.push(context, ...)`;
+      // Here, we simulate it by NOT calling `routeUpdated`.
+
+      // Click back to pop the nameless route.
+      await strategy.go(-1);
+
+      // A `popRoute` message should have been sent to the framework.
+      expect(spy.messages, hasLength(1));
+      expect(spy.messages[0].channel, 'flutter/navigation');
+      expect(spy.messages[0].methodName, 'popRoute');
+
+      // Because the popped route was nameless, the framework doesn't send any updated route
+      // information.
+
+      // The url from before the nameless route should've been preserved.
+      expect(strategy.currentEntry.url, '/named-route');
+    });
   });
 
   group('$MultiEntriesBrowserHistory', () {