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

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

Open
3 tasks done
yangannyx opened this issue Apr 26, 2024 · 5 comments
Assignees
Labels
30-x-y 31-x-y bug 🪲 bug/regression ↩️ A new version of Electron broke something component/BrowserView has-repro-gist Issue can be reproduced with code at https://gist.github.com/

Comments

@yangannyx
Copy link

Preflight Checklist

Electron Version

30.0.0-alpha.1

What operating system are you using?

macOS

Operating System Version

macOS Sonoma 14.4.1 (23E224)

What arch are you using?

arm64 (including Apple Silicon)

Last Known Working Electron version

29.3.1

Expected Behavior

I expect the content of all BrowserViews attached to a BrowserWindow to render its content.

Actual Behavior

The content of one of the BrowserViews attached to my BrowserWindow is completely wiped visually (meaning background color and text content are not visible). However, opening dev tools on that BrowserView's webContents shows that the expected content is mounted in the DOM. Drag regions also seem to be respected.

I'm able to somewhat work around this by adding a dummy BrowserView before adding the BrowserView with content that got wiped. However, this isn't a stable workaround in other projects I work on to that have a more complex BrowserView<>BrowserWindow setup.

Long term, I'm looking to migrate off BrowserViews, since they've been deprecated in Electron v30+, but still want to document this regression.

Testcase Gist URL

https://gist.github.com/b6324fe3c894393da7a09676a927e9d9

Additional Information

No response

@electron-issue-triage electron-issue-triage bot added the has-repro-gist Issue can be reproduced with code at https://gist.github.com/ label Apr 26, 2024
@yangannyx yangannyx changed the title [Bug]: [Bug]: BrowserView's content is visually wiped when vibrancy is set on Electron version 30 and up Apr 26, 2024
@dsanders11 dsanders11 added bug/regression ↩️ A new version of Electron broke something component/BrowserView labels May 6, 2024
@Hans-Halverson
Copy link

This is affecting our app (Figma) as well and I've done some investigation into this issue to try to find a fix. Posting a rather lengthy comment about what I've learned here in case it's helpful.

In short: I think I have a fix for Electron 30 (draft PR here: #42263), but later changes (from #41326) break this fix in Electron 31 and beyond, and I haven't been able to find a working solution myself.

Here's my current understanding of this bug:

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

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.

So I think 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. (Assuming Electron's view hierarchy described here is still correct).

I have done this in a draft PR (#42263) to show what I mean (I can clean this up if it's decided this should become a real PR). This appears to be working for me locally.

Electron 31+

The problem is that this fix no longer works in Electron 31 and above due to the changes made in #41326. When I test my draft PR above on main we're back to the bug where the vibrancy view moves to the top and blocks all other contents. But I can confirm that reverting #41326 and then applying my draft PR fixes vibrancy ordering again.

(As a side note, vibrancy is also really broken on current main without my draft PR but in a different way than the currently described bug. Instead when vibrancy is set the screen sometimes becomes blue, and clicking in places with the vibrancy effect can crash Electron).

I'm less confident about this part, but I think that #41326 prevents the vibrancy fix I described because that PR changed WebContentsViews to no longer be backed by a NativeViewHost. From my understanding the Chromium NSView sorting functions described above only worked because both WebContentsViews and the vibrancy view were all backed by native views via NativeViewHosts. Now that WebContentsViews are no longer NativeViewHosts I can't seem to use the Chromium views API to order the vibrancy view to the back, nor does the macOS views API appear to work either.

Next Steps

I'm happy to put in more effort to help fix this, but I'm getting pretty out of my depth. I'm hoping folks with more knowledge of these systems may be able to chime in or find this investigation useful.

Specifically I'm wondering if it makes sense to tweak (or revert) #41326 in such a way that allows us to layer the vibrancy view to fix this feature. But I don't have much context on the motivations for that PR - cc @nornagon for those questions (I hope the @ mention is appropriate, not sure the ettiquette here).

Or alternatively if there's some other way to layer native NSViews into Chromium's view hierarchy (e.g. via Layers which I explored but have very little context on). Or perhaps Electron could change the native view hierarchy to create a level where the vibrancy view could be inserted without running up against the Chromium Views API?

@codebytere
Copy link
Member

codebytere commented May 30, 2024

@Hans-Halverson @yangannyx maybe i'm missing something, but the behavior on 31 seems to be the same as the behavior on 29 and below?

29

Screenshot 2024-05-30 at 4 22 26 PM

31

Screenshot 2024-05-30 at 4 21 43 PM

@Hans-Halverson
Copy link

Hans-Halverson commented May 30, 2024

@codebytere Sorry let me clarify! The comments I made about Electron 31 were mostly in reference to my PR for Electron 30 no longer working on 31+.

(Without the above patch) The view ordering with vibrancy seems to be working in Electron 31, meaning I see the vibrant view moved to the back behind all the WebContentsViews. But there is some new buggy behavior regarding the vibrancy view happening in Electron 31 that was making it hard for me to narrow down the issue (and this buggy behavior is not seen in the fiddle attached to this issue). I've done a bit more debugging today to clarify exactly what I'm seeing. I unfortunately don't have a fiddle yet to make this easy to repro but I'll keep trying since I know this would be very helpful!

Examples of vibrancy bugs in 31

Here's what I'm seeing in our app (Figma) - it's pretty weird. Basically we've got some configuration of properties (seemingly some combination of vibrancy + WebContentsViews + app-region: drag/no-drag) that causes significant visual bugs and left click events to completely break in regions where the vibrancy effect should be shown. All of these bugs are only present when vibrancy is enabled.

Here are videos showing the visual bugs I'm seeing, both on a local debug build of 31 and an official beta release of 31. I cycled through tabs using keyboard shortcuts in the middle of each video to show some of the strange properties of the visual bugs.

A local (debug) build of the 31-x-y branch at the latest commit as of me posting:

VibrancyBlueDebug31.mov

On 31.0.0-beta.7 the blue screen is not shown where vibrancy should appear, instead it seems that each paint is layered on top of each other (note how the spinners turn into full circles and partially transparent items become really bold):

VibrancyCompoundRelease31.mov

In both videos all tabs in the tab bar are WebContentsViews. Note that if you go to the end of the video all the visual bugs disappear after all WebContentsViews load.

Additionally if you click a tab the app will crash in the debug build on this DCHECK with this stack trace. And in the release build the clicks are silently ignored.

Note that all of the visual and mouse event bugs disappear when I turn off vibrancy, and I can toggle vibrancy on and off and see the visual affects appear and disappear.

Next Steps

I'll work on getting an actual fiddle for this to help debug. The vibrancy bugs in 31 are different enough from 30 that it may be worth moving this to a new issue, but I strongly suspect the underlying reasons are closely related so it might make sense to keep this discussion here.

I have a PR for Electron 30 here that I can clean up and mark as ready to review.

And I can still confirm that on 31-x-y (and on current main), if I revert @nornagon's #41326 then apply my PR both the vibrancy ordering issues and all the vibrancy bugs I described above no longer occur.

Thank you again for any help or advice you can provide here, I really appreciate it! And I'm happy to continue working through this to find a fix for vibrancy in 30, 31, and beyond.

@Hans-Halverson
Copy link

Hans-Halverson commented May 31, 2024

I just published a PR with the fix I found for Electron 30 (described above). #42263

I've also made some progress on getting a repro for the vibrancy bugs I'm seeing in Electron 31+. I've narrowed down the visual bugs to only appearing when an animation is running (in the videos above our tab loading animation). Still haven't gotten a small repro in fiddle yet though.

@Hans-Halverson
Copy link

I've got a repro for the bugs I'm seeing in Electron 31+. Turns out they're not vibrancy related, so I opened up a new issue for them: #42335.

This PR should still fix the vibrant view ordering issue in Electron 30: #42263. And may need to be applied to 31+ as well depending on the fix for the bugs in that other issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
30-x-y 31-x-y bug 🪲 bug/regression ↩️ A new version of Electron broke something component/BrowserView has-repro-gist Issue can be reproduced with code at https://gist.github.com/
Projects
Status: 🛑 Blocks Stable
Status: 🛑 Blocks Stable
Development

No branches or pull requests

5 participants