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.

Sign up for GitHub

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

Jump to bottom

Require that the document is fully active in requestFullscreen() #85

Merged
merged 1 commit into from
May 15, 2017
Merged

Require that the document is fully active in requestFullscreen() #85

merged 1 commit into from
May 15, 2017

Conversation

foolip
Copy link
Member

@foolip foolip commented May 12, 2017 •
edited by pr-preview bot
Loading

@foolip
Copy link
Member Author

foolip commented May 12, 2017

This is similar to the check in exitFullscreen(), and Blink already has a check like this, which is why I'd like to get it into the spec. Firefox actually fails the test in web-platform-tests/wpt#5901 because it fires a fullscreenerror event, a behavior I think would be hard to implement in Blink (trying to fire an event in an inactive document does nothing last I checked) and isn't super useful.

@foolip foolip mentioned this pull request May 12, 2017
Merged
annevk
annevk requested changes May 12, 2017
View reviewed changes
fullscreen.bs

<li><p>Let <var>promise</var> be a new promise.

<li><p>If <var>pendingDoc</var> is not <a>fully active</a>, then reject <var>promise</var> with a
<code>TypeError</code> exception and return <var>promise</var>.
Copy link
Member

Choose a reason for hiding this comment

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

You cannot return here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? This is before the "in parallel" steps.

Copy link
Member

Choose a reason for hiding this comment

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

I wrongly assumed that we need to run those either way.

annevk
annevk approved these changes May 12, 2017
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Provided @upsuper agrees.

upsuper
upsuper approved these changes May 14, 2017
View reviewed changes
Copy link
Member

@upsuper upsuper left a comment

Choose a reason for hiding this comment

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

LGTM

@foolip foolip merged commit d1d1b4a into master May 15, 2017
@foolip foolip deleted the request-fully-active branch May 15, 2017 09:34
foolip added a commit to web-platform-tests/wpt that referenced this pull request May 15, 2017
@foolip
Test the "fully active" check for requestFullscreen()
a453a1c 
Test for whatwg/fullscreen#85
@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

@upsuper upsuper upsuper approved these changes

@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.

None yet
3 participants
@foolip @upsuper @annevk