Animation.ready should not be fulfilled in the case where animating elements inside an iframe which is clipped by overflow:hidden scroll port in an ancestor document
Categories
(Core :: DOM: Animation, defect, P3)
Tracking
()
People
(Reporter: hiro, Assigned: boris)
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
text/html
|
Details |
I am attaching two files to reproduce this issue.
Reporter | ||
Comment 1•5 years ago
|
||
You need to run with enabling fission related preferences like this;
./mach run --setpref "fission.autostart=true" --setpref "fission.oopif.attribute=true" parent.html
Reporter | ||
Comment 2•5 years ago
|
||
Attached wrong file. :/
Reporter | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Hiro, is this still a bug?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
You need to run with enabling fission related preferences like this;
./mach run --setpref "fission.autostart=true" --setpref "fission.oopif.attribute=true" parent.html
btw, the fission.oopif.attribute
pref no longer exists. You only need to set pref fission.autostart=true
(and gfx.webrender.all=true
) to enable Fission. (Running Fission without WebRender is not a supported configuration.)
Comment 5•5 years ago
|
||
In that case, we'll track this bug as a blocker for Fission Nightly (M6).
Comment 6•5 years ago
|
||
Hiro, I don't think this needs to block enabling Fission in Nightly since it seems like a bit of an edge case so I am changing the Fission milestone to M7. Do you agree?
Reporter | ||
Comment 7•5 years ago
|
||
Yep, I agree. But we should take time on this before we release Fission. Probably this issue is hard to notice in the wild, which means it will consume web developer's time who will be suffering from issues caused by this, that's not what we want.
Comment 8•5 years ago
|
||
Thank you!
Updated•5 years ago
|
Comment 9•4 years ago
|
||
Boris, could you please find someone to work on this?
Reporter | ||
Comment 10•4 years ago
|
||
I was guessing the reason why ready
Promise is not fullfilld there is because we don't have a refresh driver for the OOP iframe. I wonder whether rAF callbacks or setTimeout callbacks are invoked or not.
Assignee | ||
Comment 11•4 years ago
|
||
(In reply to Neha Kochar [:neha] from comment #9)
Boris, could you please find someone to work on this?
Perhaps hiro or I can work on this. Hiro, please feel free to take this if you have time.
Reporter | ||
Comment 12•4 years ago
|
||
If my suspicious is correct, i.e. there is no refresh driver, then it will be quite hard to solve this issue. I also wonder whether it works on Chrome or not, if it's not, I'd rather change/clarify the Web Animation spec. Emilio told me that the HTML spec defines rendering-opportunity and rAF callbacks are not invoked if there is no rendering-opportunity. I start thinking animation stuff should behave as same as rAF.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 13•4 years ago
|
||
Based on the talk with hiro, we should do the following things for this bug first:
- make sure whether the refresh driver exists or not
- check the Chrome behavior (and Safari)
Need to know if this is a spec issue. If so, we need to file one.
Comment 14•4 years ago
|
||
Boris, could you please re-prioritize this work as it blocks Fission Beta M7 milestone (Fx88)?
Assignee | ||
Comment 15•4 years ago
|
||
OK. I will take some time for this bug for Firefox 88.
Assignee | ||
Comment 17•4 years ago
|
||
Weird. I cannot reproduce this (i.e. parent.html and anim.html) on the current Nightly (with fission.autostart=true and gfx.webrender.all=true). I always see "ready" in the console (in both Chrome Canary and Firefox Nightly 88.0a1).
Assignee | ||
Comment 18•4 years ago
|
||
Hiro, would you mind checking the test case you attached again? Perhaps someone fixed this already.
Reporter | ||
Comment 19•4 years ago
|
||
That's because fission attribute no longer works as expected. We need a real cross origin URL.
Reporter | ||
Comment 20•4 years ago
|
||
Hmm, weird. On my linux box the original test case works as expected, if our tooltip on the tab correctly shows processes, I am seeing two different processes, one is for the top level content, the other one is for an OOP iframe. And I don't get the animation.ready.
Reporter | ||
Comment 21•4 years ago
|
||
Confirmed the new test case doesn't fullfill the animation.ready either on Windows or Mac.
Assignee | ||
Comment 22•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
Confirmed the new test case doesn't fullfill the animation.ready either on Windows or Mac.
Thanks for this. And looks like animation.ready
is not fulfilled in Google Chrome Canary (on mac), either.
Comment 23•4 years ago
|
||
Working on clarifying remaining blockers for M7 and/or future milestones...
Is Bug 1495940 still dependent on this item?
Is this bug still an issue? The outcome isn't clear from the dialog above. If so, Is this a blocker for M7? If so, can we update to the appropriate milestone so we have visibility on this as a sub-bug risk? ty
Assignee | ||
Comment 24•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
I was guessing the reason why
ready
Promise is not fullfilld there is because we don't have a refresh driver for the OOP iframe. I wonder whether rAF callbacks or setTimeout callbacks are invoked or not.
I'm not 100% sure when the refresh driver creates, but I notice these differences (in layout-debugger):
- If the animation is not hidden (or remove
overflow:hidden;
):
Animation::mReady->MaybeResolve()
is called (inResumeAt()
) and looks like there are two refresh drivers (for content process) running and one of them dispatchs an animation event. I guess the one is for OOP iframe.- Gecko has the rAF callback in the OOP iframe.
- In Google Chrome, no animation.ready, and no rAF callback.
- In Safari, no animation.ready, but it has rAF callback.
- if the animation is hidden:
- No one calls
Animation::mReady->MaybeResolve()
. But I still see at least two refresh drivers running for content process. - Gecko has the rAF callback in the OOP iframe.
- In Google Chrome, no animation.ready, and no rAF callback.
- In Safari, no animation.ready, but it has rAF callback.
So both Safari and Chrome don't fulfill animation.ready. Not sure which behavior is reasonable anyway. Maybe no animation.ready fulfilled is fine in OOP iframe for now.
BTW, how do I map the refresh driver to the OOP iframe?
Reporter | ||
Comment 25•4 years ago
|
||
A problem I am having in my mind is the inconsistency in between OOP iframes and in-process iframes. Also the inconsistency between rAF callback and animation.ready. Those should match I think.
So now I believe not fulfilling animation.ready where the animation is out of the viewport is fine in terms of the rendering-opprtunity, so we shouldn't fullfill it in non fission case as well. And if we decided that we don't fullfill animation.ready in such cases, we don't invoke rAF callbacks either in the cases.
(In reply to Boris Chiou [:boris] from comment #24)
BTW, how do I map the refresh driver to the OOP iframe?
Though I don't quite understand what you mean, the refresh driver is generated when the pres shell for the root document of the OOP iframe is created.
Assignee | ||
Comment 26•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
A problem I am having in my mind is the inconsistency in between OOP iframes and in-process iframes. Also the inconsistency between rAF callback and animation.ready. Those should match I think.
So now I believe not fulfilling animation.ready where the animation is out of the viewport is fine in terms of the rendering-opprtunity, so we shouldn't fullfill it in non fission case as well.
I see. I agree with this. So perhaps we should change this bug to not fulfill animation.ready for iframe, and perhaps we have to update the spec (or file a spec issue for this).
And If so, this might not be related to fission.
And if we decided that we don't fullfill animation.ready in such cases, we don't invoke rAF callbacks either in the cases.
So the behavior will be similar to Chrome. And so we need an extra bug for rAF.
(In reply to Boris Chiou [:boris] from comment #24)
BTW, how do I map the refresh driver to the OOP iframe?
Though I don't quite understand what you mean, the refresh driver is generated when the pres shell for the root document of the OOP iframe is created.
Thanks. So the refresh driver should be unique for each pres shell.
Brian, do you have any concerns about "not fulfill animation.ready (and finish?) for OOP iframe when out of the viewport"? I can file a spec issue in Web-animations API if you prefer this way.
Reporter | ||
Comment 27•4 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #26)
Brian, do you have any concerns about "not fulfill animation.ready (and finish?) for OOP iframe when out of the viewport"? I can file a spec issue in Web-animations API if you prefer this way.
To be precise s/OOP iframe/any iframes/, which means any animation will never be generated in such cases I assume just like CSS animations in display:none subtree. Hmm, that means we also need to stop any animations once after the animations get in the situation? that's kind of annoying.
Comment 28•4 years ago
|
||
Sorry, I'm having a bit of a hard time following all the detail here but it sounds like for the particular case of a hidden iframe we have:
Browser | .ready fulfilled | rAF fired | setTimeout called |
---|---|---|---|
Firefox OOP-iframe | no | yes | yes |
Firefox in-process iframe | yes | yes | yes |
Chrome | no | no | yes |
Safari | no | yes | ? |
Is that right?
And Hiro was suggesting that we make Firefox consistent with Chrome for both OOP and in-process iframes but concerned that it means we'd need additional logic to make sure handle dynamic updates to the situation?
Reporter | ||
Comment 29•4 years ago
|
||
Though I am not sure about the behaviors of Chrome and Safari (and IIRC Safari doesn't support OOP iframes yet), basically that's right. And I suggested Boris on Matrix to fullfill animation.ready in out-of-view OOP iframes, I think it should be easy since the refresh driver is running there.
Comment 30•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
Though I am not sure about the behaviors of Chrome and Safari (and IIRC Safari doesn't support OOP iframes yet), basically that's right. And I suggested Boris on Matrix to fullfill animation.ready in out-of-view OOP iframes, I think it should be easy since the refresh driver is running there.
That sounds great, thanks. Making Firefox consistent between OOP and in-process iframes seems like enough to solve this bug.
Making rAF etc. more generally consistent between browsers might be worth following up at some other time but resolving .ready
when other browsers don't, seems unlikely to cause major Web compat issues for now.
Updated•4 years ago
|
Comment 31•4 years ago
|
||
We can fix this in M8 because the behavior is the same in Chrome and Safari also. We should add WPT test for this so that we can bring all browsers together on this behavior.
Assignee | ||
Comment 32•4 years ago
|
||
A quick update from my debug.
We definitely have one nsRefreshDriver for the OOP iframe and another one for its parent document, but:
- If the animation is hidden, we are using
mozilla::InactiveRefreshDriverTimer::TickOne()
to tick it, so only update once and no other animation update. So there is no ready and no finished promise resolved. - If the animation is not hidden, we are using
mozilla::RefreshDriverTimer::Tick()
, so everything looks normal.
Note: If we disable fission, we only have one nsRefreshDriver for the parent document.
Still have to check why we use an inactive refresh driver for hidden animations. Perhaps it's a way for optimization or it's just a bug.
Reporter | ||
Comment 33•4 years ago
|
||
Okay, so using InactiveRefreshDriverTimer means the situation is similar to background tabs. what I don't quite understand is why rAF callback is invoked, isn't it constantly invoked? I mean if script tries to register rAF every time in the callback, is it repeatedly invoked? I am also wondering it might be possible that animation.ready is fullfilled 1 minutes later or so? Because in my understandings, InactiveRefreshDriverTimer isn't just triggering tick often.
(In reply to Boris Chiou [:boris] from comment #32)
Note: If we disable fission, we only have one nsRefreshDriver for the parent document.
Subdocuments in the same process use the same refresh driver instance of the in-process root document's refresh driver.
Assignee | ||
Comment 34•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
Okay, so using InactiveRefreshDriverTimer means the situation is similar to background tabs. what I don't quite understand is why rAF callback is invoked, isn't it constantly invoked? I mean if script tries to register rAF every time in the callback, is it repeatedly invoked?
No, it isn't. I tried to call rAF callback per 5 seconds.
e.g.
function rAF() {
dump("a rAF callback is invoked\n");
setTimeout(() => {
requestAnimationFrame(rAF);
}, 5000);
}
And with fission, in the hidden OOP iframe (with inactiveRefreshDriverTimer), the rAF is not constantly invoked. The period between two rAF callbacks is a little be random. Mostly it's more than 5 seconds.
I am also wondering it might be possible that animation.ready is fullfilled 1 minutes later or so? Because in my understandings, InactiveRefreshDriverTimer isn't just triggering tick often.
No. I didn't see any animation.ready happened after 1 minute.
We mark the BrowserChild as hidden or visibility in BrowserChild::UpdateVisibility. If this should be hidden, we set its preshell as inactive, which throttles the nsRefreshDriver. That's why we are using inactiveRefreshDriver for the OOP iframe.
So now I think rAF is also buggy in the "hidden" OOP iframe.
Reporter | ||
Comment 35•4 years ago
|
||
I don't think the rAF behavior with inactive refresh drivers is buggy, it's working what it's supposed to be. Anyway, a big question on this issue is why animation.ready will never be fulfilled. (another small question in my mind is whether the rAF behavior is same in the case of non Fission or not).
Assignee | ||
Comment 36•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
I don't think the rAF behavior with inactive refresh drivers is buggy, it's working what it's supposed to be.
I see. The behavior is reasonable. However, is this the expected behavior for OOP iframe? Should we keep using the inactive refresh driver in OOP iframe with hidden animations? Now I have no idea about which refresh driver we should use for this case because we treat it as a background tab.
Anyway, a big question on this issue is why animation.ready will never be fulfilled.
Not sure, still have to debug this for the inactive refresh driver. Will update this later.
(another small question in my mind is whether the rAF behavior is same in the case of non Fission or not).
In this case (without fission), the refresh driver of "parent.html" is a regular refresh driver (and "animation-ready.html" uses the same refresh driver), so rAF behavior is not the same.
Updated•4 years ago
|
Comment 37•4 years ago
|
||
Boris, what are the next steps for this, and resolution plan?
Assignee | ||
Comment 38•4 years ago
|
||
(In reply to Neha Kochar [:neha] from comment #37)
Boris, what are the next steps for this, and resolution plan?
It seems the current behavior is expected because we are using an inactive refresh driver timer for the OOP iframe with hidden animations, so my next step is to understand why animation.ready is never fulfilled. This may be a potential bug.
The most possible resolution plan is: we still fire the animation.ready
event, though we may have to wait for an unpredicted duration. So we will have similar behaviors for both the animation.ready
and rAF
. If this is not acceptable, we probably not use the inactive refresh driver timer for the OOP iframe with hidden animations.
Reporter | ||
Comment 39•3 years ago
|
||
I guess now I know what's going on there. Will check my guess is correct or not.
Reporter | ||
Comment 40•3 years ago
|
||
My guess was wrong. I was guessing that OOP iframe's content outside of the displayport doesn't get styled, but mattwoodrow told me it gets styled by the refresh driver tick.
Comment 41•3 years ago
|
||
If I'm understanding this correctly, this particular item has been pushed due to current behavior matching Chrome and Safari. Currently, Firefox with fission has no web-compat issues on this because we match others…is this currently being considered a corner case and a non- mission critical item? Do we have anything actionable or are we looking at moving this to a “Future” milestone?
Reporter | ||
Comment 42•3 years ago
|
||
"animation.ready" is relatively new so it will unlikely cause webcompat issues (IIRC Safari shipped it this year), yes. The test case I wrote initially happened to be found when I used animation.ready in a test for fission, without knowing what causes this animation.ready failure, it's quite hard to tell whether there are other issues causes by the same problem, and the issues might be real problems in the wild.
Reporter | ||
Comment 43•3 years ago
|
||
I did track down what's going on there;
So a problem is here in TriggerPendingAnimations, in the function we enumerates sub documents regardless whether the sub document is visible (i.e. has a rendering opportunity) or not, thus we fullfill animation.ready on non-Fission cases.
(In reply to Brian Birtles (:birtles) from comment #28)
Browser .ready fulfilled rAF fired setTimeout called Firefox OOP-iframe no yes yes Firefox in-process iframe yes yes yes Chrome no no yes Safari no yes ?
Now I think this Chrome's behavior is correct. We shouldn't fullfill animation.ready in in-process iframes or OOP iframes either. Also we shouldn't invoke any rAF callbacks either.
So, what I think currently is, this is not a Fission issue, in Fission world, the things work as expected (unintentionally though). We should fix the behavior in-process cases instead. I am going to reuse this bug for the in-process cases.
Reporter | ||
Comment 44•3 years ago
|
||
FWIW , the reason why animation.ready is not fullfilled inside the invisible OOP iframe is we don't paint the iframe document, thus we never call TriggerPendingAnimations.
Comment 45•3 years ago
|
||
Would this get fixed if we fixed the TODO
in https://phabricator.services.mozilla.com/D118703 to not activate the pres shell of invisible in-process iframes?
Reporter | ||
Comment 46•3 years ago
|
||
Yeah, looks like the rAF case should be fixed by the TODO, the animation.ready case should be handled separately here in TriggerPendingAnimations, but yeah, the animation case should also be fixed by using ShouldThrottleFrameRequests there.
Assignee | ||
Comment 47•3 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
Yeah, looks like the rAF case should be fixed by the TODO, the animation.ready case should be handled separately here in TriggerPendingAnimations, but yeah, the animation case should also be fixed by using ShouldThrottleFrameRequests there.
Yes. Using ShouldThrottleFrameRequests() in TriggerPendingAnimations() works as expected. I am writing a patch (and a test) for in-process case.
Assignee | ||
Comment 48•3 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #47)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
Yeah, looks like the rAF case should be fixed by the TODO, the animation.ready case should be handled separately here in TriggerPendingAnimations, but yeah, the animation case should also be fixed by using ShouldThrottleFrameRequests there.
Yes. Using ShouldThrottleFrameRequests() in TriggerPendingAnimations() works as expected. I am writing a patch (and a test) for in-process case.
Using ShouldThrottleFrameRequests() is not enough. We wouldn't like to trigger pending animations only if
- the frame is throttled (e.g. we don't paint this iframe), and
- this is a cross-origin iframe
So for example.
<iframe id="iframe" srcdoc='<div id="target"></div>'></iframe>
It seems we treat this as non cross-origin, so we should still trigger pending animation in this case.
Reporter | ||
Comment 49•3 years ago
|
||
Boris, we don't need to rush to fix this. I think we should just open a spec issue for the case where the target element's browsing context has no rendering opportunity. Though I think animation.ready should not be fulfilled in the case of no rendering opportunity, that's my instinct. Given that web animations can be generated in display:none subtree, there maybe cases web developers expect animation.ready is fulfilled even if it's not rendered at all.
Assignee | ||
Comment 50•3 years ago
|
||
I see. Thanks for the clarification. :)
Description
•