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

fix: BrowserViews not painting their WebContents #29919

Merged
merged 1 commit into from Jul 29, 2021

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 28, 2021

Description of Change

Closes #29859.

Fixes an issue where BrowserViews would appear not to load their webContents. Further investigation showed that it was in fact not that the webContents weren't being loaded, but that they weren't being painted properly. If I tried to open devTools on the webContents or called setBounds after the initial URL load it would work - this tracked back to Layout in inspectable_web_contents_view_views.h and to a hierarchy change in https://chromium-review.googlesource.com/c/chromium/src/+/2780032. Essentially, the change meant that Layout was no longer called by Chromium in some of the places it was before.

To fix this, I added an override for RenderViewReady and called into Layout() so that paints would happen properly again when they were supposed to.

To verify:

const { BrowserView, BrowserWindow, app } = require('electron')

app.whenReady().then(async () => {
  let win = new BrowserWindow({ width: 800, height: 600 })
  win.on('closed', () => {
    win = null
  })
  const view = new BrowserView({
    webPreferences: {
      nodeIntegration: false
    }
  })
  win.setBrowserView(view)
  view.setBounds({ x: 0, y: 0, width: 800, height: 600 })
  await view.webContents.loadURL('https://electronjs.org')
})

Checklist

Release Notes

Notes: Fixed an issue where BrowserView webContents would appear not to load in some circumstances.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/14-x-y labels Jun 28, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 28, 2021
@codebytere codebytere requested a review from jkleinsc June 28, 2021 17:09
Copy link
Member

@nornagon nornagon 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 skeptical that DidFinishNavigation is the right place to hook into to call Layout() from. What lead you to that conclusion?

Should we be calling this instead when the view is added to the hierarchy?

@codebytere
Copy link
Member Author

@nornagon i was seeking something I could guarantee would be after the webContents finished loading - i also tried RenderFrameCreated to no success. What were you envisioning? Happy to try something else!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 29, 2021
@MarshallOfSound MarshallOfSound self-assigned this Jun 30, 2021
@LGUG2Z LGUG2Z mentioned this pull request Jul 22, 2021
11 tasks
@zcbenz
Copy link
Member

zcbenz commented Jul 28, 2021

#29607 also looks similar.

Can you check if the contents_web_view_ in InspectableWebContentsViewViews has a proper size? My first instinct is that contents_web_view_ somehow did not get sized after before loading URL.

@codebytere
Copy link
Member Author

@zcbenz looks like the bounds are correct in both cases, unfortunately.

@zcbenz
Copy link
Member

zcbenz commented Jul 29, 2021

It seems that changing the size of views::WebView no longer changes the size of WebContents if RenderWidget is not ready, this behavior makes sense to me but I don't know why resizing used to work.

Calling Layout() in DidFirstVisuallyNonEmptyPaint or RenderViewReady should be able to work around the problem.

BrowserWindow has this code so it is probably why it does not suffer from this problem:

void BrowserWindow::DidFirstVisuallyNonEmptyPaint() {
if (window()->IsClosed() || window()->IsVisible())
return;
// When there is a non-empty first paint, resize the RenderWidget to force
// Chromium to draw.
auto* const view = web_contents()->GetRenderWidgetHostView();
view->Show();
view->SetSize(window()->GetContentSize());
}

Also note that the above code came from #6026, and since the implementation of ready-to-show event has changed we should probably remove it, or update its comments if it is still needed.

@codebytere
Copy link
Member Author

Using RenderViewReady works! Updated.

@codebytere codebytere requested a review from zcbenz July 29, 2021 17:54
@codebytere codebytere force-pushed the fix-browserview-webcontents branch 3 times, most recently from f5ff36b to fcd7578 Compare July 29, 2021 17:55
shell/browser/native_browser_view_views.cc Outdated Show resolved Hide resolved
shell/browser/native_browser_view_views.cc Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the fix-browserview-webcontents branch 2 times, most recently from 6c9eb19 to c037aa6 Compare July 29, 2021 20:29
@zcbenz zcbenz merged commit 639f039 into main Jul 29, 2021
@zcbenz zcbenz deleted the fix-browserview-webcontents branch July 29, 2021 23:59
@release-clerk
Copy link

release-clerk bot commented Jul 29, 2021

Release Notes Persisted

Fixed an issue where BrowserView webContents would appear not to load in some circumstances.

@trop
Copy link
Contributor

trop bot commented Jul 29, 2021

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

@trop
Copy link
Contributor

trop bot commented Jul 29, 2021

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

BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
@codebytere
Copy link
Member Author

/trop run backport-to 13-x-y

@trop
Copy link
Contributor

trop bot commented Sep 21, 2021

The backport process for this PR has been manually initiated - sending your PR to 13-x-y!

@trop
Copy link
Contributor

trop bot commented Sep 21, 2021

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

@TianbinTobin
Copy link

win.setBrowserView(view)
await view.webContents.loadURL('https://electronjs.org')
view.setBounds({ x: 0, y: 0, width: 800, height: 600 })

I tried it better this way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: BrowserViews do not load their webContents
6 participants