Merge pull request #145 from brave/window-opener-fix

better handling of frames on window start

docs/state.md

@@ -74,7 +74,6 @@ WindowStore
       activeMixedContent: boolean, // has active mixed content
       passiveMixedContent: boolean, // has passive mixed content
     },
-    parentWindowKey: number, // the key of the window this frame was opened from
     parentFrameKey: number, // the key of the frame this frame was opened from
     contextMenuDetail: {...},
     modalPromptDetail: {...},

js/components/frame.js

@@ -27,19 +27,22 @@ class Frame extends ImmutableComponent {
   }
 
   createWebview () {
-    while (this.webviewContainer.firstChild) {
-      this.webviewContainer.removeChild(this.webviewContainer.firstChild)
-    }
     // Create the webview dynamically because React doesn't whitelist all
     // of the attributes we need.
-    this.webview = document.createElement('webview')
+    this.webview = this.webview || document.createElement('webview')
     this.webview.setAttribute('preload', 'content/webviewPreload.js')
     if (this.props.frame.get('isPrivate')) {
       this.webview.setAttribute('partition', 'private-1')
     }
+    if (this.props.frame.get('guestInstanceId')) {
+      this.webview.setAttribute('data-guest-instance-id', this.props.frame.get('guestInstanceId'))
+    }
     this.webview.setAttribute('src', this.props.frame.get('src'))
-    this.webviewContainer.appendChild(this.webview)
-    this.addEventListeners()
+
+    if (!this.webviewContainer.firstChild) {
+      this.webviewContainer.appendChild(this.webview)
+      this.addEventListeners()
+    }
   }
 
   componentDidMount () {
@@ -51,8 +54,7 @@ class Frame extends ImmutableComponent {
     if (didSrcChange) {
       this.createWebview()
     }
-    if ((this.props.isActive && !prevProps.isActive && !this.props.frame.getIn(['navbar', 'urlbar', 'focused'])) ||
-        (this.props.isActive && didSrcChange)) {
+    if (this.props.isActive && !prevProps.isActive) {
       this.webview.focus()
     }
     const activeShortcut = this.props.frame.get('activeShortcut')
@@ -104,33 +106,35 @@ class Frame extends ImmutableComponent {
   }
 
   addEventListeners () {
-    // @see <a href="https://github.com/atom/electron/blob/master/docs/api/web-view-tag.md#event-new-window">new-window event</a>
     this.webview.addEventListener('focus', this.onFocus.bind(this))
-    this.webview.addEventListener('new-window', (e) => {
-      // TODO handle _top, _parent and named frames
-      // also popup blocking and security restrictions!!
+    // @see <a href="https://github.com/atom/electron/blob/master/docs/api/web-view-tag.md#event-new-window">new-window event</a>
+    this.webview.addEventListener('new-window', (e, url, frameName, disposition, options) => {
+      e.preventDefault()
+
+      let guestInstanceId = e.options.webPreferences && e.options.webPreferences.guestInstanceId
+      let windowOptions = e.options.windowOptions || {}
+      windowOptions.parentWindowKey = remote.getCurrentWindow().id
+      windowOptions.disposition = e.disposition
 
-      // @see <a href="http://www.w3.org/TR/html5/browsers.html#dom-open">dom open</a>
-      // @see <a href="http://www.w3.org/TR/html-markup/datatypes.html#common.data.browsing-context-name-or-keyword">browsing context name or keyword</a>
-      if (e.frameName.toLowerCase() === '_self') {
-        WindowActions.loadUrl(this.props.frame, e.url)
-      } else if (e.disposition === 'new-window' || e.frameName.toLowerCase() === '_blank') {
+      if (e.disposition === 'new-window' || e.disposition === 'new-popup') {
         AppActions.newWindow({
           location: e.url,
           parentFrameKey: this.props.frame.get('key'),
-          parentWindowKey: remote.getCurrentWindow().id
-        }, e.options)
+          guestInstanceId
+        }, windowOptions)
       } else {
         WindowActions.newFrame({
           location: e.url,
           parentFrameKey: this.props.frame.get('key'),
-          parentWindowKey: remote.getCurrentWindow().id,
-          openInForeground: e.disposition !== 'background-tab'
+          openInForeground: e.disposition !== 'background-tab',
+          guestInstanceId
         })
       }
     })
+    this.webview.addEventListener('destroyed', (e) => {
+      WindowActions.closeFrame(this.props.frames, this.props.frame)
+    })
     this.webview.addEventListener('close', () => {
-      // security restrictions here?
       AppActions.closeWindow(remote.getCurrentWindow().id)
     })
     this.webview.addEventListener('enter-html-full-screen', () => {
@@ -162,6 +166,11 @@ class Frame extends ImmutableComponent {
         this.webview.canGoBack(),
         this.webview.canGoForward())
     })
+    this.webview.addEventListener('did-navigate', (e) => {
+      if (this.props.isActive && this.webview.getURL() !== Config.defaultUrl) {
+        this.webview.focus()
+      }
+    })
     this.webview.addEventListener('did-start-loading', () => {
       WindowActions.onWebviewLoadStart(
         this.props.frame)

js/components/main.js

@@ -30,12 +30,6 @@ const FrameStateUtil = require('../state/frameStateUtil')
 
 class Main extends ImmutableComponent {
   componentDidMount () {
-    if (this.props.windowState.get('frames').isEmpty()) {
-      WindowActions.newFrame({
-        location: Config.defaultUrl
-      })
-    }
-
     ipc.on(messages.STOP_LOAD, () => {
       electron.remote.getCurrentWebContents().send(messages.SHORTCUT_ACTIVE_FRAME_STOP)
     })
@@ -173,6 +167,7 @@ class Main extends ImmutableComponent {
           sortedFrames.map(frame =>
             <Frame
               ref={`frame${frame.get('key')}`}
+              frames={this.props.windowState.get('frames')}
               frame={frame}
               key={frame.get('key')}
               isPreview={frame.get('key') === this.props.windowState.get('previewFrameKey')}

js/components/window.js

@@ -12,6 +12,7 @@ const Main = require('./main')
 const ipc = global.require('electron').ipcRenderer
 const messages = require('../constants/messages')
 const SiteTags = require('../constants/siteTags')
+const Config = require('../constants/config')
 
 class Window extends React.Component {
   constructor (props) {
@@ -46,9 +47,17 @@ class Window extends React.Component {
   }
 
   componentWillMount () {
-    this.props.frames.forEach((frame) => {
-      WindowActions.newFrame(frame)
-    })
+    if (this.props.frames.length === 0) {
+      WindowActions.newFrame({
+        location: Config.defaultUrl
+      })
+    } else {
+      WindowStore.suspend()
+      this.props.frames.forEach(frame => {
+        WindowActions.newFrame(frame)
+      })
+      WindowStore.resume()
+    }
   }
 
   render () {

js/state/frameStateUtil.js

@@ -135,7 +135,7 @@ export function addFrame (frames, frameOpts, newKey, activeFrameKey) {
     isPinned: frameOpts.isPinned,
     key: newKey,
     parentFrameKey: frameOpts.parentFrameKey,
-    parentWindowKey: frameOpts.parentWindowKey,
+    guestInstanceId: frameOpts.guestInstanceId,
     navbar: {
       searchSuggestions: true,
       focused: true,

js/stores/appStore.js

@@ -31,10 +31,8 @@ function navbarHeight () {
   return 75
 }
 
-const createWindow = (browserOpts, defaults, parentWindowKey) => {
-  browserOpts = browserOpts || {}
-  // clean up properties
-  delete browserOpts.webPreferences
+const createWindow = (browserOpts, defaults) => {
+  let parentWindowKey = browserOpts.parentWindowKey
 
   browserOpts.width = firstDefinedValue(browserOpts.width, browserOpts.innerWidth, defaults.width)
   // height and innerHeight are the frame webview size
@@ -156,6 +154,7 @@ function windowDefaults () {
   setDefaultWindowSize()
 
   return {
+    show: false,
     width: appState.get('defaultWindowWidth'),
     height: appState.get('defaultWindowHeight'),
     minWidth: 500,
@@ -191,9 +190,10 @@ const handleAppAction = (action) => {
       appStore.emitChange()
       break
     case AppConstants.APP_NEW_WINDOW:
-      const frameOpts = action.frameOpts && action.frameOpts.toJS() || undefined
-      const browserOpts = action.browserOpts && action.browserOpts.toJS() || undefined
-      let mainWindow = createWindow(browserOpts, windowDefaults(), frameOpts && frameOpts.parentWindowKey)
+      const frameOpts = action.frameOpts && action.frameOpts.toJS()
+      const browserOpts = (action.browserOpts && action.browserOpts.toJS()) || {}
+
+      let mainWindow = createWindow(browserOpts, windowDefaults())
       if (action.restoredState) {
         mainWindow.webContents.once('dom-ready', () => {
           mainWindow.webContents.send('restore-state', action.restoredState)
@@ -226,6 +226,8 @@ const handleAppAction = (action) => {
         mainWindow.loadURL('file://' + __dirname + '/../../app/index.html?' + queryString)
       }
       appStore.emitChange()
+
+      mainWindow.show()
       break
     case AppConstants.APP_CLOSE_WINDOW:
       let appWindow = BrowserWindow.fromId(action.appWindowId)

js/stores/windowStore.js

@@ -58,6 +58,7 @@ let currentKey = 0
 const incrementNextKey = () => ++currentKey
 
 class WindowStore extends EventEmitter {
+
   getState () {
     return windowState
   }
@@ -67,7 +68,11 @@ class WindowStore extends EventEmitter {
   }
 
   emitChange () {
-    this.emit(CHANGE_EVENT)
+    if (!this.suspended) {
+      this.emit(CHANGE_EVENT)
+    } else {
+      this.emitOnResume = true
+    }
   }
 
   addChangeListener (callback) {
@@ -77,6 +82,26 @@ class WindowStore extends EventEmitter {
   removeChangeListener (callback) {
     this.removeListener(CHANGE_EVENT, callback)
   }
+
+  /**
+   * Temporarily suspend the emitting of change events for this store
+   * You can use this when you have multiple actions that can/should be accomplished in a single render
+   */
+  suspend () {
+    this.suspended = true
+  }
+
+  /**
+   * Resume the emitting of change events
+   * A change event will be emitted if any updates were made while the store was suspended
+   */
+  resume () {
+    this.suspended = false
+    if (this.emitOnResume) {
+      this.emitOnResume = false
+      this.emitChange()
+    }
+  }
 }
 
 const windowStore = new WindowStore()

package.json

@@ -4,8 +4,8 @@
   "description": "Brave laptop and desktop browser",
   "main": "./app/index.js",
   "scripts": {
-    "start": "node ./tools/start.js --debug=5858",
-    "start-brk": "node ./tools/start.js --debug-brk=5858",
+    "start": "node ./tools/start.js --debug=5858 -enable-logging --v=0 --enable-dcheck",
+    "start-brk": "node ./tools/start.js --debug-brk=5858 -enable-logging --v=0 --enable-dcheck",
     "watch-all": "npm run watch & npm run watch-test",
     "watch-test": "NODE_ENV=test webpack --watch",
     "watch": "webpack-dev-server",
@@ -38,6 +38,10 @@
     {
       "name": "Aubrey Keus",
       "email": "[email protected]"
+    },
+    {
+      "name": "Brian Johnson",
+      "email": "[email protected]"
     }
   ],
   "license": "MPL-2.0",
@@ -89,7 +93,7 @@
     "node-static": "^0.7.7",
     "node-uuid": "^1.4.7",
     "pre-commit": "^1.1.2",
-    "spectron": "^0.35.3",
+    "spectron": "^0.36.0",
     "standard": "^5.4.1",
     "style-loader": "^0.13.0",
     "webdriverio": "^3.3.0",

test/fixtures/click_with_target.html

@@ -0,0 +1,13 @@
+<html>
+<head>
+  <meta charset="UTF-8">
+  <title>click.with.target</title>
+</head>
+<body>
+  <a href="page1.html" id="none">none</a>
+  <a href="page1.html" id="name" target="name">name</a>
+  <a href="page2.html" id="name2" target="name">name2</a>
+  <a href="page1.html" id="_self" target="_self">self</a>
+  <iframe id="parent" src="iframe_target_parent.html"/>
+</body>
+</html>

test/fixtures/iframe_target_parent.html

@@ -0,0 +1,10 @@
+<html>
+<head>
+  <meta charset="UTF-8">
+  <title>iframe.target.parent</title>
+</head>
+<body>
+  <a href="page1.html" id="_parent" target="_parent">parent</a>
+  <iframe id="top" src="iframe_target_top.html"/>
+</body>
+</html>

test/fixtures/iframe_target_top.html

@@ -0,0 +1,9 @@
+<html>
+<head>
+  <meta charset="UTF-8">
+  <title>iframe.target.top</title>
+</head>
+<body>
+  <a href="page1.html" id="_top" target="_top">top</a>
+</body>
+</html>

test/lib/brave.js

@@ -8,6 +8,19 @@ chai.use(chaiAsPromised)
 
 const Server = require('./server')
 
+var promiseMapSeries = function (array, iterator) {
+  var length = array.length
+  var current = Promise.resolve()
+  var results = new Array(length)
+
+  for (var i = 0; i < length; ++i) {
+    current = results[i] = current.then(function (i) {
+      return iterator(array[i])
+    }.bind(undefined, i))
+  }
+  return Promise.all(results)
+}
+
 var exports = {
 
   keys: {
@@ -127,6 +140,38 @@ var exports = {
         return require('electron').remote.getCurrentWindow().setSize(width, height)
       }, width, height).then((response) => response.value)
     })
+
+    this.app.client.addCommand('windowParentByUrl', function (url, childSelector = 'webview') {
+      var context = this
+      return this.windowHandles().then(response => response.value).then(function (handles) {
+        return promiseMapSeries(handles, function (handle) {
+          return context.window(handle).getAttribute(childSelector, 'src').catch(() => '')
+        })
+      }).then(function (response) {
+        let index = response.indexOf(url)
+        if (index !== -1) {
+          return context.windowByIndex(index)
+        } else {
+          return undefined
+        }
+      })
+    })
+
+    this.app.client.addCommand('windowByUrl', function (url) {
+      var context = this
+      return this.windowHandles().then(response => response.value).then(function (handles) {
+        return promiseMapSeries(handles, function (handle) {
+          return context.window(handle).getUrl()
+        })
+      }).then(function (response) {
+        let index = response.indexOf(url)
+        if (index !== -1) {
+          return context.windowByIndex(index)
+        } else {
+          return undefined
+        }
+      })
+    })
   },
 
   startApp: function () {

test/lib/server.js

@@ -15,6 +15,14 @@ Server.prototype = {
     return 'http://localhost:' + this.port + '/' + path
   },
 
+  urlWithIpAddress: function (path) {
+    return 'http://127.0.0.1:' + this.port + '/' + path
+  },
+
+  urlOrigin: function () {
+    return 'http://localhost:' + this.port
+  },
+
   /**
    * Sends signal to stop child process and stop server.
    */