Skip to content

Reject exitFullscreen() in inactive documents#68

Merged
annevk merged 2 commits into
masterfrom
inactive-document
Dec 16, 2016
Merged

Reject exitFullscreen() in inactive documents#68
annevk merged 2 commits into
masterfrom
inactive-document

Conversation

@foolip

@foolip foolip commented Dec 15, 2016

Copy link
Copy Markdown
Member

If the document becomes inactive after the request, then there will not
be another animation frame task, in which case the promise will be left
hanging instead.

Fixes #67.

zcorpan
zcorpan reviewed Dec 15, 2016
View reviewed changes
Comment thread fullscreen.bs Outdated
<li><p>Let <var>promise</var> be a new promise.

<li><p>If <var>doc</var> has no <a for=Document>browsing context</a>, or if <var>doc</var>'s
<a for=Document>browsing context</a>'s <a>active document</a> is not <var>doc</var>, then

@zcorpan zcorpan Dec 15, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this needs to be if <var>doc</var> is not <a>fully active</a>, since it might be the active document but an ancestor browsing context's document is not active.

@foolip foolip Dec 15, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Excellent, didn't know that concept existed.

@foolip foolip requested a review from zcorpan December 15, 2016 11:45
@annevk

annevk commented Dec 16, 2016

Copy link
Copy Markdown
Member

Do we have tests for this too?

annevk
annevk reviewed Dec 16, 2016
View reviewed changes
Comment thread fullscreen.bs Outdated
<ol>
<li><p>Let <var>promise</var> be a new promise.

<li><p>If <var>doc</var> is not <a>fully active</a>, reject <var>promise</var> with a

@annevk annevk Dec 16, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

then reject* (I realize it's not consistent, but I'd like new and changed text to conform to it at least.)

annevk
annevk reviewed Dec 16, 2016
View reviewed changes
Comment thread fullscreen.bs Outdated
<li><p>Let <var>promise</var> be a new promise.

<li><p>If <var>doc</var> is not <a>fully active</a>, reject <var>promise</var> with a
<code>TypeError</code> exception, and return <var>promise</var>.

@annevk annevk Dec 16, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No comma before "and". Hmm, maybe we should fix the next line as well.

@foolip
Reject exitFullscreen() in inactive documents
64f2487 
If the document becomes inactive after the request, then there will not
be another animation frame task, in which case the promise will be left
hanging instead.

Fixes #67.
@foolip

foolip commented Dec 16, 2016

Copy link
Copy Markdown
Member Author

Style possibly fixed. Then I can learn, but I haven't been able to internalize the rules for commas, so we'll keep having fun with that :)

@foolip

foolip commented Dec 16, 2016

Copy link
Copy Markdown
Member Author

I'll now write a test, which will be the first test fullscreen test involving a promise that will fail everywhere.

foolip added a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2016
@foolip
Test document.exitFullscreen() for an inactive document
95d4354 
Test for whatwg/fullscreen#68
foolip added a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2016
@foolip
Test document.exitFullscreen() for an inactive document
fa463bb 
Test for whatwg/fullscreen#68
@foolip foolip mentioned this pull request Dec 16, 2016
Merged
foolip added a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2016
@foolip
Test document.exitFullscreen() for an inactive document
19e0cdc 
Test for whatwg/fullscreen#68
@foolip

foolip commented Dec 16, 2016

Copy link
Copy Markdown
Member Author

Test is reviewed. @annevk, can you merge both together if all LGTY?

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2016
@foolip @annevk
Test document.exitFullscreen() for an inactive document
c9f1765 
Test for whatwg/fullscreen#68
@annevk annevk merged commit 59574a4 into master Dec 16, 2016
@annevk annevk deleted the inactive-document branch December 16, 2016 13:39
@foolip

foolip commented Dec 19, 2016

Copy link
Copy Markdown
Member Author

@annevk, it looks like https://fullscreen.spec.whatwg.org/#exit-fullscreen hasn't been updated, are the auto-publish scripts misbehaving?

@annevk

annevk commented Dec 19, 2016

Copy link
Copy Markdown
Member

Should be good now.

@foolip

foolip commented Dec 19, 2016

Copy link
Copy Markdown
Member Author

Yep, thanks!

@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 left review comments

@zcorpan zcorpan zcorpan 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 @annevk @zcorpan