-
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
[rendering] animation frame callback handling when iframes are involved #10521
Comments
This change was made in Chrome relatively recently (early 2023) apparently because they thought it followed the spec better: https://source.chromium.org/chromium/chromium/src/+/766274f6af98374883d2c30ca2dc0fc116f407ad Collection of all documents is here: It used to walk the document tree like WebKit. The spec seems to say to collect all the docs at the beginning and each step says "for each doc of docs". What makes you think the behavior of Safari is what the spec is saying? |
@esprehn I was commenting on the behaviour of the rAF callbacks, not on the "liveness" of the document list. Safari matches the spec afaict because a child rAF scheduled from a parent rAF runs in the same rendering update, regardless of whether the child has existing callbacks scheduled at the beginning of the rendering steps. |
Safari's behavior here is intentional to follow the spec. |
Oh I see, yeah it looks like chrome did this on purpose on a spec violating optimization: I think they should probably revert that optimization. |
The spec violation is, afaict, this line, because it assumes that new tasks won't be scheduled: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/page/page_animator.cc;l=145;drc=4268052f5025da8b928c9e59a04493b396acaad3 And yeah it seems nobody really properly implements the "collect all docs upfront" bit... @rniwa do you know if that was a requirement or just an oversight? (Given you spent deliberate effort on trying to match the spec) |
It's more of an oversight although if no browser implements that way, maybe we should update the spec to match whatever browsers are doing today. |
@rniwa that was the part of the spec I was saying Safari does not implement. It walks the tree in every step: |
It seems blink does something close enough with regards to that, so maybe the spec is web compatible? |
@emilio yeah you're right, I re-read the Chrome code and edited my comment. My bad for the confusion there. |
@emilio Why does the test have log(t + childWin.performance.timeOrigin, "parent-from-parent"); ? Shouldn't it use parentWin |
Isn't this test hitting the step 4. Unnecessary rendering which would remove the iframe's doc if nothing has to be rendered there? It seems that the results do vary if there is an already running rAF loop in the iframe (thus avoiding that unnecessary rendering step), or if the "main" task is ran from a timer task instead of in the click event (which I assume would mark the frame as potentially needing an update). Not only some do not fire in the same tick anymore, but even the whole execution order can be affected. A few new tests.Note that these tests all use the main's document's With callbacksno rAF loop - no timer - with callback
no rAF loop - timer - with callback
rAF loop - timer - with callback
rAF loop - no timer - with callback
No callbackno rAF loop - no timer - no callback
no rAF loop - timer - no callback
rAF loop - timer - no callback
rAF loop - no timer - no callback
|
Typo, yeah, fixed it up.
My understanding of that step was always that it was meant for stuff like background tabs or invisible iframes, not iframes that are on screen and just don't have enqueued work at the beginning of the step... Of course that sentence is so vague that sure, it could hit it... See #10333 and co too. |
I don't think what Chrome's doing qualifies for that step, because if your iframe has mutated content (ex. appended new text, updated styles) Chrome will consider that document for updating rendering in step 22. The spec does not allow skipping raf but running rendering like that. It's the same list of docs. I also think what Chrome is doing is wrong because many APIs cause rAF to suddenly start happening in the same frame. For example if you add a MediaQueryList then the child rafs won't miss a frame anymore because of the heuristic.
Chrome is trying to detect in advance if any step of the process will have work, but I don't think that's actually possible because each loop may enqueue work for a child document in a later step's loop. |
Back from vacation! I think the key issue here is the document filtering part of the spec, as mentioned above. I'll quote the spec text here, since resolving this issue probably involves a careful parsing:
I think a careful reading indicates that the current behavior of chromium and Firefox Nightly is correct. The filtering is done prior to running any rAF callbacks, and for the failing test the above conditions hold true for the iframe: the UA believes that updating the rendering will have no visible effect and the doc's map of animation frame callbacks is empty. So it's correct to postpone the child-from-parent callback until the next rendering update. I have no opinion as to whether that's optimal, I think reasonable people can disagree on that point. I'm open to modifying the spec, but with the current spec I don't think there's anything to do here in chromium. |
I'm not convinced I agree with that read of the spec.
That's exceptionally vague, but given updating the rendering of a document can queue rendering updates on all same-origin documents connected to it, it doesn't really seem applicable to that case? To me that bit of the spec has always meant to allow throttling of background tabs / offscreen frames, but it's so vague so other interpretations are surely possible... |
After a careful re-reading, I see your points @esprehn and @emilio. I think the false positives from Now I am wondering why the "unnecessary rendering" clause exists at all. What situation does it address that is not covered by the preceding "rendering opportunity" clause? Could we just remove the "unnecessary rendering" language altogether? |
Yeah the big issue here is that it violates the requirement that requestAnimationFrame will always run before a browser applies a DOM mutation to the screen. Worse it's inconsistent since random things (from a web developers perspective) will make rAF stop missing frames. I spent many years convincing developers to use rAF instead of Promises to batch work. Even top framework developers were convinced they had to use Promises to "not miss a frame" because they thought sometimes the browser would paint but not run rAF. That wasn't true until this change landed though. @szager-chromium would it be possible to revert that change? I worry about backsliding into a world where frameworks stop using rAF because it's unreliable. |
I don't think there is such a "requirement" though. rAF will run before the paint only when it's been called before the animation callbacks have been fired. If it's called between animation callbacks and paint, e.g. in an rAF callback or in a ResizeObserver's callback, the paint will occur before. Here we are in a rAF callback, the question is whether rAF callbacks from other documents in the same navigable should be treated as the same pool of callbacks or not. Here is a small test exposing the issue in a more visual way. It does set both the parent and the iframe's background to red in the parent's rAF callback, then schedules the next iframe's rAF callback to set its background to yellow, and at the same time it also schedules the next parent's callback to set everything back to blue.
So yes, in Chrome, if you call Also note that every browser does respect firing the iframe's ResizeObserver's callback between the parent's rAF callback and the paint, so you can still hook there if needed. And from a web-dev's point of view, I don't think this "requirement" would be better enforced by walking down the tree at every step since an iframe lower in the DOM could call the rAF of an upper frame ending up in the same situation, unless you make rAF possibly reentrant from within an update the rendering task, which would be an even worse breakage of expectations IMO. |
@Kaiido I don't agree with your interpretation. The relevant spec text is:
...
Each doc effective snapshots its set of rAF callbacks to run at the beginning of processing for that document. When the parent doc runs its rAF callbacks, the child doc's set is still dynamic. If a rAF callback in the parent doc schedules rAF in the child doc, it should run in the child doc during the same rendering update. If a rAF callback schedules another rAF callback for the same document (parent or child), then it should run during the next rendering update. However, I think that's all a bit besides the point. @emilio's test case demonstrates that when a rAF callback in the parent doc schedules a rAF callback in the child doc, Chrome behaves differently depending on whether the child doc already had a registered rAF callback prior to starting the rendering update. That's a Chrome bug. @esprehn I'm not sure if you mean revert the spec language about "unnecessary rendering" or revert the recent chromium change that modified the behavior. I don't think we should revert the chromium change; it actually brings chromium's behavior more in line with the event loop spec, the above-mentioned bug notwithstanding. If we could agree to remove the "unnecessary rendering" clause from the spec, then it would be a very simple change in chromium to fix that bug by never skipping rAF processing for a doc with a rendering opportunity. |
Something that's not tested here and came about when we were discussing, is that the See https://jsfiddle.net/6xsdc3qr/2/ to reproduce. |
Sorry this is a bit off topic, still it might warrant some clarifications:
Yes, I didn't meant to say Chrome's behavior is per specs, just that it somehow "makes sense", sorry for the confusion. My comment was a direct response to the previous one claiming that web-devs would stop using rAF because it's now "unreliable". My point was that when advocating for rAF as a batch checkpoint it should be made clear that after the doc's rAF callback it will schedule for after the next paint, and that a web-dev should probably not expect calling rAF of document A from document B's rAF callback to fire in the current animation frame. Even if indeed the specs do require some kind of order between these docs, it turns out that no browser actually follows this order, and even the specced order might be complex to handle for a web-dev. So the point was that this discrepancy doesn't make rAF much more broken, even if I'm obviously for fixing all the interop issues around here. |
@Kaiido -- you make a good point about predictability of the platform. Here's the spec text that addresses the ordering issue:
Are the docs actually |
Not sure if any of the participants here are planning to attend TPAC next month. I'm not planning to attend, but I could be convinced to change my plans if there are other people going who are interested in having a breakout session on this topic. I have a strong interest in bullet-proofing the event loop and rendering update spec. |
What is the issue with the HTML Standard?
I noticed that all browsers do something different right now with regards to
requestAnimationFrame
when iframes are involved.Here's a trivial-ish test-case:
The interesting case is
Run test (child has no callback)
.That in Safari does what the spec says, running:
Current Firefox, Chrome, and Firefox Nightly do:
If you test the
Run test (child has callback)
test, then Safari, Chrome and Firefox Nightly agree with the spec:Firefox release does:
Which is kinda odd, but makes some amount of sense. It collects all callbacks at once, and fires them. Any callback scheduled from an existing one gets fired on the next rendering update.
To be clear, I think the spec is somewhat sane, but I have questions:
docs
is frozen for the whole rendering update. That's not the case in neither Gecko or WebKit at least (not sure about Chrome).<iframe>
duringrAF
, that document would run theIntersectionObserver
steps.After finding this I kinda wanna change Gecko to match Safari and the spec and add some good tests for this, or to a somewhat simpler version of Firefox Release's behavior (the one that prevents nested rAF from firing on the same update)...
cc @whatwg/rendering @chrishtr @rniwa @annevk @zcorpan @aosmond @mfreed7 @szager-chromium
The text was updated successfully, but these errors were encountered: