From f454cb3408aa2accc4b0c15493d376f34ab6d5ff Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 2 Nov 2016 12:00:28 -0700 Subject: [PATCH 1/6] Add failing spec for hide/show webview issue --- spec/webview-spec.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/webview-spec.js b/spec/webview-spec.js index e00bc222549a..2065eb91dabf 100644 --- a/spec/webview-spec.js +++ b/spec/webview-spec.js @@ -1253,6 +1253,25 @@ describe(' tag', function () { webview.src = 'file://' + fixtures + '/api/blank.html' document.body.appendChild(webview) }) + + it('does not destroy the webContents when hiding/showing the webview (regression)', function (done) { + webview.addEventListener('dom-ready', function domReadyListener () { + const instance = webview.getAttribute('guestinstance') + + // Wait for event directly since attach happens asynchronously over IPC + ipcMain.once('ELECTRON_GUEST_VIEW_MANAGER_ATTACH_GUEST', function () { + assert(webview.getWebContents() != null) + assert.equal(instance, webview.getAttribute('guestinstance')) + done() + }) + + webview.style.display = 'none' + webview.offsetHeight + webview.style.display = 'block' + }) + webview.src = 'file://' + fixtures + '/pages/a.html' + document.body.appendChild(webview) + }) }) describe('DOM events', function () { From a737732521e3ae5730bf1b3450da112f34f38773 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 2 Nov 2016 15:19:16 -0700 Subject: [PATCH 2/6] Only remove guest from embedder when view id changes --- lib/browser/guest-view-manager.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js index 556441f6332d..9ca8326daec9 100644 --- a/lib/browser/guest-view-manager.js +++ b/lib/browser/guest-view-manager.js @@ -162,8 +162,12 @@ const attachGuest = function (embedder, elementInstanceId, guestInstanceId, para if (guestInstance.elementInstanceId) { const oldKey = `${guestInstance.embedder.getId()}-${guestInstance.elementInstanceId}` delete embedderElementsMap[oldKey] - webViewManager.removeGuest(guestInstance.embedder, guestInstanceId) - guestInstance.embedder.send(`ELECTRON_GUEST_VIEW_INTERNAL_DESTROY_GUEST-${guest.viewInstanceId}`) + + // Remove guest from embedder if moving across web views + if (guest.viewInstanceId !== params.instanceId) { + webViewManager.removeGuest(guestInstance.embedder, guestInstanceId) + guestInstance.embedder.send(`ELECTRON_GUEST_VIEW_INTERNAL_DESTROY_GUEST-${guest.viewInstanceId}`) + } } const webPreferences = { From b84fed5cb217adcc2851724cd19f7a3b49034812 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 2 Nov 2016 15:35:00 -0700 Subject: [PATCH 3/6] Add failing spec for hide/show reload issue --- spec/webview-spec.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/webview-spec.js b/spec/webview-spec.js index 2065eb91dabf..bd2834497e3e 100644 --- a/spec/webview-spec.js +++ b/spec/webview-spec.js @@ -1272,6 +1272,25 @@ describe(' tag', function () { webview.src = 'file://' + fixtures + '/pages/a.html' document.body.appendChild(webview) }) + + it('does not reload the webContents when hiding/showing the webview (regression)', function (done) { + webview.addEventListener('dom-ready', function domReadyListener () { + webview.addEventListener('did-start-loading', function () { + done(new Error('webview started loading unexpectedly')) + }) + + // Wait for event directly since attach happens asynchronously over IPC + webview.addEventListener('did-attach', function () { + done() + }) + + webview.style.display = 'none' + webview.offsetHeight + webview.style.display = 'block' + }) + webview.src = 'file://' + fixtures + '/pages/a.html' + document.body.appendChild(webview) + }) }) describe('DOM events', function () { From 133ad6e18b02bc84990d810109a95522363c46b0 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 2 Nov 2016 15:43:19 -0700 Subject: [PATCH 4/6] Only set size and load URL on first attach --- lib/browser/guest-view-manager.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js index 9ca8326daec9..7217d04615d1 100644 --- a/lib/browser/guest-view-manager.js +++ b/lib/browser/guest-view-manager.js @@ -78,7 +78,15 @@ const createGuest = function (embedder, params) { guest.on('did-attach', function () { params = this.attachParams delete this.attachParams + + const previouslyAttached = this.viewInstanceId != null this.viewInstanceId = params.instanceId + + // Only load URL and set size on first attach + if (previouslyAttached) { + return + } + this.setSize({ normal: { width: params.elementWidth, From 18e1de105f3470945be5d60d191fd0c761ec1491 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 2 Nov 2016 17:25:52 -0700 Subject: [PATCH 5/6] Add failing spec for deleted guestinstance when moving webview --- spec/webview-spec.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/webview-spec.js b/spec/webview-spec.js index bd2834497e3e..cc848f9ac547 100644 --- a/spec/webview-spec.js +++ b/spec/webview-spec.js @@ -1254,9 +1254,27 @@ describe(' tag', function () { document.body.appendChild(webview) }) + it('does not delete the guestinstance attribute when moving the webview to another parent node', function (done) { + webview.addEventListener('dom-ready', function domReadyListener () { + webview.addEventListener('did-attach', function () { + assert(webview.guestinstance != null) + assert(webview.getWebContents() != null) + done() + }) + + document.body.replaceChild(webview, div) + }) + webview.src = 'file://' + fixtures + '/pages/a.html' + + const div = document.createElement('div') + div.appendChild(webview) + document.body.appendChild(div) + }) + it('does not destroy the webContents when hiding/showing the webview (regression)', function (done) { webview.addEventListener('dom-ready', function domReadyListener () { const instance = webview.getAttribute('guestinstance') + assert(instance != null) // Wait for event directly since attach happens asynchronously over IPC ipcMain.once('ELECTRON_GUEST_VIEW_MANAGER_ATTACH_GUEST', function () { From 6fda4fc45c42ea5cd6ac17303fb11be5db3d10dd Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 2 Nov 2016 17:28:37 -0700 Subject: [PATCH 6/6] Guard against attachedCallback firing while handling detachedCallback --- lib/renderer/web-view/web-view.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/renderer/web-view/web-view.js b/lib/renderer/web-view/web-view.js index fc8b9a4abdde..27cb07ffcad7 100644 --- a/lib/renderer/web-view/web-view.js +++ b/lib/renderer/web-view/web-view.js @@ -72,9 +72,12 @@ class WebViewImpl { } this.webContents = null - this.attributes[webViewConstants.ATTRIBUTE_GUESTINSTANCE].setValueIgnoreMutation(undefined) this.beforeFirstNavigation = true this.attributes[webViewConstants.ATTRIBUTE_PARTITION].validPartitionId = true + + // Set guestinstance last since this can trigger the attachedCallback to fire + // when moving the webview using element.replaceChild + this.attributes[webViewConstants.ATTRIBUTE_GUESTINSTANCE].setValueIgnoreMutation(undefined) } // Sets the .request property. @@ -310,8 +313,8 @@ var registerWebViewElement = function () { } guestViewInternal.deregisterEvents(internal.viewInstanceId) internal.elementAttached = false - internal.reset() this.internalInstanceId = 0 + internal.reset() } proto.attachedCallback = function () { var internal, instance