Bobby Holley (:bholley) Reporter
	Description • 7 years ago

SMIL can animate display, but during an animation-only traversal we won't traverse unstyled elements. So we never handle them, which violates our tree invariants.

This causes panics in two tests:
dom/smil/crashtests/572938-4.svg
layout/reftests/svg/smil/anim-display-in-g-element.svg

Currently it's wallpapered over by the fact that for_reconstruct traversals erroneously traverse the entire tree unconditionally. I'm fixing that in bug 1384769, which causes the failures in the tests above.

To unblock that bug, I'm going to disable those two crashing tests (since we're really not handling them correctly anyway), and let them be fixed as followups.

Hiro has the start of a solution in bug 1384769 comment 14, so hopefully he can continue that here.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 1 • 7 years ago

I did dig into this a bit detail about the fact why we normal display property change from 'none' to other works fine. The key is a return value in element_needs_traversal() [1].  In normal traversal element_needs_traversal() returns true if the element has no style data.  That means we traverse descendant elements in display:none subtree when we change display property changed.

[1] https://hg.mozilla.org/mozilla-central/file/556f19ef392a/servo/components/style/traversal.rs#l276

Anyway, I will take an approach to create a SequentialTask and defer styling descendant elements in a subsequent normal traverse.

	Emilio Cobos Álvarez (:emilio)
	Comment 2 • 7 years ago

That'd work I guess (calling StyleNewChildren from the SequentialTask), though I don't think in theory a SequentialTask should be needed... How does this work on Gecko? Perhaps we should allow the animation traversal to restyle unstyled elements? Can you refresh my memory about why doesn't that work in the general case?

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 3 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> That'd work I guess (calling StyleNewChildren from the SequentialTask),
> though I don't think in theory a SequentialTask should be needed... How does
> this work on Gecko? Perhaps we should allow the animation traversal to
> restyle unstyled elements? Can you refresh my memory about why doesn't that
> work in the general case?

Honestly I don't know Gecko's setup in the case (it's SMIL!).  I think we can traverse unstyled elements in animation-only restyle (actually I did it a bit but did not finish it), if you are OK with that I am happy to do that!

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 4 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> > That'd work I guess (calling StyleNewChildren from the SequentialTask),
> > though I don't think in theory a SequentialTask should be needed... How does
> > this work on Gecko? Perhaps we should allow the animation traversal to
> > restyle unstyled elements? Can you refresh my memory about why doesn't that
> > work in the general case?
> 
> Honestly I don't know Gecko's setup in the case (it's SMIL!).  I think we
> can traverse unstyled elements in animation-only restyle (actually I did it
> a bit but did not finish it), if you are OK with that I am happy to do that!

I missed that the unstyled elements need selector matching to be styled correctly, that means we can't tell whether we need to trigger transition or not if other property is also changed (e.g. style attribute changes) when the display property is changed from 'none' to other. (there may be some ways to distinguish the case but it seems to me that it will result more complexity.)

So, I think now creating a SequentialTask is a reasonable way to solve this issue.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 5 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> > > That'd work I guess (calling StyleNewChildren from the SequentialTask),
> > > though I don't think in theory a SequentialTask should be needed... How does
> > > this work on Gecko? Perhaps we should allow the animation traversal to
> > > restyle unstyled elements? Can you refresh my memory about why doesn't that
> > > work in the general case?
> > 
> > Honestly I don't know Gecko's setup in the case (it's SMIL!).  I think we
> > can traverse unstyled elements in animation-only restyle (actually I did it
> > a bit but did not finish it), if you are OK with that I am happy to do that!
> 
> I missed that the unstyled elements need selector matching to be styled
> correctly, that means we can't tell whether we need to trigger transition or
> not if other property is also changed (e.g. style attribute changes) when
> the display property is changed from 'none' to other. (there may be some
> ways to distinguish the case but it seems to me that it will result more
> complexity.)

After some more thought, I start thinking that we don't need to trigger any transitions in such cases because when we change transitionable property along with display property change from 'none' to other, we don't create the transition for the transitionable property at all.  I've confirmed that chrome doesn't either.

Another concern is that we *need to* create CSS animations if descendants have animation property. Currently we create SequenTialTask for creating CSS animations during normal restyle and restyle for the new CSS animations in the second animation-only traversal, so we should create the SequentialTask during animation-only restyle, and need the second animation-only restyle even if we process flushing thrrotled animations.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 6 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> After some more thought, I start thinking that we don't need to trigger any
> transitions in such cases because when we change transitionable property
> along with display property change from 'none' to other, we don't create the
> transition for the transitionable property at all.  I've confirmed that
> chrome doesn't either.

Given that display property is animatable for CSS animation;

@keyframes anim {
  from { display: none }
  to { display: block }
}
#parent {
  animation: anim 10s;
}
#child {
  transition: all 5s;
  color: red;
}
<div id="parent"><div id="child"></div></div>

// Change the child style color at the 5s of the display property animation (i.e. changing display value to 'block'). 
child.style.color = 'blue';

Brian, in this case, should we trigger color transition for the child element?

	Brian Birtles (:birtles)
	Comment 7 • 7 years ago

'display' should not animatable by CSS animation. Servo_StyleSet_GetKeyframesForName should be explicitly filtering out the display property.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 8 • 7 years ago

I misunderstand that there is a plan to make display property animatable for CSS animation but actually it's for script animation. But anyway, on IRC, Brian told me that he suspects transition does not happen in such cases.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 9 • 7 years ago

I decided to create a SequentialTask for resolving styles for the descendants that were in display:none subtree.  The other way that traversing unstyled elements in animation-only restyle also traverse elements which are about to initially restyle in the animation-only restyle. It's hard to tell whether the element will be initially styled or just is in display:none subtree in element_needs_traversal().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bede1f8c0a04aca3b6f13bcff8e0ef17c85a4009

	Emilio Cobos Álvarez (:emilio)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8892286 [details]
Bug 1385089 - Move the logic for Servo_NoteExplicitHints into wrapper.rs.

https://reviewboard.mozilla.org/r/163222/#review168802

::: servo/components/style/gecko/wrapper.rs:677
(Diff revision 1)
>      pub fn owner_document_quirks_mode(&self) -> QuirksMode {
>          self.as_node().owner_doc().mCompatMode.into()
>      }
> +
> +    /// Only safe to call on the main thread, with exclusive access to the element and
> +    /// its ancestors.

Please, note here that this is also called after SMIL animations that animate display.

Also, that this schedules a style flush.

::: servo/components/style/gecko/wrapper.rs:679
(Diff revision 1)
>      }
> +
> +    /// Only safe to call on the main thread, with exclusive access to the element and
> +    /// its ancestors.
> +    unsafe fn maybe_restyle<'a>(&self,
> +                                data: &'a mut AtomicRefMut<ElementData>,

While you're here, just make this a `&mut ElementData`.

::: servo/components/style/gecko/wrapper.rs:703
(Diff revision 1)
> +
> +        // Ensure and return the RestyleData.
> +        Some(&mut data.restyle)
> +    }
> +
> +    /// 

missing docs.

	Emilio Cobos Álvarez (:emilio)
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8892287 [details]
Bug 1385089 - Set restyle subtree restyle hint if the element animates display style from 'none' to other.

https://reviewboard.mozilla.org/r/163224/#review168810

Looks fine over-all... It's a bit sad that we need to handle this edge case properly at all, but seems fine.

I have a few comments and suggestions, but nothing terribly hard. I'd like to hear your input before granting r+ though.

::: layout/style/ServoTypes.h:92
(Diff revision 1)
>  enum class UpdateAnimationsTasks : uint8_t {
>    CSSAnimations    = 1 << 0,
>    CSSTransitions   = 1 << 1,
>    EffectProperties = 1 << 2,
>    CascadeResults   = 1 << 3,
> +  DisplayChanged   = 1 << 4,

This isn't really needed, right?

I'm wondering if it'd be cleaner to just use another kind of sequential task. This would make he rest of the code easier to reason about.

The fact that DISPLAY_CHANGED can only appear in animation restyles, but all the others only in non-animation restyles makes it hard without needing it I think.

What do you think?

::: servo/components/style/gecko/wrapper.rs:1183
(Diff revision 1)
> +        //
> +        // Note: This should be done before borrowing style data below since
> +        // note_explicit_hints mutates the data.
> +        if tasks.intersects(DISPLAY_CHANGED) {
> +            self.note_explicit_hints(nsRestyleHint_eRestyle_Subtree,
> +                                     nsChangeHint_nsChangeHint_Empty);

I'd assert that DISPLAY_CHANGED appears only on its own, and return below.

Also, let's rename `DISPLAY_CHANGED` (super-generic) for something like: `DISPLAY_CHANGED_FROM_NONE_FROM_SMIL_ANIMATION`. Basically, this is a real one-off nasty edge case, and I'd prefer the name to be kept as such (though if we end up doing what I suggest above it wouldn't even be needed).

::: servo/components/style/matching.rs:189
(Diff revision 1)
>                has_animations)
>          })
>      }
>  
>      #[cfg(feature = "gecko")]
> +    fn update_descendants_in_display_none_subtree(&self,

This needs documentation. Also, I'd probably just pass `old_values: Option<&Arc<ComputedValues>>`, or `Option<&ComputedValues>`, even. Same for `new_values`.

Also, I'm not in love with the name of the function... maybe `handle_animation_display_change_for_smil_if_needed`?

Basically, the current name doesn't make it feel like anything related to animations, and I think it should, what do you think?

::: servo/components/style/matching.rs:195
(Diff revision 1)
> +                                                  context: &mut StyleContext<Self>,
> +                                                  old_values: &Option<Arc<ComputedValues>>,
> +                                                  new_values: &Arc<ComputedValues>) {
> +        use context::DISPLAY_CHANGED;
> +
> +        let was_display_none = old_values.as_ref().map_or(false, |old| {

`was_display_none` really lies, and means really something like: `display_changed_from_none` or something like that.

::: servo/components/style/matching.rs:203
(Diff revision 1)
> +            old_display_style == display::T::none &&
> +            new_display_style != display::T::none
> +        });
> +
> +        if was_display_none {
> +          // When display value is changed from none to other, we need

Can we assert we're an SVG element?

	Brian Birtles (:birtles)
	Comment 14 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> Comment on attachment 8892287 [details]
> ::: servo/components/style/matching.rs:203
> (Diff revision 1)
> > +            old_display_style == display::T::none &&
> > +            new_display_style != display::T::none
> > +        });
> > +
> > +        if was_display_none {
> > +          // When display value is changed from none to other, we need
> 
> Can we assert we're an SVG element?

I haven't followed this bug closely so this comment might not make sense, but SMIL can animate properties on HTML elements too.

	Emilio Cobos Álvarez (:emilio)
	Comment 15 • 7 years ago

(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #14)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> > Comment on attachment 8892287 [details]
> > ::: servo/components/style/matching.rs:203
> > (Diff revision 1)
> > > +            old_display_style == display::T::none &&
> > > +            new_display_style != display::T::none
> > > +        });
> > > +
> > > +        if was_display_none {
> > > +          // When display value is changed from none to other, we need
> > 
> > Can we assert we're an SVG element?
> 
> I haven't followed this bug closely so this comment might not make sense,
> but SMIL can animate properties on HTML elements too.

Oh, ok, then that assertion would not make sense, thanks Brian!

Could we assert somehow we have SMIL animation rules though?

	Brian Birtles (:birtles)
	Comment 16 • 7 years ago

You could use get_smil_override() I expect.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 17 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)

> ::: layout/style/ServoTypes.h:92
> (Diff revision 1)
> >  enum class UpdateAnimationsTasks : uint8_t {
> >    CSSAnimations    = 1 << 0,
> >    CSSTransitions   = 1 << 1,
> >    EffectProperties = 1 << 2,
> >    CascadeResults   = 1 << 3,
> > +  DisplayChanged   = 1 << 4,
> 
> This isn't really needed, right?

Right.

> I'm wondering if it'd be cleaner to just use another kind of sequential
> task. This would make he rest of the code easier to reason about.

Oh right. That sounds a good idea. I will add another SequentialTask (named PostAnimation?) and a new bit flags named PostAnimationTasks.
It looks pretty good!

> ::: servo/components/style/gecko/wrapper.rs:1183
> (Diff revision 1)
> > +        //
> > +        // Note: This should be done before borrowing style data below since
> > +        // note_explicit_hints mutates the data.
> > +        if tasks.intersects(DISPLAY_CHANGED) {
> > +            self.note_explicit_hints(nsRestyleHint_eRestyle_Subtree,
> > +                                     nsChangeHint_nsChangeHint_Empty);
> 
> I'd assert that DISPLAY_CHANGED appears only on its own, and return below.
> 
> Also, let's rename `DISPLAY_CHANGED` (super-generic) for something like:
> `DISPLAY_CHANGED_FROM_NONE_FROM_SMIL_ANIMATION`. Basically, this is a real
> one-off nasty edge case, and I'd prefer the name to be kept as such (though
> if we end up doing what I suggest above it wouldn't even be needed).

Sounds reasonable, DISPLAY_CHANGE_FROM_NONE_FOR_SMIL?

> ::: servo/components/style/matching.rs:189
> (Diff revision 1)
> >                has_animations)
> >          })
> >      }
> >  
> >      #[cfg(feature = "gecko")]
> > +    fn update_descendants_in_display_none_subtree(&self,
> 
> This needs documentation. Also, I'd probably just pass `old_values:
> Option<&Arc<ComputedValues>>`, or `Option<&ComputedValues>`, even. Same for
> `new_values`.
> 
> Also, I'm not in love with the name of the function... maybe
> `handle_animation_display_change_for_smil_if_needed`?

Great! Nice name.

> Basically, the current name doesn't make it feel like anything related to
> animations, and I think it should, what do you think?

I think SMIL contains the meaning of 'animation', so we can just say 'handle_display_change_for_smil_if_needed'?

> ::: servo/components/style/matching.rs:195
> (Diff revision 1)
> > +                                                  context: &mut StyleContext<Self>,
> > +                                                  old_values: &Option<Arc<ComputedValues>>,
> > +                                                  new_values: &Arc<ComputedValues>) {
> > +        use context::DISPLAY_CHANGED;
> > +
> > +        let was_display_none = old_values.as_ref().map_or(false, |old| {
> 
> `was_display_none` really lies, and means really something like:
> `display_changed_from_none` or something like that.

Nice name. Thanks!

> ::: servo/components/style/matching.rs:203
> (Diff revision 1)
> > +            old_display_style == display::T::none &&
> > +            new_display_style != display::T::none
> > +        });
> > +
> > +        if was_display_none {
> > +          // When display value is changed from none to other, we need
> 
> Can we assert we're an SVG element?

OK, I was also concerned it.  I think the best thing we can do here is that asserting the element has RESTYLE_SMIL hint.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8892287 [details]
Bug 1385089 - Set restyle subtree restyle hint if the element animates display style from 'none' to other.

https://reviewboard.mozilla.org/r/163224/#review169090

::: servo/components/style/matching.rs:212
(Diff revision 2)
> +          debug_assert!(data.restyle.hint.intersects(RESTYLE_SMIL),
> +                        "Display animation should only happen for SMIL");

To write this assert, I did pass &ElementData to this function since I have no idea to check restyle hint in the ElementData without borrowing the data from Element.  Is there any way to check restyle hints without borrowing?

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 21 • 7 years ago
mozreview-review-reply

Comment on attachment 8892287 [details]
Bug 1385089 - Set restyle subtree restyle hint if the element animates display style from 'none' to other.

https://reviewboard.mozilla.org/r/163224/#review169090

> To write this assert, I did pass &ElementData to this function since I have no idea to check restyle hint in the ElementData without borrowing the data from Element.  Is there any way to check restyle hints without borrowing?

It would be better to pass RestyleHint instead of ElementData.

	Brian Birtles (:birtles)
	Comment 23 • 7 years ago

P1 since failing to fix this could lead to panics (and since it looks like it's nearly done).

	Emilio Cobos Álvarez (:emilio)
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8892287 [details]
Bug 1385089 - Set restyle subtree restyle hint if the element animates display style from 'none' to other.

https://reviewboard.mozilla.org/r/163224/#review169118

::: servo/components/style/context.rs:506
(Diff revision 3)
>      }
> +
> +    /// Creates a task to do post-process for a given element as a result of
> +    /// animation-only restyle.
> +    #[cfg(feature = "gecko")]
> +    pub fn process_post_animation(el: E,

nit: I think this signature fits in the same line.

::: servo/components/style/dom.rs:631
(Diff revision 3)
> +    fn process_post_animation(&self, tasks: PostAnimationTasks);
> +
> +    /// In Gecko, element has a flag that represents the element may have
> +    /// any type of animations or not to bail out animation stuff early.
> +    /// Whereas Servo doesn't have such flag.
> +    fn may_have_animations(&self) -> bool { false }

Reordering this doesn't make much sense as part of this patch I think.

::: servo/components/style/gecko/wrapper.rs:1171
(Diff revision 3)
>          }
>          self.as_node().get_bool_flag(nsINode_BooleanFlag::ElementHasAnimations)
>      }
>  
> +    /// Process various tasks that are a result of animation-only restyle.
> +    fn process_post_animation(&self,

ditto

::: servo/components/style/gecko/wrapper.rs:1181
(Diff revision 3)
> +
> +        // If display style was changed from none to other, we need to resolve
> +        // the descendants in the display:none subtree. Instead of resolving
> +        // those styles in animation-only restyle, we defer it to a subsequent
> +        // normal restyle.
> +        if tasks.intersects(DISPLAY_CHANGED_FROM_NONE_FOR_SMIL) {

assert this? (or that `!tasks.is_empty()`?)

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 26 • 7 years ago

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=002eda089e044ae632ea22d9ac47af3b40e9a172

	Pulsebot
	Comment 28 • 7 years ago

Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/196347b93168
Set restyle subtree restyle hint if the element animates display style from 'none' to other. r=emilio

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/196347b93168