Closed Bug 1057129 Opened 10 years ago Closed 10 years ago

post animation restyles directly to :before/:after pseudo-elements

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(3 files)

patch 1 - Make AddStyleUpdatesTo handle pseudo-elements
5.62 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 2 - Post all animation restyles directly to pseudo-elements
1.62 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 3 - Post restyles from CheckAnimationRule directly to pseudo-elements
12.11 KB, patch
birtles
: review+
Details | Diff | Splinter Review 
One thing I need to do for bug 960465 is post animation restyles directly to pseudo-elements.  This is needed because bug 960465 depends on using the mechanism of updating only animation styles without touching other style (added in bug 996796) for making transitions start correctly.

This wasn't needed in bug 996796 because we don't run OMT animations on pseudo-elements.  However, we do need it for the general transitions case because we run transitions on pseudo-elements.
This (like patch 2) posts restyles directly to the pseudo-element
content nodes, which is a new thing.

This isn't needed right now since AddStyleUpdatesTo is currently only
used when updating main-thread-suppressed animations running on the
compositor.  However, it will be needed once we depend on
AddStyleUpdatesTo for bug 960465.  And it will have an effect now since
AddStyleUpdatesTo actually adds all animations rather than only the ones
that are suppressed from running on the main thread.
Attachment #8477188 - Flags: review?(birtles)
This (like patch 1) posts restyles directly to the pseudo-element
content nodes, which is a new thing as of this bug.  Previously we'd
have posted eRestyle_Subtree restyles to the pseudo element's real
element (i.e., the parent of the pseudo-element content node).

This changes the way we post animation restyles for ::before and ::after
pseudo-elements with animations on them.
Attachment #8477189 - Flags: review?(birtles)
This matches patch 2, and also fixes an incorrect use of eRestyle_Self
on the parents pseudo-elements in order to restyle those
pseudo-elements, where it would not previously have been effective.

This should all be temporary, since this code can go away with bug
960465, when animation phases are removed.
Attachment #8477190 - Flags: review?(birtles)
The background here is that the ::before and ::after pseudo-elements create an extra first-child or last-child frame of an element (potentially with multiple children, representing all the values of the 'content' property).  We actually create a fake dom node for that frame. This patch targets restyles at that fake dom node.  (I though I'd need to do work to make that legal in bug 1056917, but I couldn't find anything that needed to be done.)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #5)
> on the parents pseudo-elements in order to restyle those

oops, "parents pseudo-elements" -> "parents of pseudo-elements"
Comment on attachment 8477188 [details] [diff] [review]
patch 1 - Make AddStyleUpdatesTo handle pseudo-elements

Review of attachment 8477188 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles

::: layout/style/AnimationCommon.h
@@ +263,5 @@
>      }
>    }
>  
> +  mozilla::dom::Element* ElementToRestyle() const;
> +

Since this can return nullptr, should it be called GetElementToRestyle?
Attachment #8477188 - Flags: review?(birtles) → review+
Comment on attachment 8477189 [details] [diff] [review]
patch 2 - Post all animation restyles directly to pseudo-elements

Review of attachment 8477189 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles
Attachment #8477189 - Flags: review?(birtles) → review+
Comment on attachment 8477190 [details] [diff] [review]
patch 3 - Post restyles from CheckAnimationRule directly to pseudo-elements

Review of attachment 8477190 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles if my understanding of the changes to RestyleManager.cpp is correct and given any additional explanation you think is helpful.

::: layout/base/RestyleManager.cpp
@@ +2115,3 @@
>      newContext = mPresContext->StyleSet()->
> +                   ReparentStyleContext(oldContext, newParentContext, element,
> +                                        pseudoElement);

I don't know if I've quite understood the assumptions behind this.

Here's what I understand:

* The API for ReparentStyleContext described in nsStyleSet.h in this patch expects to get either:

(a) element, element; or
(b) pseudoElementParent, pseudoElement

(Although I have a question later about that.)

* If if oldContext->GetPseudoType() returns nsCSSPseudoElements::ePseudo_NotPseudoElement, then ElementForStyleContext will give us aFrame->GetContent()->AsElement() and we'll end up with:

    element = pseudoElement = aFrame->GetContent()->AsElement()

* oldContext->GetPseudoType() never returns ePseudo_AnonBox, ePseudo_firstLetter, ePseudo_mozColorSwatch, ePseudo_mozNumber* etc. >> Is that right? <<

* Otherwise, if parentFrame->GetContent() is non-null (as is the case for pseudo-elements?), ElementForStyleContext will give us parentFrame->GetContent()->AsElement() and we'll end up with:

    element = parentFrame->GetContent()->AsElement()
    pseudoElement = aFrame->GetContent()->AsElement()

* The final case that parentFrame is null and oldContext->GetPseudoType() is not one of the above, never occurs. >> Is that right? <<

So that seems to be correct but can you confirm the assumptions above?

If so, I wonder if you think this needs explanation? I found the |pseudoElementContent| and |pseudoElement| names surprising if these are expected to be non-null for frames that don't correspond to pseudo-elements.

::: layout/style/nsStyleSet.h
@@ +227,5 @@
>    already_AddRefed<nsStyleContext>
>    ReparentStyleContext(nsStyleContext* aStyleContext,
>                         nsStyleContext* aNewParentContext,
> +                       mozilla::dom::Element* aElement,
> +                       mozilla::dom::Element* aElementOrPseudoElement);

This seems surprising to pass in the element twice in the common (non pseudo-element) case. Would it be better to make the fourth argument just |aPseudoElement| and let it be nullptr for the non-pseudo element case?
Attachment #8477190 - Flags: review?(birtles) → review+
(In reply to Brian Birtles (:birtles, travelling 20~30 Aug, may be slower to respond) from comment #10)
> Here's what I understand:
> 
> * The API for ReparentStyleContext described in nsStyleSet.h in this patch
> expects to get either:
> 
> (a) element, element; or
> (b) pseudoElementParent, pseudoElement
> 
> (Although I have a question later about that.)

Roughly.  For pseudo-elements it expects to get the element stylistically associated with the pseudo element and the element for the pseudo-element frame (although only some pseudo-elements have such a fakelement; for others it will be the same as element).

However, the new argument (b) actually only matters for things that we run animations on, which is elements, and ::before and ::after pseudo-elements.

(Also, this is all pretty temporary; I already put the patch to remove it in my patch queue.  I just need this and at least one other temporary patch to avoid having a pile of ~50 patches with cyclic dependencies between them in order to get bug 960465 done.)

> * If if oldContext->GetPseudoType() returns
> nsCSSPseudoElements::ePseudo_NotPseudoElement, then ElementForStyleContext
> will give us aFrame->GetContent()->AsElement() and we'll end up with:
> 
>     element = pseudoElement = aFrame->GetContent()->AsElement()

Yes.

> * oldContext->GetPseudoType() never returns ePseudo_AnonBox,
> ePseudo_firstLetter, ePseudo_mozColorSwatch, ePseudo_mozNumber* etc. >> Is
> that right? <<

It's not that it never happens, but that we never do anything with the result if it does.

> * Otherwise, if parentFrame->GetContent() is non-null (as is the case for
> pseudo-elements?), ElementForStyleContext will give us
> parentFrame->GetContent()->AsElement() and we'll end up with:
> 
>     element = parentFrame->GetContent()->AsElement()
>     pseudoElement = aFrame->GetContent()->AsElement()

This is certainly what should happen for ::before and ::after.

> * The final case that parentFrame is null and oldContext->GetPseudoType() is
> not one of the above, never occurs. >> Is that right? <<

The root frame is always the same type.  I'm not sure if its pseudo-type is NotPseudoElement or AnonBox, but I don't think it matters since I don't think we can run animations on it.

> If so, I wonder if you think this needs explanation? I found the
> |pseudoElementContent| and |pseudoElement| names surprising if these are
> expected to be non-null for frames that don't correspond to pseudo-elements.

I guess they could be elementOrPseudoElementContent, etc., but that's quite long.

> >    already_AddRefed<nsStyleContext>
> >    ReparentStyleContext(nsStyleContext* aStyleContext,
> >                         nsStyleContext* aNewParentContext,
> > +                       mozilla::dom::Element* aElement,
> > +                       mozilla::dom::Element* aElementOrPseudoElement);
> 
> This seems surprising to pass in the element twice in the common (non
> pseudo-element) case. Would it be better to make the fourth argument just
> |aPseudoElement| and let it be nullptr for the non-pseudo element case?

That would make the code more complicated at both the caller and the implementation, so I'd prefer not to.
I should perhaps add some comments discouraging people from using the new parameter for anything other than its current use.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: