Skip to content

Commit

Permalink
Options: maintain history entries on the parent frame.
Browse files Browse the repository at this point in the history
Currently, navigations within the child frame remain attached to the child
frame while it signals the parent to change path and title. This doesn't quite
work because location.replace and history.replaceState affect all back/forward
list entries which reference that frame's history entry, rather than just the
current one.

Chrome is currently inconsistent about which of the two behaviors it
implements. Fix the settings page to not rely on the bug so it is future-proof.
Instead, when embedded, all pushState and replaceState calls are proxied up to
the parent which manages history for the child. The child converts all
pushState calls to replaceState calls locally.

This adds uber.setTitle, uber.replaceState, and uber.pushState calls along with
a popState message to uber_utils.js. When running standalone, they call the
standard equivalents. When running within the uber page, they delegate to the
parent as appropriate.

In doing so, this fixes places where chrome://settings-frame and
chrome://settings do not set the same URL and title the same. Notably on the
search page and treating the default "settings" page as having a path of ""
rather than "settings".

BUG=317614
TEST=Go to chrome://settings. Open 'Manage search engines...'. Press back.
     Press refresh. Should stay on search page.

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271538 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
davidben@chromium.org committed May 20, 2014
1 parent b88181d commit 9ff020c
Show file tree
Hide file tree
Showing 14 changed files with 270 additions and 123 deletions.
3 changes: 1 addition & 2 deletions chrome/browser/resources/extensions/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ cr.define('extensions', function() {
measureCheckboxStrings();

// Set the title.
var title = loadTimeData.getString('extensionSettings');
uber.invokeMethodOnParent('setTitle', {title: title});
uber.setTitle(loadTimeData.getString('extensionSettings'));

// This will request the data to show on the page and will get a response
// back in returnExtensionsData.
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/resources/help/help.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ cr.define('help', function() {
uber.onContentFrameLoaded();

// Set the title.
var title = loadTimeData.getString('helpTitle');
uber.invokeMethodOnParent('setTitle', {title: title});
uber.setTitle(loadTimeData.getString('helpTitle'));

$('product-license').innerHTML = loadTimeData.getString('productLicense');
if (cr.isChromeOS) {
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/resources/history/history.js
Original file line number Diff line number Diff line change
Expand Up @@ -1571,8 +1571,7 @@ function load() {
$('filter-controls').hidden = false;
}

var title = loadTimeData.getString('title');
uber.invokeMethodOnParent('setTitle', {title: title});
uber.setTitle(loadTimeData.getString('title'));

// Adjust the position of the notification bar when the window size changes.
window.addEventListener('resize',
Expand Down
12 changes: 3 additions & 9 deletions chrome/browser/resources/options/content_settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,11 @@ cr.define('options', function() {
// history so back/forward and tab restore works.
var hash = event.currentTarget.getAttribute('contentType');
var url = page.name + '#' + hash;
window.history.pushState({pageName: page.name},
page.title,
'/' + url);
uber.pushState({pageName: page.name}, url);

// Navigate after the history has been replaced in order to have the
// correct hash loaded.
// Navigate after the local history has been replaced in order to have
// the correct hash loaded.
OptionsPage.showPageByName('contentExceptions', false);

uber.invokeMethodOnParent('setPath', {path: url});
uber.invokeMethodOnParent('setTitle',
{title: loadTimeData.getString(hash + 'TabTitle')});
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,9 @@ cr.define('options.contentSettings', function() {
* @param {string} type The content type.
*/
showList: function(type) {
// Update the title for the type that was shown.
this.title = loadTimeData.getString(type + 'TabTitle');

var header = this.pageDiv.querySelector('h1');
header.textContent = loadTimeData.getString(type + '_header');

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/resources/options/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ function load() {
var pageName = path.slice(1).replace(/\/$/, '');
OptionsPage.showPageByName(pageName, true, {replaceState: true});
} else {
OptionsPage.showDefaultPage();
OptionsPage.showDefaultPage({replaceState: true});
}

var subpagesNavTabs = document.querySelectorAll('.subpages-nav-tabs');
Expand Down
64 changes: 27 additions & 37 deletions chrome/browser/resources/options/options_page.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,22 @@ cr.define('options', function() {

/**
* Shows the default page.
* @param {Object=} opt_propertyBag An optional bag of properties including
* replaceState (if history state should be replaced instead of pushed).
*/
OptionsPage.showDefaultPage = function() {
this.navigateToPage(this.getDefaultPage().name);
OptionsPage.showDefaultPage = function(opt_propertyBag) {
this.navigateToPage(this.getDefaultPage().name, opt_propertyBag);
};

/**
* "Navigates" to a page, meaning that the page will be shown and the
* appropriate entry is placed in the history.
* @param {string} pageName Page name.
* @param {Object=} opt_propertyBag An optional bag of properties including
* replaceState (if history state should be replaced instead of pushed).
*/
OptionsPage.navigateToPage = function(pageName) {
this.showPageByName(pageName, true);
OptionsPage.navigateToPage = function(pageName, opt_propertyBag) {
this.showPageByName(pageName, true, opt_propertyBag);
};

/**
Expand Down Expand Up @@ -120,6 +124,7 @@ cr.define('options', function() {
if (!targetPage && this.showOverlay_(pageName, rootPage)) {
if (updateHistory)
this.updateHistoryState_(!!opt_propertyBag.replaceState);
this.updateTitle_();
return;
} else {
targetPage = this.getDefaultPage();
Expand Down Expand Up @@ -166,9 +171,6 @@ cr.define('options', function() {
if (updateHistory)
this.updateHistoryState_(!!opt_propertyBag.replaceState);

// Update tab title.
this.setTitle_(targetPage.title);

// Update focus if any other control was focused on the previous page,
// or the previous page is not known.
if (document.activeElement != document.body &&
Expand All @@ -188,16 +190,10 @@ cr.define('options', function() {
page.didShowPage();
}
}
};

/**
* Sets the title of the page. This is accomplished by calling into the
* parent page API.
* @param {string} title The title string.
* @private
*/
OptionsPage.setTitle_ = function(title) {
uber.invokeMethodOnParent('setTitle', {title: title});
// Update the document title. Do this after didShowPage was called, in case
// a page decides to change its title.
this.updateTitle_();
};

/**
Expand All @@ -213,8 +209,17 @@ cr.define('options', function() {
};

/**
* Pushes the current page onto the history stack, overriding the last page
* if it is the generic chrome://settings/.
* Updates the title to title of the current page.
* @private
*/
OptionsPage.updateTitle_ = function() {
var page = this.getTopmostVisiblePage();
uber.setTitle(page.title);
};

/**
* Pushes the current page onto the history stack, replacing the current entry
* if appropriate.
* @param {boolean} replace If true, allow no history events to be created.
* @param {object=} opt_params A bag of optional params, including:
* {boolean} ignoreHash Whether to include the hash or not.
Expand All @@ -229,29 +234,16 @@ cr.define('options', function() {
if (path)
path = path.slice(1).replace(/\/(?:#|$)/, ''); // Remove trailing slash.

// Update tab title.
this.setTitle_(page.title);

// The page is already in history (the user may have clicked the same link
// twice). Do nothing.
if (path == page.name && !OptionsPage.isLoading())
return;

var hash = opt_params && opt_params.ignoreHash ? '' : window.location.hash;

// If settings are embedded, tell the outer page to set its "path" to the
// inner frame's path.
var outerPath = (page == this.getDefaultPage() ? '' : page.name) + hash;
uber.invokeMethodOnParent('setPath', {path: outerPath});

// If there is no path, the current location is chrome://settings/.
// Override this with the new page.
var historyFunction = path && !replace ? window.history.pushState :
window.history.replaceState;
historyFunction.call(window.history,
{pageName: page.name},
page.title,
'/' + page.name + hash);
var newPath = (page == this.getDefaultPage() ? '' : page.name) + hash;
var historyFunction = replace ? uber.replaceState : uber.pushState;
historyFunction.call(uber, {pageName: page.name}, newPath);
};

/**
Expand Down Expand Up @@ -281,9 +273,6 @@ cr.define('options', function() {
if (overlay.didShowPage) overlay.didShowPage();
}

// Update tab title.
this.setTitle_(overlay.title);

// Change focus to the overlay if any other control was focused by keyboard
// before. Otherwise, no one should have focus.
if (document.activeElement != document.body) {
Expand Down Expand Up @@ -350,6 +339,7 @@ cr.define('options', function() {

if (overlay.didClosePage) overlay.didClosePage();
this.updateHistoryState_(false, {ignoreHash: true});
this.updateTitle_();

this.restoreLastFocusedElement_();
};
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/resources/options/search_page.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,7 @@ cr.define('options', function() {
// Set the hash on the current page, and the enclosing uber page
var hash = text ? '#' + encodeURIComponent(text) : '';
var path = text ? this.name : '';
window.location.hash = hash;
uber.invokeMethodOnParent('setPath', {path: path + hash});
uber.pushState({}, path + hash);

// Toggle the search page if necessary.
if (text) {
Expand Down
Loading

0 comments on commit 9ff020c

Please sign in to comment.