stylo: Animation-only dirty bit not cleared on elements in display:none subtree

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

9 months ago
Created attachment 8861712 [details]
Test case

While working on SMIL I ran into an issue with display:none subtrees.

The issue is that if we have an element being animated it may cause its parent to have the animation dirty bit set on it. If we then make an ancestor display:none, during the next restyle we will skip it since in traverse_dom (or top_down_dom) we call `traversal.traverse_children` but `should_traverse_children` will return false for elements with a display:none parent. As a result we never call process_preorder/recalc_style_at on the element and the dirty bit remains. When we go to assert the tree is clean at the end of the traversal we fail because the dirty bit is still set.

The attached test case causes an assertion failure for me although I haven't applied it it to a completely clean check-out so it's possible there is something in my local working copy that triggers this.

To work around this I'm thinking of adding a check to recalc_style_at to change the part where we preprocess children to be something like:

  if traversal.should_traverse_children {
    ...(preprocess as usual)
  } else if we're in an animation-only restyle and the animation-only
    descendants dirty bit is set {
    go through descendants and clear animation-only dirty bit
  }

Hiro, does that make sense? Am I missing something?
(Assignee)

Comment 3

9 months ago
Great, thank you!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I think we should clear animation dirty bit as well as normal dirty bit
> below places;
> 
> https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/
> style/traversal.rs#l664

I'm not sure changing this part makes sense. We already unconditionally clear the animation dirty bit just above if we are in an animation restyle. What is needed is to clear it for descendants too. However it would be odd to change this to clear the animation bit on descendants but only clear the other dirty bit for the current element.

However, this part...

> https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/
> style/traversal.rs#l779

... clear_descendant_data, is sometimes called earlier in restyle_style_at and does traverse children. Fixing this (to also clear the animation-only dirty descendants bit) is enough to fix this particular test case.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> One more;
> https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/
> style/traversal.rs#l540

I don't have a test case that fails without this change but it makes sense to me to also extend this handling to the animation-only dirty descendants bit.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
Comment on attachment 8861742 [details]
Bug 1359658 - Crashtest for clearing animation-only dirty descendants bit on display:none descendants;

https://reviewboard.mozilla.org/r/133728/#review136594

Thank you for finding this and quick fix!
Attachment #8861742 - Flags: review?(hikezoe) → review+
(Assignee)

Comment 7

9 months ago
Created attachment 8861772 [details] [review]
Servo PR #16613
Comment hidden (mozreview-request)

Comment 10

9 months ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a56bcd82878
Crashtest for clearing animation-only dirty descendants bit on display:none descendants; r=hiro

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a56bcd82878
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox57: affected → ---
You need to log in before you can comment on or make changes to this bug.