fix: handle window.open events in BrowserView #12686

Anrock · 2018-04-22T12:14:05Z

Not sure if it's a proper fix, but Work For Me™
Basically it's just a mindless copypaste of -new-window, -web-contents-created, -add-new-contents handlers from lib/browser/api/browser-window.js _init along with relevant unit-tests.

notes: Fixed child windows invisible if opened with window.open from BrowserView with nativeWindowOpen enabled

Anrock · 2018-04-22T12:16:39Z

Also, personally i need this backported to 1.7.x and 1.8.x branches but i haven't found the docs on how to do this: from pull request guides it seems only permitted target branch is master. Is there any docs on backporting process or could someone explain it to me?

poiru · 2018-04-23T17:55:13Z

lib/browser/api/browser-view.js


 Object.setPrototypeOf(BrowserView.prototype, EventEmitter.prototype)
+BrowserView.prototype._init = function () {
+  // Make new windows requested by links behave like "window.open"
+  this.webContents.on('-new-window', (event, url, frameName, disposition,


Maybe we could just do this in lib/browser/api/web-contents.js to share the impl between BrowserWindow/BrowserView? cc @zcbenz

Yeah, that seems like a nice fix of "mindless copypaste".

@poiru how do i handle commits with fixes from review? Is it ok to fixup it all and push -f or is it better to lay on top of existing commits?

@Anrock Either way is fine with me. Feel free to just make new commits. We can squash it all when merging.

Anrock · 2018-04-24T08:20:12Z

@poiru i've added second commit that moves common handlers from BrowserView and BrowserWindow to WebContents. Also rebased on fresh master.

poiru

LGTM, thank you!

Anrock · 2018-04-24T09:10:11Z

@poiru thanks. Can it be also ported to 1.7? AFAIK there were some conflicts - i can make separate PR with adapted version.

MarshallOfSound · 2018-04-24T09:59:17Z

This is something that can't be backported right? It's semver/feature 🤔

Anrock · 2018-04-24T10:35:23Z

@MarshallOfSound i'd say it's a bugfix. At least i expect, from user perspective, that BrowserView should behave identically irregardless if nativeWindowOpen is true or false. Just like BrowserWindow does.

MarshallOfSound · 2018-04-24T10:53:23Z

@Anrock There's a line between "adding a feature that probably should have been there in the first place" and "fixing something that wasn't functioning correctly". I think this is quite nicely fitting in the first category

Anrock · 2018-04-24T11:11:10Z

@MarshallOfSound ok.
Only thing that bothers me now is what should i do to get this changes to 1.7 and 1.8 branches, please advice.

poiru · 2018-04-26T18:22:44Z

@ckerr Can you merge?

Anrock · 2018-05-11T07:30:26Z

@poiru @ckerr any eta on merge? Two weeks and no movement.

MarshallOfSound · 2018-05-11T07:47:02Z

The failing tests are blocking this

poiru · 2018-05-12T08:02:20Z

@Anrock The test failures might be flakes. Can you rebase on top of master and force push?

alexeykuzmin · 2018-08-31T19:04:35Z

@Anrock
Can you please rebase your branch onto the latest master and resolve merge conflicts?

Anrock · 2018-09-03T11:15:33Z

@alexeykuzmin in progress. However i suspect that test failures are not flakes actually.

Anrock · 2018-09-03T13:41:42Z

@alexeykuzmin done. But i don't have time to investigate test failures.

Akshat-Jain

Please resolve the conflicts. Thanks :-)

Anrock · 2018-09-21T09:00:06Z

@Akshat-Jain done. I think it's time to scrap this PR and rewrite from scratch. Looks like handlers have changed multiple times and given non-trivial conflicts i suspect something is missed in action.

Anrock · 2018-10-04T14:00:27Z

Can someone help with broken tests?
linux-arm-debug, linux-arm-testing, linux-ia32-testing: failed build with sscache error
inux-x64-testing-tests, linux-x64-testing-verify-ffmpeg: failed tests with cannot open display
osx-testing: marked as failed, but circleci build step is not even finished yet

Anrock · 2018-10-05T08:51:19Z

@Akshat-Jain i've rewritten this from scratch, so PR needs re-review.

Anrock · 2018-10-05T13:17:23Z

Looks like #14984 should fix part of failing linux tests.

Anrock · 2018-10-08T10:46:32Z

@alexeykuzmin regarding #15016, this PR rebased on master and in branch log i see that branch includes 918488a commit that should fix build breakage, however build is still broken.
Anything else i'm missing?

Anrock · 2018-10-11T10:02:00Z

@Akshat-Jain ping

codebytere · 2018-10-11T17:28:32Z

hi @Anrock, @akshat-jain isn't involved in this project at all; you can see his review wasn't a valid one as it was a grey check. You can ping anyone in @electron/reviewers for future review!

Anrock · 2018-10-11T17:52:17Z

@codebytere thank you! How do i find member list of @electron/reviewers?

codebytere · 2018-10-18T03:05:05Z

ah i mean you can literally tag @electron/reviewers!

Anrock · 2018-10-18T07:56:26Z

@codebytere for some reason it doesn't appear in autocompletion list and not marked as mention after i manually type it.

MarshallOfSound · 2018-10-18T08:26:38Z

@Anrock All the test failures seem related to your code changes. For instance:

BrowserWindow module "webPreferences" option "sandbox" option should inherit the sandbox setting in opened windows - should inherit the sandbox setting in opened windows

Without green builds we can't look at merging this, you can run the tests locally with npm run test to verify before pushing to our CI.

codebytere · 2018-11-08T16:04:47Z

@Anrock are you still interested in moving forward with this?

Anrock · 2018-11-08T19:42:11Z

@codebytere yes, but i really don't have time to do it - busy fixing window.opener in my app. And i guess code is gone stale again. Should i close PR then or there is someone else interested doing it?

zcbenz · 2018-11-27T02:11:24Z

I have rebased the branch and added some fixes and tests.

release-clerk · 2018-11-27T06:59:20Z

Release Notes Persisted

Fixed child windows invisible if opened with window.open from BrowserView with nativeWindowOpen enabled

welcome · 2018-11-27T06:59:21Z

Congrats on merging your first pull request! 🎉🎉🎉

BinaryMuse · 2018-12-13T23:26:05Z

/trop run backport-to 4-0-x

fix: handle window.open events in BrowserView

Anrock requested a review from a team April 22, 2018 12:14

MarshallOfSound added the semver/minor backwards-compatible functionality label Apr 22, 2018

MarshallOfSound requested a review from a team April 22, 2018 12:28

poiru reviewed Apr 23, 2018

View reviewed changes

poiru approved these changes Apr 24, 2018

View reviewed changes

poiru added target/1-8-x labels Apr 24, 2018

MarshallOfSound removed target/1-8-x labels Apr 24, 2018

Anrock changed the title ~~handle window.open events in BrowserView~~ fix: handle window.open events in BrowserView Sep 3, 2018

Akshat-Jain reviewed Sep 14, 2018

View reviewed changes

Anrock closed this Oct 4, 2018

Anrock reopened this Oct 4, 2018

Anrock mentioned this pull request Oct 8, 2018

ci: "Generate breakpad symbols" fails on linux-*-testing #15016

Closed

Anrock and others added 3 commits November 27, 2018 10:59

fix: move window.open handlers from BrowserWindow to WebContents

9ff7807

fix: remove event monitor before destroying window

d1f0d6c

test: add test for window.open in BrowserView

63874da

zcbenz approved these changes Nov 27, 2018

View reviewed changes

zcbenz merged commit aafbd86 into electron:master Nov 27, 2018

Anrock deleted the browserview-handle-window-open branch December 4, 2018 11:27

BinaryMuse pushed a commit that referenced this pull request Dec 13, 2018

Merge pull request #12686 from Anrock/browserview-handle-window-open

4a53fb3

fix: handle window.open events in BrowserView

fix: handle window.open events in BrowserView #12686

fix: handle window.open events in BrowserView #12686

Conversation

Anrock commented Apr 22, 2018 • edited

Anrock commented Apr 22, 2018

poiru Apr 23, 2018

Choose a reason for hiding this comment

Anrock Apr 23, 2018

Choose a reason for hiding this comment

Anrock Apr 23, 2018

Choose a reason for hiding this comment

poiru Apr 23, 2018 • edited

Choose a reason for hiding this comment

Anrock commented Apr 24, 2018

poiru left a comment

Choose a reason for hiding this comment

Anrock commented Apr 24, 2018 • edited

MarshallOfSound commented Apr 24, 2018

Anrock commented Apr 24, 2018

MarshallOfSound commented Apr 24, 2018

Anrock commented Apr 24, 2018

poiru commented Apr 26, 2018

Anrock commented May 11, 2018

MarshallOfSound commented May 11, 2018

poiru commented May 12, 2018

alexeykuzmin commented Aug 31, 2018

Anrock commented Sep 3, 2018

Anrock commented Sep 3, 2018

Akshat-Jain left a comment

Choose a reason for hiding this comment

Anrock commented Sep 21, 2018

Anrock commented Oct 4, 2018

Anrock commented Oct 5, 2018

Anrock commented Oct 5, 2018

Anrock commented Oct 8, 2018 • edited

Anrock commented Oct 11, 2018

codebytere commented Oct 11, 2018

Anrock commented Oct 11, 2018

codebytere commented Oct 18, 2018

Anrock commented Oct 18, 2018

MarshallOfSound commented Oct 18, 2018

codebytere commented Nov 8, 2018

Anrock commented Nov 8, 2018

zcbenz commented Nov 27, 2018

release-clerk bot commented Nov 27, 2018

welcome bot commented Nov 27, 2018

BinaryMuse commented Dec 13, 2018

Anrock commented Apr 22, 2018 •

edited

poiru Apr 23, 2018 •

edited

Anrock commented Apr 24, 2018 •

edited

Anrock commented Oct 8, 2018 •

edited