Settings: manage location.hash explicitly on a Page.

Maintain the current hash associated each Page's state and use that rather than
querying location.hash directly. This fixes several long-standing issues with
the settings search page and the back/forward list:

- Opening an overlay from a search page no longer keeps the fragment in the
  URL.
- Closing an overlay no longer loses the search in the URL.
- Navigating back from one search page to another actually works.
- Navigating back from the search page doesn't clear the search box.

Add tests for this behavior.

BUG=410204

Review URL: https://codereview.chromium.org/555163007

Cr-Commit-Position: refs/heads/master@{#294614}

chrome/browser/resources/help/help.js

@@ -47,6 +47,6 @@
    */
   window.onpopstate = function(e) {
     var pageName = PageManager.getPageNameFromPath();
-    PageManager.setState(pageName, e.state);
+    PageManager.setState(pageName, location.hash, e.state);
   };
 })();

chrome/browser/resources/options/content_settings.js

@@ -33,17 +33,9 @@ cr.define('options', function() {
           this.pageDiv.querySelectorAll('.exceptions-list-button');
       for (var i = 0; i < exceptionsButtons.length; i++) {
         exceptionsButtons[i].onclick = function(event) {
-          var page = ContentSettingsExceptionsArea.getInstance();
-
-          // Add on the proper hash for the content type, and store that in the
-          // history so back/forward and tab restore works.
           var hash = event.currentTarget.getAttribute('contentType');
-          var url = page.name + '#' + hash;
-          uber.pushState({pageName: page.name}, url);
-
-          // Navigate after the local history has been replaced in order to have
-          // the correct hash loaded.
-          PageManager.showPageByName('contentExceptions', false);
+          PageManager.showPageByName('contentExceptions', true,
+                                     {hash: '#' + hash});
         };
       }
 

chrome/browser/resources/options/content_settings_exceptions_area.js

@@ -655,9 +655,8 @@ cr.define('options.contentSettings', function() {
      * location's hash.
      */
     didShowPage: function() {
-      var hash = location.hash;
-      if (hash)
-        this.showList(hash.slice(1));
+      if (this.hash)
+        this.showList(this.hash.slice(1));
     },
   };
 

chrome/browser/resources/options/options.js

@@ -248,7 +248,8 @@ function load() {
   // appropriately to chrome://settings/. If the URL matches, updateHistory_
   // will avoid the extra replaceState.
   var updateHistory = true;
-  PageManager.showPageByName(pageName, updateHistory, {replaceState: true});
+  PageManager.showPageByName(pageName, updateHistory,
+                             {replaceState: true, hash: location.hash});
 
   var subpagesNavTabs = document.querySelectorAll('.subpages-nav-tabs');
   for (var i = 0; i < subpagesNavTabs.length; i++) {
@@ -279,5 +280,5 @@ window.onbeforeunload = function() {
  */
 window.onpopstate = function(e) {
   var pageName = PageManager.getPageNameFromPath();
-  PageManager.setState(pageName, e.state);
+  PageManager.setState(pageName, location.hash, e.state);
 };

chrome/browser/resources/options/search_page.js

@@ -172,6 +172,11 @@ cr.define('options', function() {
       this.setSearchActive_(true);
     },
 
+    /** @override */
+    didChangeHash: function() {
+      this.setSearchActive_(true);
+    },
+
     /**
      * Called before this page will be hidden.
      */
@@ -200,7 +205,7 @@ cr.define('options', function() {
       this.searchActive_ = active;
 
       if (active) {
-        var hash = location.hash;
+        var hash = this.hash;
         if (hash) {
           this.searchField.value =
               decodeURIComponent(hash.slice(1).replace(/\+/g, ' '));
@@ -219,6 +224,8 @@ cr.define('options', function() {
           for (var i = 0, section; section = this.advancedSections_[i]; i++)
             $('settings').appendChild(section);
         }
+      } else {
+        this.searchField.value = '';
       }
 
       var pagesToSearch = this.getSearchablePages_();
@@ -280,25 +287,23 @@ cr.define('options', function() {
       // Cleanup the search query string.
       text = SearchPage.canonicalizeQuery(text);
 
-      // Set the hash on the current page, and the enclosing uber page. Only do
-      // this if the page is not current. See https://crbug.com/401004.
-      var hash = text ? '#' + encodeURIComponent(text) : '';
-      var path = text ? this.name : '';
-      if (location.hash != hash || location.pathname != '/' + path)
-        uber.pushState({}, path + hash);
-
-      // Toggle the search page if necessary.
-      if (text) {
-        if (!this.searchActive_)
-          PageManager.showPageByName(this.name, false);
-      } else {
+      // If the search string becomes empty, flip back to the default page.
+      if (!text) {
         if (this.searchActive_)
-          PageManager.showDefaultPage(false);
-
+          PageManager.showDefaultPage();
         this.insideSetSearchText_ = false;
         return;
       }
 
+      // Toggle the search page if necessary. Otherwise, update the hash.
+      var hash = '#' + encodeURIComponent(text);
+      if (this.searchActive_) {
+        if (this.hash != hash)
+          this.setHash(hash);
+      } else {
+        PageManager.showPageByName(this.name, true, {hash: hash});
+      }
+
       var foundMatches = false;
 
       // Remove any prior search results.

chrome/browser/ui/webui/options/options_browsertest.js

@@ -520,14 +520,25 @@ TEST_F('OptionsWebUIExtendedTest', 'DISABLED_ShowSearchPageNoQuery',
   this.verifyHistory_(['settings'], testDone);
 });
 
+// Manipulate the search page via the search field.
+TEST_F('OptionsWebUIExtendedTest', 'ShowSearchFromField', function() {
+  $('search-field').onsearch({currentTarget: {value: 'query'}});
+  this.verifyOpenPages_(['settings', 'search'], 'search#query');
+  this.verifyHistory_(['', 'search#query'], function() {
+    $('search-field').onsearch({currentTarget: {value: 'query2'}});
+    this.verifyOpenPages_(['settings', 'search'], 'search#query2');
+    this.verifyHistory_(['', 'search#query', 'search#query2'], function() {
+      $('search-field').onsearch({currentTarget: {value: ''}});
+      this.verifyOpenPages_(['settings'], '');
+      this.verifyHistory_(['', 'search#query', 'search#query2', ''], testDone);
+    }.bind(this));
+  }.bind(this));
+});
+
 // Show a page without updating history.
 TEST_F('OptionsWebUIExtendedTest', 'ShowPageNoHistory', function() {
   this.verifyOpenPages_(['settings'], '');
-  // There are only two main pages, 'settings' and 'search'. It's not possible
-  // to show the search page using PageManager.showPageByName, because it
-  // reverts to the settings page if it has no search text set. So we show the
-  // search page by performing a search, then test showPageByName.
-  $('search-field').onsearch({currentTarget: {value: 'query'}});
+  PageManager.showPageByName('search', true, {hash: '#query'});
 
   // The settings page is also still "open" (i.e., visible), in order to show
   // the search results. Furthermore, the URL hasn't been updated in the parent
@@ -543,38 +554,34 @@ TEST_F('OptionsWebUIExtendedTest', 'ShowPageNoHistory', function() {
 });
 
 TEST_F('OptionsWebUIExtendedTest', 'ShowPageWithHistory', function() {
-  // See comments for ShowPageNoHistory.
-  $('search-field').onsearch({currentTarget: {value: 'query'}});
+  PageManager.showPageByName('search', true, {hash: '#query'});
   var self = this;
   this.verifyHistory_(['', 'search#query'], function() {
     PageManager.showPageByName('settings', true);
-    self.verifyOpenPages_(['settings'], '#query');
-    self.verifyHistory_(['', 'search#query', '#query'],
+    self.verifyOpenPages_(['settings'], '');
+    self.verifyHistory_(['', 'search#query', ''],
                         testDone);
   });
 });
 
 TEST_F('OptionsWebUIExtendedTest', 'ShowPageReplaceHistory', function() {
-  // See comments for ShowPageNoHistory.
-  $('search-field').onsearch({currentTarget: {value: 'query'}});
+  PageManager.showPageByName('search', true, {hash: '#query'});
   var self = this;
   this.verifyHistory_(['', 'search#query'], function() {
     PageManager.showPageByName('settings', true, {'replaceState': true});
-    self.verifyOpenPages_(['settings'], '#query');
-    self.verifyHistory_(['', '#query'], testDone);
+    self.verifyOpenPages_(['settings'], '');
+    self.verifyHistory_(['', ''], testDone);
   });
 });
 
 // This should be identical to ShowPageWithHisory.
 TEST_F('OptionsWebUIExtendedTest', 'NavigateToPage', function() {
-  // See comments for ShowPageNoHistory.
-  $('search-field').onsearch({currentTarget: {value: 'query'}});
+  PageManager.showPageByName('search', true, {hash: '#query'});
   var self = this;
   this.verifyHistory_(['', 'search#query'], function() {
     PageManager.showPageByName('settings');
-    self.verifyOpenPages_(['settings'], '#query');
-    self.verifyHistory_(['', 'search#query', '#query'],
-                        testDone);
+    self.verifyOpenPages_(['settings'], '');
+    self.verifyHistory_(['', 'search#query', ''], testDone);
   });
 });
 
@@ -674,6 +681,38 @@ TEST_F('OptionsWebUIExtendedTest', 'CloseOverlay', function() {
   });
 });
 
+// Hashes are maintained separately for each page and are preserved when
+// overlays close.
+TEST_F('OptionsWebUIExtendedTest', 'CloseOverlayWithHashes', function() {
+  // Open an overlay on top of the search page.
+  PageManager.showPageByName('search', true, {hash: '#1'});
+  this.verifyOpenPages_(['settings', 'search'], 'search#1');
+  PageManager.showPageByName('languages', true, {hash: '#2'});
+  this.verifyOpenPages_(['settings', 'search', 'languages'],
+                        'languages#2');
+  PageManager.showPageByName('addLanguage', true, {hash: '#3'});
+  this.verifyOpenPages_(['settings', 'search', 'languages', 'addLanguage'],
+                       'addLanguage#3');
+
+  this.verifyHistory_(['', 'search#1', 'languages#2', 'addLanguage#3'],
+                      function() {
+    // Close the layer-2 overlay.
+    PageManager.closeOverlay();
+    this.verifyOpenPages_(['settings', 'search', 'languages'], 'languages#2');
+    this.verifyHistory_(
+        ['', 'search#1', 'languages#2', 'addLanguage#3', 'languages#2'],
+        function() {
+      // Close the layer-1 overlay.
+      PageManager.closeOverlay();
+      this.verifyOpenPages_(['settings', 'search'], 'search#1');
+      this.verifyHistory_(
+          ['', 'search#1', 'languages#2', 'addLanguage#3', 'languages#2',
+           'search#1'],
+          testDone);
+    }.bind(this));
+  }.bind(this));
+});
+
 // Test that closing an overlay that did not push history when opening does not
 // again push history.
 TEST_F('OptionsWebUIExtendedTest', 'CloseOverlayNoHistory', function() {

ui/webui/resources/js/cr/ui/page_manager/page.js

@@ -28,6 +28,7 @@ cr.define('cr.ui.pageManager', function() {
     this.pageDiv.page = null;
     this.tab = null;
     this.lastFocusedElement = null;
+    this.hash = '';
   }
 
   Page.prototype = {
@@ -65,6 +66,12 @@ cr.define('cr.ui.pageManager', function() {
      */
     initializePage: function() {},
 
+    /**
+     * Called by the PageManager when this.hash changes while the page is
+     * already visible. This is analogous to the hashchange DOM event.
+     */
+    didChangeHash: function() {},
+
     /**
      * Sets focus on the first focusable element. Override for a custom focus
      * strategy.
@@ -117,6 +124,19 @@ cr.define('cr.ui.pageManager', function() {
       return true;
     },
 
+    /**
+     * Updates the hash of the current page. If the page is topmost, the history
+     * state is updated.
+     * @param {string} hash The new hash value. Like location.hash, this
+     *     should include the leading '#' if not empty.
+     */
+    setHash: function(hash) {
+      if (this.hash == hash)
+        return;
+      this.hash = hash;
+      PageManager.onPageHashChanged(this);
+    },
+
     /**
      * Gets the container div for this page if it is an overlay.
      * @type {HTMLDivElement}

ui/webui/resources/js/cr/ui/page_manager/page_manager.js

@@ -139,6 +139,7 @@ cr.define('cr.ui.pageManager', function() {
      *     showing the page (defaults to true).
      * @param {Object=} opt_propertyBag An optional bag of properties including
      *     replaceState (if history state should be replaced instead of pushed).
+     *     hash (a hash state to attach to the page).
      */
     showPageByName: function(pageName,
                              opt_updateHistory,
@@ -163,7 +164,8 @@ cr.define('cr.ui.pageManager', function() {
       var targetPage = this.registeredPages[pageName.toLowerCase()];
       if (!targetPage || !targetPage.canShowPage()) {
         // If it's not a page, try it as an overlay.
-        if (!targetPage && this.showOverlay_(pageName, rootPage)) {
+        var hash = opt_propertyBag.hash || '';
+        if (!targetPage && this.showOverlay_(pageName, hash, rootPage)) {
           if (opt_updateHistory)
             this.updateHistoryState_(!!opt_propertyBag.replaceState);
           this.updateTitle_();
@@ -189,6 +191,9 @@ cr.define('cr.ui.pageManager', function() {
         }
       });
 
+      // Update the page's hash.
+      targetPage.hash = opt_propertyBag.hash || '';
+
       // Update visibilities to show only the hierarchy of the target page.
       this.forEachPage_(!isRootPageLocked, function(page) {
         page.visible = page.name == pageName ||
@@ -215,6 +220,11 @@ cr.define('cr.ui.pageManager', function() {
         }
       });
 
+      // If the target page was already visible, notify it that its hash
+      // changed externally.
+      if (targetPageWasVisible)
+        targetPage.didChangeHash();
+
       // Update the document title. Do this after didShowPage was called, in
       // case a page decides to change its title.
       this.updateTitle_();
@@ -293,6 +303,16 @@ cr.define('cr.ui.pageManager', function() {
         this.updateScrollPosition_();
     },
 
+    /**
+     * Called when a page's hash changes. If the page is the topmost visible
+     * page, the history state is updated.
+     * @param {cr.ui.pageManager.Page} page The page whose hash has changed.
+     */
+    onPageHashChanged: function(page) {
+      if (page == this.getTopmostVisiblePage())
+        this.updateHistoryState_(false);
+    },
+
     /**
      * Returns the topmost visible page, or null if no page is visible.
      * @return {cr.ui.pageManager.Page} The topmost visible page.
@@ -316,7 +336,7 @@ cr.define('cr.ui.pageManager', function() {
 
       if (overlay.didClosePage)
         overlay.didClosePage();
-      this.updateHistoryState_(false, {ignoreHash: true});
+      this.updateHistoryState_(false);
       this.updateTitle_();
 
       this.restoreLastFocusedElement_();
@@ -391,9 +411,10 @@ cr.define('cr.ui.pageManager', function() {
     /**
      * Callback for window.onpopstate to handle back/forward navigations.
      * @param {string} pageName The current page name.
+     * @param {string} hash The hash to pass into the page.
      * @param {Object} data State data pushed into history.
      */
-    setState: function(pageName, data) {
+    setState: function(pageName, hash, data) {
       var currentOverlay = this.getVisibleOverlay_();
       var lowercaseName = pageName.toLowerCase();
       var newPage = this.registeredPages[lowercaseName] ||
@@ -403,7 +424,7 @@ cr.define('cr.ui.pageManager', function() {
         currentOverlay.visible = false;
         if (currentOverlay.didClosePage) currentOverlay.didClosePage();
       }
-      this.showPageByName(pageName, false);
+      this.showPageByName(pageName, false, {hash: hash});
     },
 
 
@@ -454,12 +475,13 @@ cr.define('cr.ui.pageManager', function() {
     /**
      * Shows a registered overlay page. Does not update history.
      * @param {string} overlayName Page name.
+     * @param {string} hash The hash state to associate with the overlay.
      * @param {cr.ui.pageManager.Page} rootPage The currently visible root-level
      *     page.
      * @return {boolean} Whether we showed an overlay.
      * @private
      */
-    showOverlay_: function(overlayName, rootPage) {
+    showOverlay_: function(overlayName, hash, rootPage) {
       var overlay = this.registeredOverlayPages[overlayName.toLowerCase()];
       if (!overlay || !overlay.canShowPage())
         return false;
@@ -475,10 +497,13 @@ cr.define('cr.ui.pageManager', function() {
         this.showPageByName(overlay.parentPage.name, false);
       }
 
+      overlay.hash = hash;
       if (!overlay.visible) {
         overlay.visible = true;
         if (overlay.didShowPage)
           overlay.didShowPage();
+      } else {
+        overlay.didChangeHash();
       }
 
       // Change focus to the overlay if any other control was focused by
@@ -584,11 +609,9 @@ cr.define('cr.ui.pageManager', function() {
      * to update the history.
      * @param {boolean} replace If true, handlers should replace the current
      *     history event rather than create new ones.
-     * @param {Object=} opt_params A bag of optional params, including:
-     *     {boolean} ignoreHash Whether to include the hash or not.
      * @private
      */
-    updateHistoryState_: function(replace, opt_params) {
+    updateHistoryState_: function(replace) {
       if (this.isDialog)
         return;
 
@@ -601,9 +624,7 @@ cr.define('cr.ui.pageManager', function() {
 
       // If the page is already in history (the user may have clicked the same
       // link twice, or this is the initial load), do nothing.
-      var hash = opt_params && opt_params.ignoreHash ?
-          '' : window.location.hash;
-      var newPath = (page == this.defaultPage_ ? '' : page.name) + hash;
+      var newPath = (page == this.defaultPage_ ? '' : page.name) + page.hash;
       if (path == newPath)
         return;