Closed
Bug 1398661
Opened 7 years ago
Closed 7 years ago
stylo: content property animation does not work without explicit style flush
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(2 files)
Attaching file has an infinite content property animation, the content property is updated only if there is explicit style flush (e.g mouse movement).
Assignee | ||
Comment 2•7 years ago
|
||
'content' property animation is very rare, so I don't think this blocks shipping 57, but it would be nice.
Flags: needinfo?(hikezoe)
Priority: -- → P3
Assignee | ||
Comment 3•7 years ago
|
||
I started looking into this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
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!
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
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
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
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.
Attachment #8907410 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c28f6309fe34
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•