Open Bug 1578309 Opened 5 years ago Updated 3 years ago

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)

defect

Tracking

()

ASSIGNED

People

(Reporter: hiro, Assigned: boris)

Details

Attachments

(1 file, 3 obsolete files)

Attached file parent.html (obsolete) (deleted) —

I am attaching two files to reproduce this issue.

Attached file child.html (obsolete) (deleted) —

You need to run with enabling fission related preferences like this;

./mach run --setpref "fission.autostart=true" --setpref "fission.oopif.attribute=true" parent.html

Attached file anim.html (obsolete) (deleted) —

Attached wrong file. :/

Attachment #9089935 - Attachment is obsolete: true

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.)

Flags: needinfo?(hikezoe.birchill)

Yes.

Flags: needinfo?(hikezoe.birchill)

In that case, we'll track this bug as a blocker for Fission Nightly (M6).

Fission Milestone: --- → M6

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?

Fission Milestone: M6 → M7
Flags: needinfo?(hikezoe.birchill)

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.

Flags: needinfo?(hikezoe.birchill)

Thank you!

Severity: normal → S4

Boris, could you please find someone to work on this?

Flags: needinfo?(boris.chiou)

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.

(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.

Flags: needinfo?(boris.chiou)

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: nobody → boris.chiou

Based on the talk with hiro, we should do the following things for this bug first:

  1. make sure whether the refresh driver exists or not
  2. check the Chrome behavior (and Safari)

Need to know if this is a spec issue. If so, we need to file one.

Boris, could you please re-prioritize this work as it blocks Fission Beta M7 milestone (Fx88)?

Flags: needinfo?(boris.chiou)

OK. I will take some time for this bug for Firefox 88.

Will check this next week.

Flags: needinfo?(boris.chiou)

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).

Hiro, would you mind checking the test case you attached again? Perhaps someone fixed this already.

Flags: needinfo?(hikezoe.birchill)
Attached file A new case (deleted) —

That's because fission attribute no longer works as expected. We need a real cross origin URL.

Attachment #9089934 - Attachment is obsolete: true
Attachment #9089936 - Attachment is obsolete: true
Flags: needinfo?(hikezoe.birchill)

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.

Confirmed the new test case doesn't fullfill the animation.ready either on Windows or Mac.

(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.

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
Flags: needinfo?(boris.chiou)

(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):

  1. If the animation is not hidden (or remove overflow:hidden;):
  • Animation::mReady->MaybeResolve() is called (in ResumeAt()) 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.
  1. 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?

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.

(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.

Flags: needinfo?(brian)

(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.

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?

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.

(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.

Flags: needinfo?(brian)
Whiteboard: [3/12] ETA: TBD

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.

Status: NEW → ASSIGNED
Fission Milestone: M7 → M8

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.

Flags: needinfo?(boris.chiou)

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.

(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.

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).

(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.

Whiteboard: [3/12] ETA: TBD

Boris, what are the next steps for this, and resolution plan?

Flags: needinfo?(boris.chiou)

(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.

Flags: needinfo?(boris.chiou)

I guess now I know what's going on there. Will check my guess is correct or not.

Flags: needinfo?(hikezoe.birchill)

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.

Flags: needinfo?(hikezoe.birchill)

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?

"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.

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.

No longer blocks: rendering-fission
Fission Milestone: M8 → ---
Summary: Animation.ready is not fulfilled in the case where animating elements inside an out-of-process iframe which is clipped by overflow:hidden scroll port in an ancestor document → 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

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.

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?

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.

(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.

(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

  1. the frame is throttled (e.g. we don't paint this iframe), and
  2. 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.

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.

I see. Thanks for the clarification. :)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: