refactor: remove InspectableWebContentsViewMac in favor of the Views version by michal-pichlinski-openfin · Pull Request #44628 · electron/electron

michal-pichlinski-openfin · 2024-11-12T20:52:25Z

Description of Change

This is the re-land of #41326 with fixes for the issues which enforced revert of the refactor.

For ease of review first commit is a cherry-pick of original change e67ab9a without fixing conflicts and build issues. Second commit resolves these conflicts and fixes building Electron. Further commits are fixes for following issues which caused revert:
Fixes #42995
Fixes #43010
Fixes #43003
Fixes #42336
Fixes #42335
Fixes #42975

Checklist

PR description included and stakeholders cc'd
npm test passes

Release Notes

Notes: none

michal-pichlinski-openfin · 2024-11-12T20:55:25Z

@zcbenz Hi! Could you please take a look at these changes?

erickzhao

API LGTM

should be a user-facing noop

itsananderson

API LGTM

Thanks for fixing those regressions so we can re-land this!

codebytere

Thanks for working to get this re-landed!

codebytere · 2024-11-13T09:52:23Z

patches/chromium/fix_put_nsvisualeffectview_before_viewscompositorsuperview.patch

+Date: Tue, 29 Oct 2024 21:16:29 +0100
+Subject: fix: Put NSVisualEffectView before ViewsCompositorSuperview
+
+Otherwise when using `vibrancy` in `BrowserWindow` NSVisualEffectView


What's the path forward for this patch? I'd like to try upstreaming it or discussing it in Chromium issues before potentially floating this patch indefinitely.

Chromium does not use NSVisualEffectView and vibrancy in their codebase, so it is not needed for them, however I'll try to upstream this change.

These patches can potentially be avoided if we swizzle the function sortSubviewsUsingFunction of BridgedContentView. We can override the implementation to sort as needed in Electron.

Take a look at how @gerhardberger was able to do this to override mouse hit behavior: https://github.com/gerhardberger/electron-drag-click/blob/5d6878e308d561c711f22027ba76dad48a8b8143/electron_drag_click.mm#L83-L97

Personally I don't like that idea. My fix is dependent on the SubviewSorter implementation, it is just adding another special view with higher priority. If we use overriding implementation of sortSubviewsUsingFunction we need to copy SubviewSorter implementation and apply to that copy changes which patch introduces. It will be still dependent on the SubviewSorter implementation, but without patch we will not notice changes in the original method or we may miss using our version of SubviewSorter if Chromium starts using its original implementation somewhere else. It may end up with potentially hidden bug if everything luckily compiles, which could be revealed on the gclient sync phase due to conflict during patching. Additionally these overrides look like missing inheritance substituted with inelegant solution. However if you still prefer using method_setImplementation and keeping this change fully in the Electron I will replace this patch with method_setImplementation.

Upstream attempt: https://chromium-review.googlesource.com/c/chromium/src/+/6030552

Chromium team is not responding, can we merge this PR with this patch? Or should I use method_setImplementation in order to move change outside of the patches (in my opinion this is a bad idea: #44628 (comment))

This patch has been upstreamed to Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/6030552

Nice work on upstreaming the patch 👍

Can you add the line below to the patch description so the Upgrades Working Group folks can know that it can be dropped once Chromium is updated?

Upstreamed at https://chromium-review.googlesource.com/c/chromium/src/+/6030552

Applied in: 2cf198e

codebytere · 2024-11-13T10:17:46Z

shell/browser/api/electron_api_base_window.cc


-  // Destroy the native window in next tick because the native code might be
-  // iterating all windows.
-  base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(


This regresses #42975

Fixed in: 7c36dde

Is fix from 7c36dde good enough?

codebytere · 2024-11-13T10:21:31Z

patches/chromium/fix_add_method_which_disables_headless_mode_on_native_widget.patch

+If we don't set `is_headless_` to false then some child windows
+e.g. autofill popups will be created in headless mode leading to
+ui problems (like dissapearing popup during typing in html's
+input list.


Same here - is this an issue upstream would ever face @CezaryKulakowski? Can it be upstreamed?

Unfortunately, I doubt it can be upstreamed, because we are abusing Chromium Widget headless mode for enforcing painting it before we show the window (as it is stated in the original refactor commit):

// We initialize the window in headless mode to allow painting before it is // shown, but we don't want the headless behavior of allowing the window to be // placed unconstrained.

Is this non upstreamable patch a blocker?

I don't think this is a blocker, if you looked into it and it's not tenable to upstream, that's alright, but we wanted to at least consider it

michal-pichlinski-openfin · 2024-11-26T12:12:37Z

@codebytere Could you please respond to my comments in the issues?

michal-pichlinski-openfin · 2024-11-27T10:21:28Z

ping

michal-pichlinski-openfin · 2024-11-28T07:40:36Z

@codebytere ping

michal-pichlinski-openfin · 2024-11-29T12:18:39Z

ping ping

michal-pichlinski-openfin · 2024-12-02T07:26:39Z

ping ping ping

VerteDinde · 2024-12-10T19:45:38Z

@michal-pichlinski-openfin Sorry, we're in the middle of a quiet period right now, so responses will be slow! If you wouldn't mind resolving the conflicts in ui/cocoa, I'll re-run CI and take a look 🙇‍♀️

michal-pichlinski-openfin · 2025-01-03T13:21:54Z

ping

gerhardberger · 2025-01-03T16:36:56Z

would note that with this change in place the resizing experience is much, much smoother and more responsive on macOS compared to without it.

michal-pichlinski-openfin · 2025-01-06T12:49:44Z

ping

michal-pichlinski-openfin · 2025-01-07T16:08:34Z

@VerteDinde @codebytere ping

michal-pichlinski-openfin · 2025-01-08T15:27:12Z

ping ping

michal-pichlinski-openfin · 2025-01-09T16:01:32Z

ping ping ping @VerteDinde @codebytere

michal-pichlinski-openfin · 2025-01-10T07:25:54Z

ping ping ping ping

samuelmaddock · 2025-01-13T16:32:13Z

patches/chromium/fix_put_nsvisualeffectview_before_viewscompositorsuperview.patch

+Date: Tue, 29 Oct 2024 21:16:29 +0100
+Subject: fix: Put NSVisualEffectView before ViewsCompositorSuperview
+
+Otherwise when using `vibrancy` in `BrowserWindow` NSVisualEffectView


Nice work on upstreaming the patch 👍

Can you add the line below to the patch description so the Upgrades Working Group folks can know that it can be dropped once Chromium is updated?

Upstreamed at https://chromium-review.googlesource.com/c/chromium/src/+/6030552

shell/browser/native_window_mac.mm

shell/browser/ui/inspectable_web_contents_view.cc

samuelmaddock

I will run CI and try to pull down the build to test soon.

samuelmaddock · 2025-01-14T16:35:54Z

shell/browser/ui/cocoa/electron_ns_window.mm

+- (void)onElectronNativeWindowClosed {
+  shell_ = nullptr;
+}


I suspect the observer can be replaced by overriding NSWindow's close method.

- (void)close { shell_ = nullptr; [super close]; }

@michal-pichlinski-openfin I made a small refactor to address this 0a53552

@samuelmaddock @michal-pichlinski-openfin We're seeing crash reports which I believe are related to this change. See #47679.

When closing a window, it looks like sometimes a function is called on shell_ which is set to nullptr here.

Any chance you could take a look? :)

shell/browser/native_window_mac.mm

samuelmaddock

I ran e pr download-dist 44628 and tested the build locally against the previous regression issues. Everything seems to be working as expected.

lgtm

…version * cherry-pick: refactor: remove InspectableWebContentsViewMac in favor of the Views version (electron#41326) commit e67ab9a Confilcts not resolved, except removal of the files removed by the original commit. * resolved conflicts and build issues after cherry-pick * cherry-picked: fix: add method allowing to disable headless mode in native widget electron#42996 fixing electron#42995 * fix: displaying select popup in window created as fullscreen window `constrainFrameRect:toScreen:` is not being call for windows created with `fullscreen: true` therefore `headless` mode was not being removed and `RenderWidgetHostNSViewBridge::DisplayPopupMenu` ignored displaying popup. Issue could be fixed by placing additional removal of `headless` mode in the `toggleFullScreen:`, but `orderWindow:relativeTo:` is called both for a regular and a fullscreen window, therefore there will be a single place fixing both cases. Because `electron::NativeWindowMac` lifetime may be shorter than `ElectronNSWindow` on which macOS may execute `orderWindow:relativeTo:` we need to clear `shell_` when `NativeWindow` is being closed. Fixes electron#43010. * fix: Content visibility when using `vibrancy` We need to put `NSVisualEffectView` before `ViewsCompositorSuperview` otherwise when using `vibrancy` in `BrowserWindow` `NSVisualEffectView` will hide content displayed by the compositor. Fixes electron#43003 Fixes electron#42336 In fact main issues reported in these tickets were not present after cherry-picking original refactor switching to `views::WebView`, so text could be selected and click event was properly generated. However both issues testcases were using `vibrancy` and actual content was invisible, because it was covered by the `NSVisualEffectView`. * fix: EXCEPTION_ACCESS_VIOLATION crash on BrowserWindow.destroy() Restored postponed deletion of the `NativeWindow`. Restoration caused `DCHECK(new_parent_ui_layer->GetCompositor());` failure in `BrowserCompositorMac::SetParentUiLayer` after the spec test: `chrome extensions chrome.webRequest does not take precedence over Electron webRequest - http` with stack: ``` 7 Electron Framework 0x000000011fe07830 content::BrowserCompositorMac::SetParentUiLayer(ui::Layer*) + 628 8 Electron Framework 0x000000011fe0c154 content::RenderWidgetHostViewMac::SetParentUiLayer(ui::Layer*) + 220 9 Electron Framework 0x000000011fe226a8 content::WebContentsViewMac::CreateViewForWidget(content::RenderWidgetHost*) + 600 10 Electron Framework 0x000000011fd37e4c content::WebContentsImpl::CreateRenderWidgetHostViewForRenderManager(content::RenderViewHost*) + 164 11 Electron Framework 0x000000011fb32278 content::RenderFrameHostManager::CreateSpeculativeRenderFrame(content::SiteInstanceImpl*, bool, scoped_refptr<content::BrowsingContextState> const&) + 816 12 Electron Framework 0x000000011fb2ab8c content::RenderFrameHostManager::CreateSpeculativeRenderFrameHost(content::SiteInstanceImpl*, content::SiteInstanceImpl*, bool) + 1308 13 Electron Framework 0x000000011fb28598 content::RenderFrameHostManager::GetFrameHostForNavigation(content::NavigationRequest*, content::BrowsingContextGroupSwap*, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*) + 1796 14 Electron Framework 0x000000011fa78660 content::NavigationRequest::SelectFrameHostForOnRequestFailedInternal(bool, bool, std::__Cr::optional<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>> const&) + 280 15 Electron Framework 0x000000011fa6a994 content::NavigationRequest::OnRequestFailedInternal(network::URLLoaderCompletionStatus const&, bool, std::__Cr::optional<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>> const&, bo + 1008 16 Electron Framework 0x000000011fa7772c content::NavigationRequest::OnRequestFailed(network::URLLoaderCompletionStatus const&) + 72 17 Electron Framework 0x000000011f8554ac content::NavigationURLLoaderImpl::NotifyRequestFailed(network::URLLoaderCompletionStatus const&) + 248 ``` This was probably the reason of removing `NativeWindow` immediately in order to cleanup `views_host_` in `WebContentsViewMac` to prevent using layer without compositor in `WebContentsViewMac::CreateViewForWidget`. `[ElectronNSWindowDelegate windowWillClose:]` is deleting window host and the compositor used by the `NativeWindow` therefore detach `NativeWindow` contents from parent. This will clear `views_host_` and prevent failing mentioned `DCHECK`. Fixes electron#42975 * chore: Applied review suggestions

michal-pichlinski-openfin · 2025-01-17T08:54:03Z

ping @codebytere Could you please resolve your change request? Or should change something more?

release-clerk · 2025-01-17T15:21:14Z

No Release Notes

trop · 2025-01-17T15:21:31Z

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

michal-pichlinski-openfin requested a review from a team as a code owner November 12, 2024 20:52

electron-cation bot added the new-pr 🌱 PR opened recently label Nov 12, 2024

VerteDinde added the semver/minor backwards-compatible functionality label Nov 12, 2024

electron-cation bot added the api-review/requested 🗳 label Nov 12, 2024

VerteDinde added the no-backport label Nov 12, 2024

erickzhao approved these changes Nov 12, 2024

View reviewed changes

itsananderson approved these changes Nov 12, 2024

View reviewed changes

codebytere requested changes Nov 13, 2024

View reviewed changes

electron-cation bot added api-review/approved ✅ and removed new-pr 🌱 PR opened recently api-review/requested 🗳 labels Nov 19, 2024

michal-pichlinski-openfin requested review from codebytere and erickzhao November 26, 2024 15:53

michal-pichlinski-openfin requested a review from VerteDinde January 8, 2025 15:27

samuelmaddock reviewed Jan 13, 2025

View reviewed changes

michal-pichlinski-openfin requested a review from samuelmaddock January 14, 2025 12:25

samuelmaddock reviewed Jan 14, 2025

View reviewed changes

michal-pichlinski-openfin requested a review from samuelmaddock January 15, 2025 15:24

samuelmaddock approved these changes Jan 15, 2025

View reviewed changes

michal-pichlinski-openfin force-pushed the mpichlinski/reland-macos-webview/1 branch 2 times, most recently from 88fd302 to 2b6d669 Compare January 15, 2025 21:06

michal-pichlinski-openfin and others added 2 commits January 16, 2025 08:17

refactor: directly cleanup shell

e2962bc

michal-pichlinski-openfin force-pushed the mpichlinski/reland-macos-webview/1 branch from 2b6d669 to e2962bc Compare January 16, 2025 07:17

codebytere approved these changes Jan 17, 2025

View reviewed changes

samuelmaddock added target/35-x-y PR should also be added to the "35-x-y" branch. and removed no-backport labels Jan 17, 2025

samuelmaddock merged commit 6953f55 into electron:main Jan 17, 2025
56 checks passed

trop bot mentioned this pull request Jan 17, 2025

refactor: remove InspectableWebContentsViewMac in favor of the Views version #45238

Merged

trop bot added in-flight/35-x-y merged/35-x-y PR was merged to the "35-x-y" branch. and removed target/35-x-y PR should also be added to the "35-x-y" branch. in-flight/35-x-y labels Jan 17, 2025

clavin mentioned this pull request Mar 29, 2025

Transparent windows flickering and ghosting artifacts on MAS build #46352

Closed

3 tasks

codebytere mentioned this pull request Apr 11, 2025

fix: paint and flash issues on macOS #46615

Merged

4 tasks

rf-figma mentioned this pull request Jun 3, 2025

WebContentsView background doesn't cover existing views until pageload finishes #47351

Open

3 tasks

codebytere mentioned this pull request Jun 10, 2025

[Bug]: Switching between two views back and forth will flicker. #43961

Closed

3 tasks

nikwen mentioned this pull request Jul 14, 2025

Electron crashes on macOS in accessibilityFocusedUIElement #47679

Closed

3 tasks

codebytere mentioned this pull request Jul 15, 2025

fix: handle missing NativeWindowMac in ElectronNSWindow #47751

Merged

3 tasks

nikwen mentioned this pull request Aug 18, 2025

Overlapped WebContentsViews: Underlying element drag triggers top view hover penetration #48098

Open

3 tasks

Conversation

michal-pichlinski-openfin commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

michal-pichlinski-openfin commented Nov 12, 2024

Uh oh!

erickzhao left a comment

Choose a reason for hiding this comment

Uh oh!

itsananderson left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michal-pichlinski-openfin commented Nov 26, 2024

Uh oh!

michal-pichlinski-openfin commented Nov 27, 2024

Uh oh!

michal-pichlinski-openfin commented Nov 28, 2024

Uh oh!

michal-pichlinski-openfin commented Nov 29, 2024

Uh oh!

michal-pichlinski-openfin commented Dec 2, 2024

Uh oh!

VerteDinde commented Dec 10, 2024

Uh oh!

michal-pichlinski-openfin commented Jan 3, 2025

Uh oh!

gerhardberger commented Jan 3, 2025

Uh oh!

michal-pichlinski-openfin commented Jan 6, 2025

Uh oh!

michal-pichlinski-openfin commented Jan 7, 2025

Uh oh!

michal-pichlinski-openfin commented Jan 8, 2025

Uh oh!

michal-pichlinski-openfin commented Jan 9, 2025

Uh oh!

michal-pichlinski-openfin commented Nov 12, 2024 •

edited

Loading