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: replace BrowserView with WebContentsView #35658

Merged
merged 103 commits into from Dec 13, 2023
Merged

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Sep 13, 2022

Description of Change

Replaces the experimental BrowserView API with a new WebContentsView API.

  • Documents the existing BaseWindow, which BrowserWindow extends.
    • Currently we duplicate all the documentation for the inherited methods into
      the BrowserWindow docs. Ideally we could deduplicate this, but for
      usability and consistency (avoiding "where did all the BrowserWindow
      methods go???" type questions), we should probably change the docs site to
      account for that change, e.g. to have the duplication of docs for inherited
      methods happen at render time.
  • Adds one new BaseWindow read-write property, contentView.
  • Documents the existing View and WebContentsView.
    • WebContentsView is the replacement for the old BrowserView class. We
      also expose and document the View class, because that is the type
      of BaseWindow.contentView, to which WebContentsViews are added.
  • Adds several new View properties and methods.
    • removeChildView(), children, setBounds(), getBounds(), setBackgroundColor(), setVisible().
  • Deprecates the BrowserView API and updates all references to it to point to WebContentsView instead.
    • We also need a new guide on how to use WebContentsView.
  • Adds a JavaScript shim with the same API surface as the old BrowserView API, which
    under the hood delegates to the new WebContentsView API.
  • Removes BrowserView-specific draggable region handling. This will be
    replaced with refactor: use views NonClientHitTest for draggable regions on mac #35603, but may need some more holes poked to enable draggable
    regions to work correctly on all platforms in WebContentsViews.
  • Changes setAutoResize to behave consistently on all platforms. (The behavior on macOS has been changed to match the behavior on Windows.)

Still TODO:

  • BrowserView had options for auto-sizing. This isn't yet possible with the
    currently exposed APIs of WebContentsView / View, but it should be
    doable!
    This is done, I ported the autoresizing logic from C++ to JS.
  • There is some code about compositor recycling for BrowserViews that I glazed
    over and removed in this PR. I'm not really sure how to test "is this still
    working properly", but it deserves investigation.
    I tested this using the example from fix: Disable new fade animation for BrowserViews #14911 and it seems like things are okay (no flickering)!
  • Test that background colors are working properly. as far as I can tell, they are.
  • Test that onbeforeunload works properly in WebContentsView.
  • Draggable regions are broken in WebContentsView. I think this should be fixed with chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView #35007, so I'm going to say that doesn't block this PR from landing.

Checklist

Release Notes

Notes: Added WebContentsView and BaseWindow, replacing the now-deprecated BrowserView APIs.

@nornagon nornagon added no-backport semver/major incompatible API changes wip ⚒ labels Sep 13, 2022
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Sep 13, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 20, 2022
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

I'm very much in favor of the direction these changes are taking the view-related classes. Cool to see classes move directly into JS. 👍👍

Do you happen to know whether the View class is at all compatible with the design of BaseView in #32890? Would be great if these two proposals could build off of each other.

@nornagon
Copy link
Member Author

My main comment on #32890 is that BaseView should not exist, it should just use View. I believe this was addressed in the RFC, and that the PR is out of date.

Copy link

release-clerk bot commented Dec 13, 2023

Release Notes Persisted

Added WebContentsView and BaseWindow, replacing the now-deprecated BrowserView APIs.

@nornagon nornagon added the target/29-x-y PR should also be added to the "29-x-y" branch. label Dec 13, 2023
@nornagon
Copy link
Member Author

/trop run backport

@trop
Copy link
Contributor

trop bot commented Dec 13, 2023

The backport process for this PR has been manually initiated - here we go! :D

@trop
Copy link
Contributor

trop bot commented Dec 13, 2023

I have automatically backported this PR to "29-x-y", please check out #40759

@trop trop bot added in-flight/29-x-y and removed target/29-x-y PR should also be added to the "29-x-y" branch. labels Dec 13, 2023
@zcbenz
Copy link
Member

zcbenz commented Dec 20, 2023

@nornagon According to @BrandonXLF at #40301 (comment) this PR included a breaking change that made webviews non-transparent by default, are you fine if we merge #40301 which would revert the behavior change?

@nornagon
Copy link
Member Author

nornagon commented Jan 2, 2024

@zcbenz I recall there being some uncertainty about the naming for the property that #40301 adds; I'd rather not gate fixing the bug with non-transparent webviews on resolving that naming issue. But fine if #40301 is going to be merged anyway and it happens to fix this bug :)

@zcbenz
Copy link
Member

zcbenz commented Jan 3, 2024

Yeah there were concerns about API naming in #40301 and eventually a good solution was suggested in the discussion on Slack and the PR implemented it.

Let's merge #40866 first and then #40301 so the code history looks clearer.

@CezaryKulakowski
Copy link
Contributor

I've reported two issues caused by this change:
#41141
#41142

@CezaryKulakowski
Copy link
Contributor

I' ve reported issue for WebContentsView visibility on macOS: #41276

@code-small-white
Copy link

Why didn't I see any related documentation?

@panther7
Copy link

In changelog:

BrowserView is now a shim over WebContentsView and the old implementation has been removed.

But, BrowserView is still here (v30.0.0) ...

@dsanders11
Copy link
Member

@panther7, BrowserView was not removed, the implementation was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/approved ✅ merged/29-x-y PR was merged to the "29-x-y" branch. no-backport semver/major incompatible API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet