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: html fullscreen transitions stacking #34908

Merged
merged 1 commit into from Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions shell/browser/api/electron_api_web_contents.cc
Expand Up @@ -3524,7 +3524,12 @@ void WebContents::EnumerateDirectory(

bool WebContents::IsFullscreenForTabOrPending(
const content::WebContents* source) {
return html_fullscreen_;
bool transition_fs = owner_window()
? owner_window()->fullscreen_transition_state() !=
NativeWindow::FullScreenTransitionState::NONE
: false;

return html_fullscreen_ || transition_fs;
}

bool WebContents::TakeFocus(content::WebContents* source, bool reverse) {
Expand Down Expand Up @@ -3846,9 +3851,8 @@ void WebContents::SetHtmlApiFullscreen(bool enter_fullscreen) {
? !web_preferences->ShouldDisableHtmlFullscreenWindowResize()
: true;

if (html_fullscreenable) {
if (html_fullscreenable)
owner_window_->SetFullScreen(enter_fullscreen);
}

UpdateHtmlApiFullscreen(enter_fullscreen);
native_fullscreen_ = false;
Expand Down
9 changes: 9 additions & 0 deletions shell/browser/native_window.cc
Expand Up @@ -718,6 +718,15 @@ std::string NativeWindow::GetAccessibleTitle() {
return base::UTF16ToUTF8(accessible_title_);
}

void NativeWindow::HandlePendingFullscreenTransitions() {
if (pending_transitions_.empty())
return;

bool next_transition = pending_transitions_.front();
pending_transitions_.pop();
SetFullScreen(next_transition);
}

// static
int32_t NativeWindow::next_id_ = 0;

Expand Down
16 changes: 16 additions & 0 deletions shell/browser/native_window.h
Expand Up @@ -7,6 +7,7 @@

#include <list>
#include <memory>
#include <queue>
#include <string>
#include <vector>

Expand Down Expand Up @@ -317,6 +318,17 @@ class NativeWindow : public base::SupportsUserData,
observers_.RemoveObserver(obs);
}

enum class FullScreenTransitionState { ENTERING, EXITING, NONE };

// Handle fullscreen transitions.
void HandlePendingFullscreenTransitions();
void set_fullscreen_transition_state(FullScreenTransitionState state) {
fullscreen_transition_state_ = state;
}
FullScreenTransitionState fullscreen_transition_state() const {
return fullscreen_transition_state_;
}

views::Widget* widget() const { return widget_.get(); }
views::View* content_view() const { return content_view_; }

Expand Down Expand Up @@ -375,6 +387,10 @@ class NativeWindow : public base::SupportsUserData,
// The "titleBarStyle" option.
TitleBarStyle title_bar_style_ = TitleBarStyle::kNormal;

std::queue<bool> pending_transitions_;
FullScreenTransitionState fullscreen_transition_state_ =
FullScreenTransitionState::NONE;

private:
std::unique_ptr<views::Widget> widget_;

Expand Down
13 changes: 0 additions & 13 deletions shell/browser/native_window_mac.h
Expand Up @@ -8,7 +8,6 @@
#import <Cocoa/Cocoa.h>

#include <memory>
#include <queue>
#include <string>
#include <vector>

Expand Down Expand Up @@ -174,11 +173,6 @@ class NativeWindowMac : public NativeWindow,
void SetCollectionBehavior(bool on, NSUInteger flag);
void SetWindowLevel(int level);

enum class FullScreenTransitionState { ENTERING, EXITING, NONE };

// Handle fullscreen transitions.
void SetFullScreenTransitionState(FullScreenTransitionState state);
void HandlePendingFullscreenTransitions();
bool HandleDeferredClose();
void SetHasDeferredWindowClose(bool defer_close) {
has_deferred_window_close_ = defer_close;
Expand Down Expand Up @@ -249,13 +243,6 @@ class NativeWindowMac : public NativeWindow,
bool zoom_to_page_width_ = false;
absl::optional<gfx::Point> traffic_light_position_;

std::queue<bool> pending_transitions_;
FullScreenTransitionState fullscreen_transition_state() const {
return fullscreen_transition_state_;
}
FullScreenTransitionState fullscreen_transition_state_ =
FullScreenTransitionState::NONE;

// Trying to close an NSWindow during a fullscreen transition will cause the
// window to lock up. Use this to track if CloseWindow was called during a
// fullscreen transition, to defer the -[NSWindow close] call until the
Expand Down
14 changes: 0 additions & 14 deletions shell/browser/native_window_mac.mm
Expand Up @@ -586,11 +586,6 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) {
return [window_ isVisible] && !occluded && !IsMinimized();
}

void NativeWindowMac::SetFullScreenTransitionState(
FullScreenTransitionState state) {
fullscreen_transition_state_ = state;
}

bool NativeWindowMac::IsEnabled() {
return [window_ attachedSheet] == nil;
}
Expand Down Expand Up @@ -674,15 +669,6 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) {
return [window_ isMiniaturized];
}

void NativeWindowMac::HandlePendingFullscreenTransitions() {
if (pending_transitions_.empty())
return;

bool next_transition = pending_transitions_.front();
pending_transitions_.pop();
SetFullScreen(next_transition);
}

bool NativeWindowMac::HandleDeferredClose() {
if (has_deferred_window_close_) {
SetHasDeferredWindowClose(false);
Expand Down
8 changes: 4 additions & 4 deletions shell/browser/ui/cocoa/electron_ns_window_delegate.mm
Expand Up @@ -238,7 +238,7 @@ - (void)windowWillEnterFullScreen:(NSNotification*)notification {
// Store resizable mask so it can be restored after exiting fullscreen.
is_resizable_ = shell_->HasStyleMask(NSWindowStyleMaskResizable);

shell_->SetFullScreenTransitionState(FullScreenTransitionState::ENTERING);
shell_->set_fullscreen_transition_state(FullScreenTransitionState::ENTERING);

shell_->NotifyWindowWillEnterFullScreen();

Expand All @@ -247,7 +247,7 @@ - (void)windowWillEnterFullScreen:(NSNotification*)notification {
}

- (void)windowDidEnterFullScreen:(NSNotification*)notification {
shell_->SetFullScreenTransitionState(FullScreenTransitionState::NONE);
shell_->set_fullscreen_transition_state(FullScreenTransitionState::NONE);

shell_->NotifyWindowEnterFullScreen();

Expand All @@ -258,13 +258,13 @@ - (void)windowDidEnterFullScreen:(NSNotification*)notification {
}

- (void)windowWillExitFullScreen:(NSNotification*)notification {
shell_->SetFullScreenTransitionState(FullScreenTransitionState::EXITING);
shell_->set_fullscreen_transition_state(FullScreenTransitionState::EXITING);

shell_->NotifyWindowWillLeaveFullScreen();
}

- (void)windowDidExitFullScreen:(NSNotification*)notification {
shell_->SetFullScreenTransitionState(FullScreenTransitionState::NONE);
shell_->set_fullscreen_transition_state(FullScreenTransitionState::NONE);

shell_->SetResizable(is_resizable_);
shell_->NotifyWindowLeaveFullScreen();
Expand Down
67 changes: 64 additions & 3 deletions spec-main/api-browser-window-spec.ts
Expand Up @@ -4973,19 +4973,80 @@ describe('BrowserWindow module', () => {
expect(w.isFullScreen()).to.be.false('is fullscreen');
});

it('handles several HTML fullscreen transitions', async () => {
const w = new BrowserWindow();
await w.loadFile(path.join(fixtures, 'pages', 'a.html'));

expect(w.isFullScreen()).to.be.false('is fullscreen');

const enterFullScreen = emittedOnce(w, 'enter-full-screen');
const leaveFullScreen = emittedOnce(w, 'leave-full-screen');

await w.webContents.executeJavaScript('document.getElementById("div").requestFullscreen()', true);
await enterFullScreen;
await w.webContents.executeJavaScript('document.exitFullscreen()', true);
await leaveFullScreen;

expect(w.isFullScreen()).to.be.false('is fullscreen');

await delay();

await w.webContents.executeJavaScript('document.getElementById("div").requestFullscreen()', true);
await enterFullScreen;
await w.webContents.executeJavaScript('document.exitFullscreen()', true);
await leaveFullScreen;

expect(w.isFullScreen()).to.be.false('is fullscreen');
});

it('handles several transitions in close proximity', async () => {
const w = new BrowserWindow();

expect(w.isFullScreen()).to.be.false('is fullscreen');

const enterFS = emittedNTimes(w, 'enter-full-screen', 2);
const leaveFS = emittedNTimes(w, 'leave-full-screen', 2);

w.setFullScreen(true);
w.setFullScreen(false);
w.setFullScreen(true);
w.setFullScreen(false);

const enterFullScreen = emittedNTimes(w, 'enter-full-screen', 2);
await enterFullScreen;
await Promise.all([enterFS, leaveFS]);

expect(w.isFullScreen()).to.be.true('not fullscreen');
expect(w.isFullScreen()).to.be.false('not fullscreen');
});

it('handles several chromium-initiated transitions in close proximity', async () => {
const w = new BrowserWindow();
await w.loadFile(path.join(fixtures, 'pages', 'a.html'));

expect(w.isFullScreen()).to.be.false('is fullscreen');

let enterCount = 0;
let exitCount = 0;

const done = new Promise<void>(resolve => {
const checkDone = () => {
if (enterCount === 2 && exitCount === 2) resolve();
};

w.webContents.on('enter-html-full-screen', () => {
enterCount++;
checkDone();
});

w.webContents.on('leave-html-full-screen', () => {
exitCount++;
checkDone();
});
});

await w.webContents.executeJavaScript('document.getElementById("div").requestFullscreen()', true);
await w.webContents.executeJavaScript('document.exitFullscreen()');
await w.webContents.executeJavaScript('document.getElementById("div").requestFullscreen()', true);
await w.webContents.executeJavaScript('document.exitFullscreen()');
await done;
});

it('does not crash when exiting simpleFullScreen (properties)', async () => {
Expand Down
33 changes: 31 additions & 2 deletions spec-main/chromium-spec.ts
Expand Up @@ -10,7 +10,7 @@ import * as url from 'url';
import * as ChildProcess from 'child_process';
import { EventEmitter } from 'events';
import { promisify } from 'util';
import { ifit, ifdescribe, delay, defer } from './spec-helpers';
import { ifit, ifdescribe, defer, delay } from './spec-helpers';
import { AddressInfo } from 'net';
import { PipeTransport } from './pipe-transport';

Expand Down Expand Up @@ -1653,7 +1653,7 @@ describe('iframe using HTML fullscreen API while window is OS-fullscreened', ()
server.close();
});

it('can fullscreen from out-of-process iframes (OOPIFs)', async () => {
ifit(process.platform !== 'darwin')('can fullscreen from out-of-process iframes (non-macOS)', async () => {
const fullscreenChange = emittedOnce(ipcMain, 'fullscreenChange');
const html =
'<iframe style="width: 0" frameborder=0 src="http://localhost:8989" allowfullscreen></iframe>';
Expand All @@ -1677,8 +1677,37 @@ describe('iframe using HTML fullscreen API while window is OS-fullscreened', ()
expect(width).to.equal(0);
});

ifit(process.platform === 'darwin')('can fullscreen from out-of-process iframes (macOS)', async () => {
await emittedOnce(w, 'enter-full-screen');
const fullscreenChange = emittedOnce(ipcMain, 'fullscreenChange');
const html =
'<iframe style="width: 0" frameborder=0 src="http://localhost:8989" allowfullscreen></iframe>';
w.loadURL(`data:text/html,${html}`);
await fullscreenChange;

const fullscreenWidth = await w.webContents.executeJavaScript(
"document.querySelector('iframe').offsetWidth"
);
expect(fullscreenWidth > 0).to.be.true();

await w.webContents.executeJavaScript(
"document.querySelector('iframe').contentWindow.postMessage('exitFullscreen', '*')"
);
await emittedOnce(w.webContents, 'leave-html-full-screen');

const width = await w.webContents.executeJavaScript(
"document.querySelector('iframe').offsetWidth"
);
expect(width).to.equal(0);
Copy link
Member

Choose a reason for hiding this comment

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

This assertion is failing on macOS

not ok 1119 iframe using HTML fullscreen API while window is OS-fullscreened can fullscreen from out-of-process iframes (macOS)
  AssertionError: expected 1024 to equal 0
      at Context.<anonymous> (electron/spec-main/chromium-spec.ts:1701:22)


w.setFullScreen(false);
await emittedOnce(w, 'leave-full-screen');
});

// TODO(jkleinsc) fix this flaky test on WOA
ifit(process.platform !== 'win32' || process.arch !== 'arm64')('can fullscreen from in-process iframes', async () => {
if (process.platform === 'darwin') await emittedOnce(w, 'enter-full-screen');
Copy link
Member

Choose a reason for hiding this comment

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

And this one times out

not ok 1120 iframe using HTML fullscreen API while window is OS-fullscreened can fullscreen from in-process iframes
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/distiller/project/src/electron/spec-main/chromium-spec.ts)
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, these appear to be flakes as they show up on other PRs too. I've rerun.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, there are some more unrelated errors now but the errors I shared here are still present. Do you have a link to an old CI run in the 19-x-y line that has the same error?


const fullscreenChange = emittedOnce(ipcMain, 'fullscreenChange');
w.loadFile(path.join(fixturesPath, 'pages', 'fullscreen-ipif.html'));
await fullscreenChange;
Expand Down
1 change: 1 addition & 0 deletions spec-main/fixtures/webview/fullscreen/frame.html
Expand Up @@ -2,6 +2,7 @@
<div id="div">
WebView
</div>
<video></video>
<script type="text/javascript" charset="utf-8">
const {ipcRenderer} = require('electron')
ipcRenderer.send('webview-ready')
Expand Down