Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support electron V5 #47

Merged
merged 20 commits into from Jul 18, 2019
Merged

feat: support electron V5 #47

merged 20 commits into from Jul 18, 2019

Conversation

magne4000
Copy link
Member

@magne4000 magne4000 commented Apr 9, 2019

Diff description

@magne4000 magne4000 requested a review from hugomano April 23, 2019 14:28
@hugomano
Copy link
Contributor

hugomano commented Apr 29, 2019

Origin chrome-extension is now a secure origin in the renderer
Screenshot 2019-04-29 at 16 37 44
Screenshot 2019-04-29 at 16 41 28

@hugomano
Copy link
Contributor

hugomano commented Apr 29, 2019

Blocked by electron/electron#17989 (workaround for electron/electron#18032).

Tried to hook with an event listener like this:

const hookWindowOpen = (webContents) => {
  webContents.on('new-window', (event, url, frameName, disposition, options, additionalFeatures) => {
    event.preventDefault()

    Object.assign(options, {
      show: false,
      width: 600,
      height: 800,
      webPreferences: {
        nodeIntegration: true,
        contextIsolation: false,
        preload: path.join(__dirname, '../renderer/init.js')
      }
    })

    console.log(options);

    const win = new BrowserWindow(options);

    win.once('ready-to-show', () => {
      win.show()
      console.log(url)
      win.webContents.loadURL('https://github.com')
    })

    event.newGuest = win;
  })
}

app.on('web-contents-created', function (event, webContents) {
  hookWindowOpen(webContents)
})

the window.opener stay null after navigation and preloads doesn't load. Really related to the process model IMO.

@hugomano
Copy link
Contributor

hugomano commented Apr 30, 2019

Test with BrowserWindow

Try with the main browser window instead of the webview

function createWindow() {
  mainWindow = new BrowserWindow({
    width: 800,
    height: 600,
    webPreferences: {
      nodeIntegration: true,
      webviewTag: true,
      contextIsolation: false,
      nativeWindowOpen: true,
    }
  });

  mainWindow.loadURL('https://mail.google.com');
}
...

It's the same behavior than our webview:

  • window.open('https://mail.google.com'), opener available but not preloads
  • window.open('https://github.com'), opener not accessible and preloads loaded

Don't attach the newly created window for newGuest on new-open

mainWindow.webContents.on('new-window', (event, url, frameName, disposition, options, additionalFeatures) => {
    ...
})

The options include the embedder through options.webContents which is the reference for window.opener.

In all case, if we create an orphan browser window, the opener is null and preloads are loaded.

Navigation

setup: the opener is webview with domain mail.google.com

  • catch the new-open event for loading mail.google.com: the opener is available but preloads are missing
  • catch the new-open event for loading github.com: the opener is null but preloads are loaded
  • catch the new-open event for loading mail.google.com then navigate to GitHub.com then back to mail.google.com: the opener is null and preloads are loaded

@hugomano hugomano changed the title Electron 5 compat feat(electron): support V5 May 3, 2019
@hugomano hugomano changed the title feat(electron): support V5 feat: support electron V5 May 3, 2019
@hugomano hugomano mentioned this pull request May 9, 2019
//
// used by: Mixmax and other apps using Google OAuth
// issue ref: https://github.com/electron/electron/issues/18032
win.opener = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the fix of electron/electron#18032 in electron 5.0.4, I think we can remove most of these workarounds, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep I on it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants