fix: BrowserWindow add the same BrowserView #48053
Conversation
OK |
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
|
|
||
| const ownerWindow = browserView.ownerWindow; | ||
| if (ownerWindow && ownerWindow !== this) { | ||
| ownerWindow.removeBrowserView(browserView); |
There was a problem hiding this comment.
hm, think something might've gotten a little messed up here!
3db2c09 to
21ad54f
Compare
|
| if (this._browserViews.includes(browserView)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This introduces a breaking change to the z-order of BrowserViews: Previously, calling addBrowserView would make the view the top-most view.
This is so useful that it's documented for the successor API.
There was a problem hiding this comment.
addBrowserView did not have this feature originally, so I kept it consistent with the original implementation.
After the BrowserView module was refactored in electron 30, some behaviors changed implicitly, including the 'z-order change' you mentioned.(Refactored implementation)
I think this adjustment should be consistent with the original version. What do you think?
There was a problem hiding this comment.
@nikwen strictly speaking we broke it in the above linked PR without documenting a change so it's not a breaking change in this case - i'm fine with potentially breaking it but it should go through a cycle then and we've deprecated BrowserView already so there's not really a point imo
Description of Change
Closes #48057
Fixed addBrowserView to prevent unnecessary removal and re-adding of the same BrowserView.
Checklist
npm testpassesRelease Notes
Notes: Fixed addBrowserView to prevent unnecessary removal and re-adding of the same BrowserView.