Open Bug 1272495 Opened 9 years ago Updated 2 years ago

NS_FRAME_MAY_BE_TRANSFORMED should be cleared once script animation of transform is finished

Categories

(Core :: DOM: Animation, defect)

defect

Tracking

()

Tracking Status
firefox49 --- affected

People

(Reporter: hiro, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

I can't provide any test html files for now, but I confirmed that NS_FRAME_MAY_BE_TRANSFORMED is not cleared in debugger with following code. var target = document.getElementById('target'); var anim = target.animate({ transform: ['translateX(0px)', 'translateX(100px)'] }, 1000); anim.onfinish = function() { setTimeout(function() { // wait some frames just in case var newAnim = target.animate({ opacity: [0, 1] }, 1000); // at this point NS_FRAME_MAY_BE_TRANSFORMED is still set. }, 100); };
Blocks: 1223658
It looks to me that ElementHasAnimations is not cleared either.
I am guessing that we can set/clear it in EffectSets, but not convinced because there may be cases that we don't have nsIFrame there.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > It looks to me that ElementHasAnimations is not cleared either. ElementHasAnimations is not intended to be cleared. It is just a performance optimization that allows us to quickly skip any elements that have never had any animations (which is most elements). Maybe NS_FRAME_MAY_BE_TRANSFORMED is similar but I was under the impression that it influenced layerization. If it doesn't then it's probably ok not to clear it.
(In reply to Brian Birtles (:birtles) from comment #3) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > > It looks to me that ElementHasAnimations is not cleared either. > > ElementHasAnimations is not intended to be cleared. It is just a performance > optimization that allows us to quickly skip any elements that have never had > any animations (which is most elements). > > Maybe NS_FRAME_MAY_BE_TRANSFORMED is similar but I was under the impression > that it influenced layerization. If it doesn't then it's probably ok not to > clear it. I think it doesn't affect layerization. But in these days, and from now on we will have bunch of animations created by animate() method in real world, I think it's worth clearing NS_FRAME_MAY_BE_TRANSFORMED, the cost of clearing flag is fairly light.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > I am guessing that we can set/clear it in EffectSets, but not convinced > because there may be cases that we don't have nsIFrame there. In EffectSet::AddEffect() (or after calling AddEffect() in KeyframeEffectReadOnly::UpdateTargetRegistration()) which I am trying to set the flag, there is another problem that mWinsInCascade is not yet updated, as a result HasAnimationOfProperty(eCSSProperty_transform) fails. But I think it's OK if we property clear the flag when the animation is finished.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > > I am guessing that we can set/clear it in EffectSets, but not convinced > > because there may be cases that we don't have nsIFrame there. > > In EffectSet::AddEffect() (or after calling AddEffect() in > KeyframeEffectReadOnly::UpdateTargetRegistration()) which I am trying to set > the flag, there is another problem that mWinsInCascade is not yet updated, > as a result HasAnimationOfProperty(eCSSProperty_transform) fails. But I > think it's OK if we property clear the flag when the animation is finished. Oops. I meant that 'checking mProperties without mWinsInCascade flag' is OK to me.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > > > I am guessing that we can set/clear it in EffectSets, but not convinced > > > because there may be cases that we don't have nsIFrame there. > > > > In EffectSet::AddEffect() (or after calling AddEffect() in > > KeyframeEffectReadOnly::UpdateTargetRegistration()) which I am trying to set > > the flag, there is another problem that mWinsInCascade is not yet updated, > > as a result HasAnimationOfProperty(eCSSProperty_transform) fails. But I > > think it's OK if we property clear the flag when the animation is finished. > > Oops. I meant that 'checking mProperties without mWinsInCascade flag' is OK > to me. Ah, it is NOT actually good. We should avoid this.
There are two animations created by animate() on red box and an animation created by animate() on blue box. The first one on the red box is a simple transform from translateX(0px) to translateX(100px). The second one starts after the first one is finished. The second one is transform: ['none', 'none', 'translateX(100px)']. The animation on the blue box has the same property of the second one on red box. Currently we don't create layer for transform animations until the transform style is applied. It's the case of the blue box. But the second animation on the red box is layerized at the beginning of the animation because of NS_FRAME_MAY_BE_TRANSFORMED flag set by the first transform on the red box.
I think we have to introduce something like AnimationsWithDestroyedFrame[1]. I thought initially we can clear the flag when we remove the effect from EffectSet. But, at that point (i.e, before styling), it's hard to tell that the transform style being set to the nsIFrame is set by this animation or not. (We can't clear the flag if some other transform style is applied because the flag will not be restored until reframing.) So a property way I can think of is to clear the flag after all styling has processed, at that point the transform style set by the finished/cancelled animation is not applied any more (except fill:forwards cases). That means we can clear the flag if there is no other transform there. [1] https://dxr.mozilla.org/mozilla-central/rev/1f1a8b96d5167153d1f750439ba6a1063155a4bc/layout/base/RestyleManager.h#263
Attached file A reftest (obsolete) (deleted) —
Comment on attachment 8754113 [details] A reftest I did post an incomplete test.
Attachment #8754113 - Attachment is patch: false
Attachment #8754113 - Attachment is obsolete: true
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: