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: webview should maximize on requestFullscreen #29952

Merged
merged 1 commit into from Jul 2, 2021

Conversation

zcbenz
Copy link
Member

@zcbenz zcbenz commented Jun 30, 2021

Description of Change

Close #27719.

When entering fullscreen with Element.requestFullscreen in child frames, the parent frame should also enter fullscreen mode too. Chromium handles this for iframes, but not for webviews, as the latter are essentially main frames instead of child frames.

This PR makes sure webviews propagate the fullscreen state to embedder.

Note that the patch itself in this PR is not enough to fix the issue, to fix it we have to make the fullscreen state sync between the webview and its embedder frame, and in Electron we are just manually synchronizing the states. A decent patch should change callers or callees of IsFullscreenForTabOrPending to be aware of outer frames (webviews), but it would require much more refactoring. I think it does not worth such efforts upstreaming the patch to Chromium, because very few people would encounter this problem in Chrome browser and the patch in this PR is very easy to maintain.

Release Notes

Notes: Fix requestFullscreen inside webview does not make the element take fullscreen.

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/12-x-y labels Jun 30, 2021
@zcbenz zcbenz requested a review from a team as a code owner June 30, 2021 05:52
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 30, 2021
@zcbenz zcbenz requested a review from miniak June 30, 2021 05:53

Note that we also need to manually update embedder's
`api::WebContents::IsFullscreenForTabOrPending` value.

Copy link
Member

Choose a reason for hiding this comment

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

Is this something we can potentially upstream?

Copy link
Member Author

Choose a reason for hiding this comment

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

The patch itself is not enough to fix the issue, so we can not upstream it directly.

To fix the problem we also have to make the fullscreen state sync between the webview and its embedder frame, and in Electron we are just manually synchronizing the states. A decent patch should change callers or callees of IsFullscreenForTabOrPending to be aware of outer frames (webviews), but it would require much more refactoring.

So I think it does not worth such efforts upstreaming the changes to Chromium, as very few people would encounter this problem in Chrome browser and the patch in this PR is very easy to maintain.

Copy link
Contributor

@miniak miniak left a comment

Choose a reason for hiding this comment

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

There is still one issue, when you exit window fullscreen by clicking the green semaphore button on Mac, the webview's webContents does not exit HTML fullscreen mode. It works propertly for iframes.

@miniak
Copy link
Contributor

miniak commented Jun 30, 2021

I still need this workaround to make it work fully

app.on('web-contents-created', (event, webContents) => {
  webContents.on('enter-html-full-screen', () => {
    if (webContents.getType() === 'webview') {
      const window = webContents.getOwnerBrowserWindow();
      window.once('leave-html-full-screen', () => {
        webContents.executeJavaScript('document.exitFullscreen()', true);
      });
    }
  });
});

@zcbenz zcbenz force-pushed the fix-webview-fullscreen branch 4 times, most recently from cac2915 to 241117c Compare July 1, 2021 05:37
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 1, 2021
@zcbenz
Copy link
Member Author

zcbenz commented Jul 1, 2021

@miniak The mentioned problem should have been fixed, can you test again?

Copy link
Contributor

@miniak miniak left a comment

Choose a reason for hiding this comment

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

it works now!

@jkleinsc jkleinsc merged commit 6eff923 into main Jul 2, 2021
@jkleinsc jkleinsc deleted the fix-webview-fullscreen branch July 2, 2021 00:56
@release-clerk
Copy link

release-clerk bot commented Jul 2, 2021

Release Notes Persisted

Fix requestFullscreen inside webview does not make the element take fullscreen.

@trop
Copy link
Contributor

trop bot commented Jul 2, 2021

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

@trop trop bot removed the target/13-x-y label Jul 2, 2021
@trop
Copy link
Contributor

trop bot commented Jul 2, 2021

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

@trop trop bot added the in-flight/13-x-y label Jul 2, 2021
@trop
Copy link
Contributor

trop bot commented Jul 2, 2021

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

@TranceGeniK
Copy link

Is there a way to prevent that behavior ?
It may be preferable in some cases to have the fullscreen limited to the webview container.

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.

iframe inside a webview doesn't maximize on requestFullScreen
6 participants