Closed Bug 1368889 Opened 7 years ago Closed 7 years ago

stylo: test_transitions.html fails for pseudo elements

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files)

A simplified test case which fail in test_transition.html
594 bytes, text/html
Details
Bug 1368889 - Post animation restyle hint againt pseudo element instead of its parent.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
      No description provided.
The transition in the test file works fine, but getComputedStyle() returns a wrong value.
I just realized that currently we don't set animation-only dirty bit to pseudo element at all. (Thanks to Cameron and Brian)

I am not sure this bug is fixed by the dirty bit for pseudo, but we should fix it anyway.
I did confirm that setting the dirty bit make the failure tests in test_transitions.html pass.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> I did confirm that setting the dirty bit make the failure tests in
> test_transitions.html pass.

Good news! Thanks for fixing this.
Comment on attachment 8875466 [details]
Bug 1368889 - Post animation restyle hint againt pseudo element instead of its parent.

https://reviewboard.mozilla.org/r/146904/#review150986

::: layout/base/ServoRestyleManager.cpp:86
(Diff revision 1)
> +    // FIXME: When reframeing happens, EffectCompositor::mElementsToRestyle
> +    // still has unbinded old pseudo element. We should drop it.

If the remaining unbinded element in mElementsToRestyle is not a problem, I will drop FIXME comment here. If not, I will open a new bug for it. I think it's not a big problem but it might be related to bug 1366422, so anyway I will investigate it further.

FWIW: We already skip the case in another place [1]
[1] https://hg.mozilla.org/mozilla-central/file/a49112c7a576/dom/animation/EffectCompositor.cpp#l302
Comment on attachment 8875466 [details]
Bug 1368889 - Post animation restyle hint againt pseudo element instead of its parent.

https://reviewboard.mozilla.org/r/146904/#review151040

::: dom/animation/EffectCompositor.h:252
(Diff revision 1)
> +  // Returns the target element of restyling.
> +  // The target element is the parent of |aElement| if |aPseudoType| is ::after
> +  // or ::before, otherwise |aElement| itself.

Returns the target element for restyling.

If |aPseudoType| is ::after of ::before, returns the generated content element of which |aElement| is the parent. If |aPseudoType| is any other pseudo type (other than CSSPseudoElementType::NotPseudo) returns nullptr. Otherwise, returns |aElement|.

::: layout/base/ServoRestyleManager.cpp:86
(Diff revision 1)
>  {
> -  Servo_NoteExplicitHints(aElement, aRestyleHint, nsChangeHint(0));
> +  Element* elementToRestyle =
> +    EffectCompositor::GetElementToRestyle(aElement, aPseudoType);
> +
> +  if (!elementToRestyle) {
> +    // FIXME: When reframeing happens, EffectCompositor::mElementsToRestyle

reframing

::: layout/base/ServoRestyleManager.cpp:86
(Diff revision 1)
> +    // FIXME: When reframeing happens, EffectCompositor::mElementsToRestyle
> +    // still has unbinded old pseudo element. We should drop it.

In the Gecko case we will end up clearing out these elements in EffectCompositor::AddStyleUpdatesTo since we unconditionally empty mElementsToRestyle there.

I think we don't necessarily do that for the Servo case since PreTraverseInSubtree always checks the element is in the subtree before dropping it from mElementsToRestyle so perhaps there might be a case where we end up with elements persisting?

Perhaps at the end of PreTraverse we should add an extra step to clear mElementsToRestyle unconditionally? If that even works?

I agree that can be a separate bug, however.
Attachment #8875466 - Flags: review?(bbirtles) → review+
Thanks for the review!

(In reply to Brian Birtles (:birtles) from comment #10)
> ::: layout/base/ServoRestyleManager.cpp:86
> (Diff revision 1)
> > +    // FIXME: When reframeing happens, EffectCompositor::mElementsToRestyle
> > +    // still has unbinded old pseudo element. We should drop it.
> 
> In the Gecko case we will end up clearing out these elements in
> EffectCompositor::AddStyleUpdatesTo since we unconditionally empty
> mElementsToRestyle there.
> 
> I think we don't necessarily do that for the Servo case since
> PreTraverseInSubtree always checks the element is in the subtree before
> dropping it from mElementsToRestyle so perhaps there might be a case where
> we end up with elements persisting?
> 
> Perhaps at the end of PreTraverse we should add an extra step to clear
> mElementsToRestyle unconditionally? If that even works?

It's not yet clear to me where it happens, just clearing the element is enough to do or not. Anyway filed bug 1371107 for it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Thanks for the review!
> 
> (In reply to Brian Birtles (:birtles) from comment #10)
> > ::: layout/base/ServoRestyleManager.cpp:86
> > (Diff revision 1)
> > > +    // FIXME: When reframeing happens, EffectCompositor::mElementsToRestyle
> > > +    // still has unbinded old pseudo element. We should drop it.
> > 
> > In the Gecko case we will end up clearing out these elements in
> > EffectCompositor::AddStyleUpdatesTo since we unconditionally empty
> > mElementsToRestyle there.
> > 
> > I think we don't necessarily do that for the Servo case since
> > PreTraverseInSubtree always checks the element is in the subtree before
> > dropping it from mElementsToRestyle so perhaps there might be a case where
> > we end up with elements persisting?
> > 
> > Perhaps at the end of PreTraverse we should add an extra step to clear
> > mElementsToRestyle unconditionally? If that even works?
> 
> It's not yet clear to me where it happens, just clearing the element is
> enough to do or not. Anyway filed bug 1371107 for it.

I meant the cases causing it other than reframing.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dedcbcd21122
Post animation restyle hint againt pseudo element instead of its parent. r=birtles
https://hg.mozilla.org/mozilla-central/rev/dedcbcd21122
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: