From 3ff17ae253093af57f7d6282993eedf0cdf43789 Mon Sep 17 00:00:00 2001 From: yan Date: Tue, 13 Sep 2016 12:49:30 -0700 Subject: [PATCH] Follow-up fixes to #3857 Fix #3928 Fix #3927 Auditors: @bridiver Test Plan: Same as https://github.com/brave/browser-laptop/pull/3857 but also make sure that entering a URL like nytimes.com in the URL bar does not show a blank location before the page is done loading. --- js/actions/windowActions.js | 19 ++++++++++++++----- js/components/frame.js | 8 +++++++- js/stores/windowStore.js | 3 +++ test/components/navigationBarTest.js | 12 ++++++++++-- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 9a5d0e5fde4..8ba106137c6 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -72,14 +72,23 @@ const windowActions = { location }, true) } else { - dispatch({ - actionType: WindowConstants.WINDOW_SET_URL, - location, - key: frame.get('key') - }) + this.setUrl(location, frame.get('key')) } }, + /** + * Dispatches a message to the store to set the new URL. + * @param {string} location + * @param {number} key + */ + setUrl: function (location, key) { + dispatch({ + actionType: WindowConstants.WINDOW_SET_URL, + location, + key + }) + }, + /** * Dispatches a message to the store to let it know a page has been navigated. * diff --git a/js/components/frame.js b/js/components/frame.js index b9161d55817..bfd762cc895 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -23,7 +23,7 @@ const debounce = require('../lib/debounce.js') const getSetting = require('../settings').getSetting const config = require('../constants/config') const settings = require('../constants/settings') -const { aboutUrls, isSourceAboutUrl, isTargetAboutUrl, getTargetAboutUrl, getBaseUrl, isNavigatableAboutPage } = require('../lib/appUrlUtil') +const { aboutUrls, isSourceAboutUrl, isTargetAboutUrl, getSourceAboutUrl, getTargetAboutUrl, getBaseUrl, isNavigatableAboutPage } = require('../lib/appUrlUtil') const { isFrameError } = require('../lib/errorUtil') const locale = require('../l10n') const appConfig = require('../constants/appConfig') @@ -852,6 +852,12 @@ class Frame extends ImmutableComponent { }) windowActions.loadUrl(this.frame, 'about:error') appActions.removeSite(siteUtil.getDetailFromFrame(this.frame)) + } else { + const currentLocation = this.webview.getURL() + if (currentLocation !== e.validatedURL) { + windowActions.setUrl(isTargetAboutUrl(currentLocation) + ? getSourceAboutUrl(currentLocation) : currentLocation, this.props.frameKey) + } } } this.webview.addEventListener('load-commit', (e) => { diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 43d6ae50c9d..e363f197e54 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -293,6 +293,9 @@ const doAction = (action) => { windowState = windowState.mergeIn(tabStatePath(action.key), { location: action.location }) + // Show the location for directly-entered URLs before the page finishes + // loading + updateNavBarInput(action.location, frameStatePath(action.key)) } break case WindowConstants.WINDOW_SET_NAVIGATED: diff --git a/test/components/navigationBarTest.js b/test/components/navigationBarTest.js index 814e2e3aa06..c4732e69605 100644 --- a/test/components/navigationBarTest.js +++ b/test/components/navigationBarTest.js @@ -596,12 +596,20 @@ describe('navigationBar', function () { yield this.app.client.keys('\uE007') }) + it('sets location to new URL immediately', function * () { + yield this.app.client + .waitUntil(function () { + return this.getValue(urlInput).then((val) => { + return val === 'https://bayden.com/test/redir/goscript.aspx' + }) + }) + }) + it('clears urlbar if page does not load', function * () { yield this.app.client .waitUntil(function () { return this.getValue(urlInput).then((val) => { - console.log('value', val) - return val === '' + return val.endsWith('/about-newtab.html') }) }) })