Attaching file has an infinite content property animation, the content property is updated only if there is explicit style flush (e.g mouse movement).

Hiro, should this block shipping?

'content' property animation is very rare, so I don't think this blocks shipping 57, but it would be nice.

I started looking into this.

A problem for 'content' animation is that we call EffectCompositor::PreTraverseInSubtree() with a generated content for |aRoot|. So, nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(target.mElement, aRoot) fails. The |target.mElement| is the parent of the generated content unfortunately. Though I don't know the reason why the animation worked before bug 1383332, I guess it worked accidentally.

Gah, I had been trying to write a reftest for this, finally I notice it's impossible to write reftest since when we remove 'reftest-wait' from documentElement, nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(target.mElement, aRoot) check succeeds!

getComputedStyle() can't be used for test case either because of the same reason in comment 5. I did give up writing test cases for this bug. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f94a632aeba2422ef6b20e893bc95075f646f7

The try in comment 6 revealed that we are calling inDOMUtils::GetCSSStyleRules() with a pseudo element without specifying |aPseudo|. https://treeherder.mozilla.org/logviewer.html#?job_id=130482024&repo=try&lineNumber=8179

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > The try in comment 6 revealed that we are calling > inDOMUtils::GetCSSStyleRules() with a pseudo element without specifying > |aPseudo|. > > https://treeherder.mozilla.org/logviewer. > html#?job_id=130482024&repo=try&lineNumber=8179 I believe GetCSSStyleRules should be called with pseudo type if it's called for pseudo element. Filed bug 1399314.

As per a try [1] which adds an assertion to check the given element is neigher ::before nor ::after in nsComputedDOMStyle::DoGetStyleContextNoFlush(), the only use case that getCSSSstyleRules() is called against the pseudo element it self is in devtools. I am going to add an assertion in EffectCompositor::PreTraverse(Element*, CSSPseudoElementType) after bug 1399314 is fixed. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3606f6d8b77d5af2d85f360da9fdc6b31e70344f

Comment on attachment 8907410 [details] Bug 1398661 - Convert target element to the parent if the target element is pseudo in EffectCompositor::PreTraverseInSubtree/PreTraverse. https://reviewboard.mozilla.org/r/179090/#review184274 ::: dom/animation/EffectCompositor.cpp:992 (Diff revision 1) > + // of the root element later in this function, but for pseudo elements the > + // element in mElementsToRestyle is the parent of the pseudo. > + if (aRoot && > + (aRoot->IsGeneratedContentContainerForBefore() || > + aRoot->IsGeneratedContentContainerForAfter())) { > + aRoot = aRoot->GetParent()->AsElement(); nit: Just GetParentElement()? ::: dom/animation/EffectCompositor.cpp:1035 (Diff revision 1) > } > > // Ignore restyles that aren't in the flattened tree subtree rooted at > // aRoot. > if (aRoot && !nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle( > target.mElement, aRoot)) { Should we instead special-case this check? Seems somewhat cleaner, but no big deal either way.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11) > Comment on attachment 8907410 [details] > Bug 1398661 - Convert target element to the parent if the target element is > pseudo in EffectCompositor::PreTraverseInSubtree/PreTraverse. > > https://reviewboard.mozilla.org/r/179090/#review184274 > > ::: dom/animation/EffectCompositor.cpp:992 > (Diff revision 1) > > + // of the root element later in this function, but for pseudo elements the > > + // element in mElementsToRestyle is the parent of the pseudo. > > + if (aRoot && > > + (aRoot->IsGeneratedContentContainerForBefore() || > > + aRoot->IsGeneratedContentContainerForAfter())) { > > + aRoot = aRoot->GetParent()->AsElement(); > > nit: Just GetParentElement()? Nice. > ::: dom/animation/EffectCompositor.cpp:1035 > (Diff revision 1) > > } > > > > // Ignore restyles that aren't in the flattened tree subtree rooted at > > // aRoot. > > if (aRoot && !nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle( > > target.mElement, aRoot)) { > > Should we instead special-case this check? Seems somewhat cleaner, but no > big deal either way. I will go with the current way, here is inside a closure.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c28f6309fe34 Convert target element to the parent if the target element is pseudo in EffectCompositor::PreTraverseInSubtree/PreTraverse. r=emilio