-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Introduce animation frame tasks #2627
Conversation
Other specifications (Fullscreen) should use this by saying "queue an animation frame task for /doc/ to ...". Fixes the fullscreen part of #707.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing I'm wondering about is whether this abstraction is correct. I thought we had come to the conclusion that we didn't want a callback per document, but rather one for all similar-origin documents?
source
Outdated
<var>doc</var>, it must run the following steps:</p> | ||
|
||
<ol> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This newline can be removed.
source
Outdated
list.</p></li> | ||
|
||
<li><p>For each <var>task</var> in <var>tasks</var>, run <var>task</var>.</p></li> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one as well.
callback identifier</dfn>, which is a number which must initially be zero.</p> | ||
|
||
<p>When the user agent is to <dfn>queue an animation frame task</dfn> for a <code>Document</code> | ||
<var>doc</var> with a <span data-x="concept-task">task</span> <var>task</var>, it must append |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are actual tasks? That doesn't seem entirely correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All we need is something that can be "run", a closure with no arguments. Using the existing animation frame callbacks (ignoring the timestamp) is in fact an option, but I'm not sure if it's a good (or bad) idea to mix the two.
Also, should we just have fullscreen tasks here? Otherwise if lots of things end up queuing animation frame tasks their relative order is unknown. |
For this I would like review in particular from @annevk and @upsuper. Animation frame tasks are bound to a document, and this was never very clear with "As part of the next animation frame task, run these substeps" in https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen. The way I propose to use this to solve whatwg/fullscreen#74 is to apply all of the state changes (top layer manipulation) as soon as the resize happens and then queue an animation frame task for each "fullscreenchange" event to be fired. A consequence of this is that the order of events will always be in "browser context tree order", which will reverse the current order when exiting fullscreen. |
Do you mean in #707? When would the list of tasks for similar-origin documents be processed? If it's at the step I'm now traversing all the documents, then couldn't one use the top level browsing context's document to achieve that kind of effect?
#707 (comment) is the last I thought about this. I suspect we'll be better off if other specs can use a primitive like this. If the order of the two algorithms that queue the animation frame tasks is not well defined, then there would already be a problem if they both "queue a task", fire events, or do anything allowing the order to be observed. |
I guess that's a good point. Only doing the traversing here for any layout-related work is clearly better than everyone doing their own traversal. It would mean however that you get fullscreen for outer, than maybe some UI event for inner, than fullscreen for inner. Rather than all fullscreen events directly adjacent. That's probably reasonable, but if any framework is doing its own cross-document synchronizing, it's going to end up in trouble. |
I think it makes sense to make I share the concern with @annevk that introducing this general concept might not be a good idea. Making fullscreen events not adjacent may be a problem, while I'm more worried about the order between different kinds of steps. I mean, we may want to make them dispatch in a specific order so that authors may find it easier to write handler functions modularly and performantly. For example, I have a feeling that we may actually want fullscreen events to be dispatched at a very early stage, probably even before resize, because conceptually fullscreen change can trigger some state-related DOM change, and then later handler functions can rely on that the DOM state has been changed, and just apply their size-based changes accordingly. I'm not sure if this is something we should take into account, and whether authors would actually want to rely on that. |
Sorry it's taking a while to prepare the corresponding Fullscreen PR, I just keep running into smaller changes that would be nice to do first. The top commit in whatwg/fullscreen#86 should give some idea about where this is going, even though it's in a rough shape. Looks like that was automatically published at https://fullscreen.spec.whatwg.org/branch-snapshots/animation-frame-tasks/, so PTAL and see if that changes anything about this PR. |
whatwg/fullscreen#92 uses this now and is ready for review. |
Tests for whatwg/url#53 and friends, as fixed by whatwg/html#2627.
I'm abandoning this in favor of #2763 after discussion with @upsuper on whatwg/fullscreen#92. I still like the idea of a generic task concept here to make it easier for other specs to do things at animation frame timing, but will wait until the next time the need comes up. |
Other specifications (Fullscreen) should use this by saying "queue an
animation frame task for /doc/ to ...".
Fixes the fullscreen part of #707.