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: exit HTML fullscreen when window leaves fullscreen #13090

Merged
merged 1 commit into from Jun 18, 2018

Conversation

Projects
None yet
4 participants
@miniak
Contributor

miniak commented May 27, 2018

Fixes #12653

@miniak miniak requested a review from electron/reviewers as a code owner May 27, 2018

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 27, 2018

Contributor

@zcbenz any idea how to do this in the C++ code directly?

Contributor

miniak commented May 27, 2018

@zcbenz any idea how to do this in the C++ code directly?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 May 28, 2018

Member

@miniak content layer checks for the fullscreen state of the page with WebContentsDelegate::IsFullscreenForTabOrPending and we have been returning the wrong state https://github.com/electron/electron/blob/master/atom/browser/common_web_contents_delegate.cc#L273, also this bug is present since the original implementation. The fix here would be to return the fullscreen state of the owner window using owner_window_->IsFullscreen()

Member

deepak1556 commented May 28, 2018

@miniak content layer checks for the fullscreen state of the page with WebContentsDelegate::IsFullscreenForTabOrPending and we have been returning the wrong state https://github.com/electron/electron/blob/master/atom/browser/common_web_contents_delegate.cc#L273, also this bug is present since the original implementation. The fix here would be to return the fullscreen state of the owner window using owner_window_->IsFullscreen()

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 28, 2018

Contributor

@deepak1556 I don't think that's correct, being in window fullscreen mode does not imply HTML fullscreen, however HTML fullscreen implies window fullscreen.

  // Returns whether the active tab is in html fullscreen mode.
  bool IsActiveTabFullscreen() const {
    auto* contents = GetActiveWebContents();
    return contents->GetDelegate()->IsFullscreenForTabOrPending(contents);
  }
Contributor

miniak commented May 28, 2018

@deepak1556 I don't think that's correct, being in window fullscreen mode does not imply HTML fullscreen, however HTML fullscreen implies window fullscreen.

  // Returns whether the active tab is in html fullscreen mode.
  bool IsActiveTabFullscreen() const {
    auto* contents = GetActiveWebContents();
    return contents->GetDelegate()->IsFullscreenForTabOrPending(contents);
  }
@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz May 28, 2018

Contributor

@zcbenz any idea how to do this in the C++ code directly?

You can add an override in atom_api_browser_window.cc:

BrowserWindow::OnWindowLeaveFullScreen() {
  TopLevelWindow::OnWindowLeaveFullScreen();
  web_contents()->...
}

But WebContents does not have an API to execute script directly, and webContents.executeJavaScript is implemented in JavaScript, so you have to send IPC messages to implement the same behavior.

Contributor

zcbenz commented May 28, 2018

@zcbenz any idea how to do this in the C++ code directly?

You can add an override in atom_api_browser_window.cc:

BrowserWindow::OnWindowLeaveFullScreen() {
  TopLevelWindow::OnWindowLeaveFullScreen();
  web_contents()->...
}

But WebContents does not have an API to execute script directly, and webContents.executeJavaScript is implemented in JavaScript, so you have to send IPC messages to implement the same behavior.

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 28, 2018

Contributor

@zcbenz by implementing in C++ I mean more like doing it without executeJavaScript :) I should probably look into Chromium code how they handle it in the browser code part.

Contributor

miniak commented May 28, 2018

@zcbenz by implementing in C++ I mean more like doing it without executeJavaScript :) I should probably look into Chromium code how they handle it in the browser code part.

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 May 28, 2018

Member

being in window fullscreen mode does not imply HTML fullscreen, however HTML fullscreen implies window fullscreen.

Yes agree, sorry for the confusion I meant we need to fix the value of html_fullscreen_ based on the when the window exits fullscreen mode while in html fullscreen, so that the content layer can read the right value and update the UI. We don't have to inject any script to achieve this.

Member

deepak1556 commented May 28, 2018

being in window fullscreen mode does not imply HTML fullscreen, however HTML fullscreen implies window fullscreen.

Yes agree, sorry for the confusion I meant we need to fix the value of html_fullscreen_ based on the when the window exits fullscreen mode while in html fullscreen, so that the content layer can read the right value and update the UI. We don't have to inject any script to achieve this.

@codebytere codebytere changed the title from Exit HTML fullscreen when window leaves fullscreen to fix: exit HTML fullscreen when window leaves fullscreen May 29, 2018

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon May 29, 2018

Contributor

You should be able to call WebContents::ExitFullscreen to exit fullscreen from the browser side—it'll send the appropriate IPCs to the renderer to cause it to exit fullscreen mode (and also pointer lock, etc.). I'm not sure what the best way is to traverse all the webviews in the tree though—perhaps WebContents::ForEachFrame?

Contributor

nornagon commented May 29, 2018

You should be able to call WebContents::ExitFullscreen to exit fullscreen from the browser side—it'll send the appropriate IPCs to the renderer to cause it to exit fullscreen mode (and also pointer lock, etc.). I'm not sure what the best way is to traverse all the webviews in the tree though—perhaps WebContents::ForEachFrame?

@zcbenz

zcbenz approved these changes Jun 18, 2018

@zcbenz zcbenz merged commit 2eb5b75 into master Jun 18, 2018

11 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@zcbenz zcbenz deleted the miniak/fix-exit-html-fullscreen branch Jun 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment