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

Fullscreen/unfullscreen ASAP; fire events at animation frame timing #92

Merged
merged 1 commit into from
Jun 15, 2017
Merged

Fullscreen/unfullscreen ASAP; fire events at animation frame timing #92

merged 1 commit into from
Jun 15, 2017

Conversation

foolip
Copy link
Member

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

This means that changes to document.fullscreenElement and other state
will be observable as soon as the resize itself is (e.g. via
window.innerWidth) and before resize or scroll events are fired.

The fullscreenchange event is still delayed to animation frame timing.

This also includes a slight change when /resize/ is true in "exit
fullscreen". By changing /doc/ to /topLevelDoc/ in this case, we can
make sure that we always fully unfullscreen all documents in this case.
This makes a "fully exit fullscreen" corner case unnecessary.

Fixes #74.

Preview | Diff

@foolip foolip mentioned this pull request May 31, 2017
Closed
@foolip
Copy link
Member Author

foolip commented May 31, 2017

This depends on whatwg/html#2627. @upsuper, would you mind reviewing both? I'm fine with not making animation frame tasks a generic concept if you think that's better, it wouldn't affect the important changes here.

@foolip foolip mentioned this pull request Jun 1, 2017
Closed
@foolip
Copy link
Member Author

foolip commented Jun 7, 2017

Ping @upsuper.

@upsuper
Copy link
Member

upsuper commented Jun 7, 2017

Sorry for the delay. Please give me another day. I'll try to review it tomorrow.

@upsuper
Copy link
Member

upsuper commented Jun 7, 2017

And yeah, I'd probably prefer we don't make animation frame tasks a generic concept. Having each thing in its own queue makes order of them clearer, and would also allow us to specify the order if we need / want, at least for this kind of things which can potentially affect rendering.

@foolip
Copy link
Member Author

foolip commented Jun 14, 2017

Thanks for the review, @upsuper. I've now pushed an update that defines the "run the fullscreen rendering steps" already references by HTML, which means this doesn't depend on any HTML changes.

The implementation I hope to land in Blink is more similar to the "animation frame tasks" model I had, but currently it is indistinguishable. I might want to revisit whatwg/html#2627 later, but it's not important today.

Care to take another look? If it's OK with you, I plan to write the tests for this in https://chromium-review.googlesource.com/521162, so you wouldn't need to do that review.

@foolip
Fullscreen/unfullscreen ASAP; fire events at animation frame timing
a62b790 
This means that changes to document.fullscreenElement and other state
will be observable as soon as the resize itself is (e.g. via
window.innerWidth) and before resize or scroll events are fired.

The fullscreenchange event is still delayed to animation frame timing.

This also includes a slight change when /resize/ is true in "exit
fullscreen". By changing /doc/ to /topLevelDoc/ in this case, we can
make sure that we always fully unfullscreen all documents in this case.
This makes a "fully exit fullscreen" corner case unnecessary.

Fixes #74.
@foolip foolip changed the title Fullscreen/unfullscreen ASAP; fire events in animation frame tasks git showFullscreen/unfullscreen ASAP; fire events in animation frame tasks Jun 15, 2017
@foolip foolip changed the title git showFullscreen/unfullscreen ASAP; fire events in animation frame tasks Fullscreen/unfullscreen ASAP; fire events at animation frame timing Jun 15, 2017
@foolip foolip merged commit 4208e5a into master Jun 15, 2017
@foolip foolip deleted the fullscreen-asap branch June 15, 2017 08:49
@foolip
Copy link
Member Author

foolip commented Jun 15, 2017

To repeat, tests will be added in https://chromium-review.googlesource.com/521162

foolip added a commit to whatwg/html that referenced this pull request Jun 15, 2017
@foolip
Link to "run the fullscreen rendering steps" in Fullscreen
c503df8 
Introduced in whatwg/fullscreen#92.

Drive-by: re-wrap lines to 100 columns.
@foolip foolip mentioned this pull request Jun 15, 2017
Merged
foolip added a commit to whatwg/html that referenced this pull request Jun 15, 2017
@foolip
Link to "run the fullscreen rendering steps" in Fullscreen
ca65ca4 
Introduced in whatwg/fullscreen#92.

Fixes the fullscreen part of #707.

Drive-by: re-wrap lines to 100 columns.
@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}
@foolip foolip mentioned this pull request Feb 1, 2018
Closed
@foolip foolip mentioned this pull request Aug 29, 2018
Closed
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

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@foolip @upsuper