feat: add `win.setTopBrowserView()` so that BrowserViews can be raised #27007

stewartlord · 2020-12-15T01:02:57Z

Description of Change

This PR adds win.setTopBrowserView() so that browser views can be raised without removing them and re-adding them to the window. Raising browser views is necessary to control which view receives pointer events when there are overlapping views. Removing and re-adding a view also works, but has the problem of introducing a noticeable flicker.

I'm unable to write a test for this because it does not appear possible to test which browser view receives pointer events.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added win.setTopBrowserView() so that BrowserViews can be raised.

This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation.

zcbenz

~~It seems that this API is essentially win.removeBrowserView(view) && win.addBrowserView(view)?~~

Sorry I did not see the PR description.

zcbenz · 2020-12-15T02:37:40Z

I think we should probably allow moving browser view to arbitrary position, like win.reorderBrowserView(view, index), otherwise we might have to add another API to move a BrowserView to bottom in future.

shell/browser/api/electron_api_base_window.cc

stewartlord · 2020-12-15T05:25:54Z

I think we should probably allow moving browser view to arbitrary position, like win.reorderBrowserView(view, index), otherwise we might have to add another API to move a BrowserView to bottom in future.

Yes this was on my mind was well. Ultimately I went for the simplest possible solution, but am happy to entertain alternatives.

My hesitation with arbitrary reordering is that I didn't want to take on any additional bookkeeping as I don't believe that the existing browser_views_ (std::map) reflects stacking order. That's not necessarily a problem for Windows and Linux as the underlying Chromium API supports this via ReorderChildView. The Mac/NSView API doesn't work that way, but it is doable with a touch more effort.

Do we think users would be confused that they can set the indexes, but getBrowserViews doesn't reflect the order they set?

zcbenz · 2020-12-15T06:00:14Z

Do we think users would be confused that they can set the indexes, but getBrowserViews doesn't reflect the order they set?

I think if we allow reordering the views then we should make getBrowserViews reflect the order, since this API returns an Array instead of Set.

But yeah it requires additional bookkeeping so it might not really worth the efforts. Not sure how others in @electron/wg-api think.

itsananderson · 2020-12-16T22:42:49Z

Bumped to semver/minor since this is adding new functionality

stewartlord · 2020-12-17T16:39:12Z

Do we think users would be confused that they can set the indexes, but getBrowserViews doesn't reflect the order they set?

I think if we allow reordering the views then we should make getBrowserViews reflect the order, since this API returns an Array instead of Set.

But yeah it requires additional bookkeeping so it might not really worth the efforts. Not sure how others in @electron/wg-api think.

Does the wg-api group meet regularly to consider questions like this? Just curious what the next steps are. Thanks!

stewartlord · 2020-12-28T18:40:29Z

An alternative change that might provoke less anxiety would be to add a moveTop method to BrowserView directly. This would align with the moveTop method on BrowserWindow.

zcbenz · 2021-01-15T00:00:15Z

On the API itself, I prefer having APIs supporting arbitrary reordering, but I'm also OK with a simple setTopBrowserView API.

electron#27007) * feat: Raise a browser view via `BrowserWindow.setTopBrowserView()`. This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation. * feat: setTopBrowserView support for Windows and Linux * docs: add info about setTopBrowserView * docs: Clarify behavior when browserView is not yet attached. * fix: throw en error when browserView is not attached to the window * fix: build error * fix: test * fix: add test case * fix: tests * fix: reparenting * fix: close second window in tests Co-authored-by: sentialx <sentialx@gmail.com>

trop · 2021-02-11T18:33:44Z

@sentialx has manually backported this PR to "10-x-y", please check out #27711

electron#27007) * feat: Raise a browser view via `BrowserWindow.setTopBrowserView()`. This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation. * feat: setTopBrowserView support for Windows and Linux * docs: add info about setTopBrowserView * docs: Clarify behavior when browserView is not yet attached. * fix: throw en error when browserView is not attached to the window * fix: build error * fix: test * fix: add test case * fix: tests * fix: reparenting * fix: close second window in tests Co-authored-by: sentialx <sentialx@gmail.com>

trop · 2021-02-11T18:35:20Z

@sentialx has manually backported this PR to "11-x-y", please check out #27712

electron#27007) * feat: Raise a browser view via `BrowserWindow.setTopBrowserView()`. This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation. * feat: setTopBrowserView support for Windows and Linux * docs: add info about setTopBrowserView * docs: Clarify behavior when browserView is not yet attached. * fix: throw en error when browserView is not attached to the window * fix: build error * fix: test * fix: add test case * fix: tests * fix: reparenting * fix: close second window in tests Co-authored-by: sentialx <sentialx@gmail.com>

trop · 2021-02-11T18:40:04Z

@sentialx has manually backported this PR to "12-x-y", please check out #27713

#27007) (#27712) * feat: Raise a browser view via `BrowserWindow.setTopBrowserView()`. This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation. * feat: setTopBrowserView support for Windows and Linux * docs: add info about setTopBrowserView * docs: Clarify behavior when browserView is not yet attached. * fix: throw en error when browserView is not attached to the window * fix: build error * fix: test * fix: add test case * fix: tests * fix: reparenting * fix: close second window in tests Co-authored-by: sentialx <sentialx@gmail.com> Co-authored-by: Stewart Lord <stew@offbynone.com>

#27007) (#27713) * feat: Raise a browser view via `BrowserWindow.setTopBrowserView()`. This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation. * feat: setTopBrowserView support for Windows and Linux * docs: add info about setTopBrowserView * docs: Clarify behavior when browserView is not yet attached. * fix: throw en error when browserView is not attached to the window * fix: build error * fix: test * fix: add test case * fix: tests * fix: reparenting * fix: close second window in tests Co-authored-by: sentialx <sentialx@gmail.com> Co-authored-by: Stewart Lord <stew@offbynone.com>

#27711) * feat: add `win.setTopBrowserView()` so that BrowserViews can be raised (#27007) * feat: Raise a browser view via `BrowserWindow.setTopBrowserView()`. This is similar to removing and re-adding a browser view, but avoids a visible flicker as the browser view is not removed from the window when using `setTopBrowserView`. Note: if the given browser view is not attached to the window, it will be added. This commit contains the macOS implementation. * feat: setTopBrowserView support for Windows and Linux * docs: add info about setTopBrowserView * docs: Clarify behavior when browserView is not yet attached. * fix: throw en error when browserView is not attached to the window * fix: build error * fix: test * fix: add test case * fix: tests * fix: reparenting * fix: close second window in tests Co-authored-by: sentialx <sentialx@gmail.com> * fix: build error Co-authored-by: Stewart Lord <stew@offbynone.com>

stewartlord and others added 4 commits December 8, 2020 13:40

feat: setTopBrowserView support for Windows and Linux

0c580f2

docs: add info about setTopBrowserView

a5b67de

docs: Clarify behavior when browserView is not yet attached.

5c61f6d

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 15, 2020

zcbenz previously requested changes Dec 15, 2020

View reviewed changes

zcbenz added semver/patch backwards-compatible bug fixes api-review/requested 🗳 labels Dec 15, 2020

codebytere requested changes Dec 15, 2020

View reviewed changes

shell/browser/api/electron_api_base_window.cc Outdated Show resolved Hide resolved

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 16, 2020

itsananderson added semver/minor backwards-compatible functionality and removed semver/patch backwards-compatible bug fixes labels Dec 16, 2020

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 16, 2020

codebytere removed the api-review/requested 🗳 label Dec 17, 2020

electron-cation bot added the api-review/requested 🗳 label Dec 17, 2020

codebytere added semver/major incompatible API changes semver/minor backwards-compatible functionality and removed semver/minor backwards-compatible functionality semver/major incompatible API changes labels Dec 17, 2020

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 22, 2020

jkleinsc added semver/minor backwards-compatible functionality and removed semver/minor backwards-compatible functionality labels Jan 20, 2021

trop bot added needs-manual-bp/11-x-y and removed target/11-x-y labels Feb 11, 2021

sentialx mentioned this pull request Feb 11, 2021

feat: add win.setTopBrowserView() so that BrowserViews can be raised #27711

Merged

trop bot added in-flight/10-x-y and removed needs-manual-bp/10-x-y labels Feb 11, 2021

sentialx mentioned this pull request Feb 11, 2021

feat: add win.setTopBrowserView() so that BrowserViews can be raised #27712

Merged

trop bot added in-flight/11-x-y and removed needs-manual-bp/11-x-y labels Feb 11, 2021

sentialx mentioned this pull request Feb 11, 2021

feat: add win.setTopBrowserView() so that BrowserViews can be raised #27713

Merged

trop bot added in-flight/12-x-y and removed needs-manual-bp/12-x-y labels Feb 11, 2021

sentialx mentioned this pull request Feb 14, 2021

Tracking Electron contributions wexond/browser-base#555

Open

trop bot added merged/11-x-y and removed in-flight/11-x-y labels Feb 18, 2021

trop bot added merged/12-x-y and removed in-flight/12-x-y labels Feb 18, 2021

trop bot removed the in-flight/10-x-y label Feb 18, 2021

trop bot added the merged/10-x-y label Feb 18, 2021

Imperat mentioned this pull request Mar 7, 2023

[Bug]: White frames appear when adding BrowserView with already loaded page #37201

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `win.setTopBrowserView()` so that BrowserViews can be raised #27007

feat: add `win.setTopBrowserView()` so that BrowserViews can be raised #27007

stewartlord commented Dec 15, 2020 •

edited by miniak

zcbenz left a comment •

edited

zcbenz commented Dec 15, 2020

stewartlord commented Dec 15, 2020

zcbenz commented Dec 15, 2020

itsananderson commented Dec 16, 2020

stewartlord commented Dec 17, 2020

stewartlord commented Dec 28, 2020

zcbenz commented Jan 15, 2021

trop bot commented Feb 11, 2021

trop bot commented Feb 11, 2021

trop bot commented Feb 11, 2021

feat: add win.setTopBrowserView() so that BrowserViews can be raised #27007

feat: add win.setTopBrowserView() so that BrowserViews can be raised #27007

Conversation

stewartlord commented Dec 15, 2020 • edited by miniak

Description of Change

Checklist

Release Notes

zcbenz left a comment • edited

Choose a reason for hiding this comment

zcbenz commented Dec 15, 2020

stewartlord commented Dec 15, 2020

zcbenz commented Dec 15, 2020

itsananderson commented Dec 16, 2020

stewartlord commented Dec 17, 2020

stewartlord commented Dec 28, 2020

zcbenz commented Jan 15, 2021

trop bot commented Feb 11, 2021

trop bot commented Feb 11, 2021

trop bot commented Feb 11, 2021

feat: add `win.setTopBrowserView()` so that BrowserViews can be raised #27007

feat: add `win.setTopBrowserView()` so that BrowserViews can be raised #27007

stewartlord commented Dec 15, 2020 •

edited by miniak

zcbenz left a comment •

edited