fix: vibrant view is inserted into Views API hierarchy by Hans-Halverson · Pull Request #42263 · electron/electron

Hans-Halverson · 2024-05-24T00:22:44Z

Description of Change

Fixes #41979 in Electron 30.

Summary

In Electron 30 setting a BrowserWindow's vibrancy on macOS will often cause the entire contents of the screen to have the vibrancy effect, wiping all actual window contents. This traces back to #35658 which replaced BrowserViews with WebContentsViews that use Chromium's Views API. We can fix this by inserting the vibrant view into Chromium's Views hierarchy by wrapping the vibrant view in a NativeViewHost.

Before Electron 30

The vibrancy setting creates a native macOS NSVisualEffectView that is inserted as a direct child of the window's contentView here. Prior to the BrowserView -> WebContentsView switch, BrowserViews were backed by native macOS NSViews which were also inserted as direct children of the window's contentView here, and the order of BrowserViews and the vibrancy view was maintained using the macOS NSView ordering APIs.

Electron 30 Breakage

After BrowserView was replaced with WebContentsView, WebContentsViews are no longer ordered using the macOS view ordering APIs. Instead WebContentsViews use Chromium's Views API (i.e. AddChildViewAt, RemoveChildView, etc). The problem is that the vibrancy view is attempting to use the macOS view ordering API whereas all WebContentsViews now use Chromium's Views API.

I think that the exact bug we're seeing in Electron 30 where the vibrancy view is moved to the top and blocks all other contents is because deep inside Chromium the Views API calls macOS's sortSubviewsUsingFunction here, using a function that moves all unregistered views (aka the vibrancy view) to the very top.

This can be solved by adding the vibrancy view into the Chromium views hierarchy so that Chromium knows how to order it appropriately. We can wrap the vibrancy view in a NativeViewHost and insert it as the first child of the root ContentsView so that it is behind both the BrowserWindow's webContents and all of the WebContentsViews.

Notes

This is a PR against 30-x-y since this fix is specific to Electron 30. In Electron 31+ there are different issues with a potentially similar solution for vibrancy (see #41979 (comment) for more details). One proposed fix would be to revert #41326 and then apply this PR which fixes the vibrancy bugs I am seeing in 31+, but for now this PR fixes the 30 branch.

Let me know if there is a better way to get this fix in, since it's technically not a backport to the 30 branch so CI is currently failing.

Checklist

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

Release Notes

Notes: Fixed BrowserWindow vibrancy on macOS

welcome · 2024-05-24T00:22:46Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

Hans-Halverson · 2024-05-31T17:22:27Z

cc'ing stakeholders as mentioned in the PR template (and nobody is currently assigned to this PR), I hope this is appropriate!

@codebytere as the assignee for the relevant issue
@nornagon since this bug appears to be a direct consequence of the BrowserView -> WebContentsView change in #35658

codebytere

Pulled this down and tested it and it works as expected - thanks!

Hans-Halverson · 2024-06-04T17:54:11Z

Thank you so much for taking a look! Is there anything else I need to do to help get this merged into Electron 30?

At this point it looks like just the build-linux step is failing, but I imagine that is a flake given this change doesn't touch non-macOS code (though I don't have the power to retry the job).

samuelmaddock · 2024-06-05T05:28:35Z

Not that this should block merging, but it would be great if we could add visual tests for vibrancy views so we can more easily catch this in the future.

A few tests reference our ScreenCapture helper class which might work here. I suspect a simple visual diff check might also be a good fit, but we haven't instrumented such a test before.

VerteDinde

Looks good to me!

welcome · 2024-06-05T18:14:26Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2024-06-05T18:14:26Z

Release Notes Persisted

Fixed BrowserWindow vibrancy on macOS

* fix: vibrant view is inserted into Views API hierarchy (#42263) * Update shell/browser/native_window_mac.mm Co-authored-by: Charles Kerr <charles@charleskerr.com> --------- Co-authored-by: Hans Halverson <hans_halverson@alumni.brown.edu> Co-authored-by: Charles Kerr <charles@charleskerr.com>

Co-authored-by: Hans Halverson <hans_halverson@alumni.brown.edu>

* fix: vibrant view is inserted into Views API hierarchy (#42263) Co-authored-by: Hans Halverson <hans_halverson@alumni.brown.edu> * Update shell/browser/native_window_mac.mm Co-authored-by: Charles Kerr <charles@charleskerr.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Hans Halverson <hans_halverson@alumni.brown.edu> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

fix: vibrant view is inserted into Views API hierarchy

04fe6df

electron-cation bot added the new-pr 🌱 PR opened recently label May 24, 2024

Hans-Halverson mentioned this pull request May 24, 2024

[Bug]: BrowserView's content is visually wiped when vibrancy is set on Electron version 30 and up #41979

Closed

3 tasks

electron-cation bot removed the new-pr 🌱 PR opened recently label May 31, 2024

Hans-Halverson marked this pull request as ready for review May 31, 2024 17:15

electron-cation bot added the new-pr 🌱 PR opened recently label May 31, 2024

Hans-Halverson mentioned this pull request May 31, 2024

[Bug]: Setting vibrancy on macOS breaks click events on WebContentsView in Electron 31 #42336

Closed

3 tasks

codebytere added the backport-check-skip Skip trop's backport validity checking label Jun 3, 2024

electron-cation bot added new-pr 🌱 PR opened recently and removed new-pr 🌱 PR opened recently labels Jun 3, 2024

codebytere added the semver/patch backwards-compatible bug fixes label Jun 3, 2024

codebytere approved these changes Jun 4, 2024

View reviewed changes

codebytere added the 30-x-y label Jun 4, 2024

VerteDinde approved these changes Jun 5, 2024

View reviewed changes

VerteDinde merged commit e645ea8 into electron:30-x-y Jun 5, 2024

codebytere pushed a commit that referenced this pull request Jul 29, 2024

fix: vibrant view is inserted into Views API hierarchy (#42263)

9e676ec

trop bot added a commit that referenced this pull request Jul 30, 2024

fix: vibrant view is inserted into Views API hierarchy (#42263)

c806d82

Co-authored-by: Hans Halverson <hans_halverson@alumni.brown.edu>

trop bot added a commit that referenced this pull request Jul 30, 2024

fix: vibrant view is inserted into Views API hierarchy (#42263)

7def26d

Co-authored-by: Hans Halverson <hans_halverson@alumni.brown.edu>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: vibrant view is inserted into Views API hierarchy#42263

fix: vibrant view is inserted into Views API hierarchy#42263
VerteDinde merged 1 commit intoelectron:30-x-yfrom
Hans-Halverson:hhalverson/fix-vibrancy-setting-30

Hans-Halverson commented May 24, 2024 •

edited

Loading

Uh oh!

welcome bot commented May 24, 2024

Uh oh!

Hans-Halverson commented May 31, 2024

Uh oh!

codebytere left a comment

Uh oh!

Hans-Halverson commented Jun 4, 2024

Uh oh!

samuelmaddock commented Jun 5, 2024

Uh oh!

VerteDinde left a comment •

edited

Loading

Uh oh!

welcome bot commented Jun 5, 2024

Uh oh!

release-clerk bot commented Jun 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Hans-Halverson commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Summary

Before Electron 30

Electron 30 Breakage

Notes

Checklist

Release Notes

Uh oh!

welcome bot commented May 24, 2024

Uh oh!

Hans-Halverson commented May 31, 2024

Uh oh!

codebytere left a comment

Choose a reason for hiding this comment

Uh oh!

Hans-Halverson commented Jun 4, 2024

Uh oh!

samuelmaddock commented Jun 5, 2024

Uh oh!

VerteDinde left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welcome bot commented Jun 5, 2024

Uh oh!

release-clerk bot commented Jun 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hans-Halverson commented May 24, 2024 •

edited

Loading

VerteDinde left a comment •

edited

Loading