Skip to content

In requestFullscreen(), handle the pending element moving#87

Merged
foolip merged 1 commit into
masterfrom
pending-element-moved
May 16, 2017
Merged

In requestFullscreen(), handle the pending element moving#87
foolip merged 1 commit into
masterfrom
pending-element-moved

Conversation

@foolip

@foolip foolip commented May 16, 2017 •
edited by pr-preview Bot
Loading

Copy link
Copy Markdown
Member

Fixes #33.

Drive-by: capitalization in lt attributes to match dfn.

Preview | Diff

@foolip
In requestFullscreen(), handle the pending element moving
e96e446 
Fixes #33.

Drive-by: capitalization in lt attributes to match dfn.
@foolip

foolip commented May 16, 2017

Copy link
Copy Markdown
Member Author

A test that already tests this was added in web-platform-tests/wpt#4250:
http://w3c-test.org/fullscreen/api/element-request-fullscreen-and-move-to-iframe-manual.html

Firefox Nightly already passes this test.

It was an oversight that this test was included before the spec change, I've gone through the PR and checked for other things not yet reflected in the spec, but can't spot anything.

@foolip foolip merged commit cbdc4f7 into master May 16, 2017
@foolip foolip deleted the pending-element-moved branch May 16, 2017 10:37
@foolip

foolip commented May 16, 2017

Copy link
Copy Markdown
Member Author

Merged, but can you confirm if this will work for Gecko, @upsuper? Maybe the tests doesn't exercise some minor difference between the spec and what Gecko does.

@upsuper

upsuper commented May 16, 2017

Copy link
Copy Markdown
Member

There is an issue here that, it seems when this happens, the browser window would end up being in fullscreen state while the content is not? It doesn't sound like a helpful state, but this is probably a separate issue.

Anyway, I think it works for Gecko, and I think except having the issue above, Gecko should have been doing something like this now, but I'm not completely sure.

@foolip foolip mentioned this pull request May 16, 2017
Open
@foolip

foolip commented May 16, 2017

Copy link
Copy Markdown
Member Author

Yeah, that is a problem, and IIRC you have a fix for it in Gecko? I've filed #88 but probably won't get back to it myself anytime soon since it's such a corner case.

@upsuper

upsuper commented May 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Gecko doesn't have this problem. The algorithm is something like, when we are trying to apply fullscreen state to pending document from window change, if there is nothing successfully gets fullscreened eventually, we revert the window state. I don't know if something like this is specable...

@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Jun 20, 2017
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 20, 2017
@foolip @chromium-wpt-export-bot
Reland "Sync requestFullscreen() and exitFullscreen() algorithms with…
8061570 
… the spec"

This relands https://codereview.chromium.org/2573773002/, which was
reverted in https://codereview.chromium.org/2654083006/.

Relevant spec changes since December 2016:
 * whatwg/fullscreen#68
 * whatwg/fullscreen#72
 * whatwg/fullscreen#85
 * whatwg/fullscreen#87
 * whatwg/fullscreen#90
 * whatwg/fullscreen#92

Updated tests for whatwg/fullscreen#92:

 * document-exit-fullscreen-nested-manual.html asserts that fullscreenElement
   changes are not synchronous.

 * document-exit-fullscreen-timing-manual.html and
   element-request-fullscreen-timing-manual.html assert that fullscreenElement
   changes before the resize event. (They still fail in content_shell because
   there is no resize, but pass in chromium if run manually.)

 * element-request-fullscreen-and-exit-iframe-manual.html asserts that the order
   of "run the fullscreen steps" (firing events) is now frame tree order.

Bug: 402376
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I9c01b237ecfd7d74b28e3dbafcacdefe43416cdf
Reviewed-on: https://chromium-review.googlesource.com/521162
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: John Mellor <johnme@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480702}
WPT-Export-Revision: 26d298d134b1407ffb035481dea00e95e66a4de9
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 20, 2017
@foolip @chromium-wpt-export-bot
Reland "Sync requestFullscreen() and exitFullscreen() algorithms with…
9c9f267 
… the spec"

This relands https://codereview.chromium.org/2573773002/, which was
reverted in https://codereview.chromium.org/2654083006/.

Relevant spec changes since December 2016:
 * whatwg/fullscreen#68
 * whatwg/fullscreen#72
 * whatwg/fullscreen#85
 * whatwg/fullscreen#87
 * whatwg/fullscreen#90
 * whatwg/fullscreen#92

Updated tests for whatwg/fullscreen#92:

 * document-exit-fullscreen-nested-manual.html asserts that fullscreenElement
   changes are not synchronous.

 * document-exit-fullscreen-timing-manual.html and
   element-request-fullscreen-timing-manual.html assert that fullscreenElement
   changes before the resize event. (They still fail in content_shell because
   there is no resize, but pass in chromium if run manually.)

 * element-request-fullscreen-and-exit-iframe-manual.html asserts that the order
   of "run the fullscreen steps" (firing events) is now frame tree order.

Bug: 402376
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I9c01b237ecfd7d74b28e3dbafcacdefe43416cdf
Reviewed-on: https://chromium-review.googlesource.com/521162
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: John Mellor <johnme@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480702}
WPT-Export-Revision: 26d298d134b1407ffb035481dea00e95e66a4de9
MXEBot pushed a commit to mirror/chromium that referenced this pull request Jun 21, 2017
@foolip
Reland "Sync requestFullscreen() and exitFullscreen() algorithms with…
81e33d3 
… the spec"

This relands https://codereview.chromium.org/2573773002/, which was
reverted in https://codereview.chromium.org/2654083006/.

Relevant spec changes since December 2016:
 * whatwg/fullscreen#68
 * whatwg/fullscreen#72
 * whatwg/fullscreen#85
 * whatwg/fullscreen#87
 * whatwg/fullscreen#90
 * whatwg/fullscreen#92

Updated tests for whatwg/fullscreen#92:

 * document-exit-fullscreen-nested-manual.html asserts that fullscreenElement
   changes are not synchronous.

 * document-exit-fullscreen-timing-manual.html and
   element-request-fullscreen-timing-manual.html assert that fullscreenElement
   changes before the resize event. (They still fail in content_shell because
   there is no resize, but pass in chromium if run manually.)

 * element-request-fullscreen-and-exit-iframe-manual.html asserts that the order
   of "run the fullscreen steps" (firing events) is now frame tree order.

Bug: 402376
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I9c01b237ecfd7d74b28e3dbafcacdefe43416cdf
Reviewed-on: https://chromium-review.googlesource.com/521162
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: John Mellor <johnme@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480862}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@annevk annevk annevk approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@foolip @upsuper @annevk