1202512 - style changes can be missed when an ancestor eRestyle_SomeDescendants is recorded and a descendant is then changed

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: test

This is causing bug 1193065.

Consider this test:

  <style>
    .x .y { background-color: red; }
  </style>
  <p class=x><span><i class=y>hello</i></span></p>
  <script>
    document.querySelector("p").classList.toggle("x");
    document.querySelector("i").classList.toggle("y");
  </script>

Before bug 1180118, the toggling of .x would record an eRestyle_Subtree restyle hint for the <p>.  The toggling of .y would not record any restyle hint, since we call SelectorMatchesTree to see if the ancestors of the <i> match the left part of the selector (just |.x | in this case).  That the .y toggling did not produce a restyle thing didn't matter, since the eRestyle_Subtree from the .x toggling on the ancestor would deal with restyling the <i>.

After bug 1180118, the toggling of .x would record an eRestyle_SomeDescendants for the <p> with selector |.y|.  We only process that once we flush style, and by that time, the .y has been toggled off the <i> so we don't end up restyling it.

Comment 1

[Cameron McCormack (:heycam)]

[Tracking Requested - why for this release]:

Comment 2

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #1)
> [Tracking Requested - why for this release]:

Certain combinations of dynamic updates in a page can cause incorrect styling.

Comment 3

[Cameron McCormack (:heycam)]

One way to solve this is to restyle elements when their relevant attributes are changed even if their LHS selector (the |.x | in the test) fails to match when calling SelectorMatchesTree, as long as an ancestor generated an eRestyle_SomeDescendants.

We could do that by having hints called say eRestyle_Conditional_{Self,Subtree,LaterSiblings} and generating them from nsCSSRuleProcessor.cpp's AttributeEnumFunc when SelectorMatchesTree fails.  (eRestyle_Conditional_Subtree would not cause the element to be a restyle root.)  Then while processing the restyles, we resolve eRestyle_Conditional_{Self,Subtree,LaterSiblings} into corresponding eRestyle_{Self,Subtree,LaterSiblings} hints depending on whether eRestyle_SomeDescendants is also present.

Comment 4

[Cameron McCormack (:heycam)]

Since we'll want to uplift this to beta along with bug 1180118 to fix bug 1172239, the comment 3 approach seems a bit risky.  I'm trying something simpler: if we detect an attribute change for an element and there is an ancestor element that currently has an eRestyle_SomeDescendants restyle hint, then we just return the restyle hint we would've returned had SelectorMatchesTree not returned false.  We can keep some state bits on elements (a new pair of restyle bits) to record this so we don't have to look up the pending restyle table.  (And it's also a bit cleaner to look at element state bits from within nsCSSRuleProcessor than to give it a reference to the RestyleTracker.)

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

Comment 5

[Cameron McCormack (:heycam)]

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

Comment 6

[Cameron McCormack (:heycam)]

And I think we'll only want to uplift to Aurora, at this point, not Beta.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it.

If you think the state bits are overkill, and it's fine to just look up the mPendingRestyles table for each element we look at in SelectorMatchesTree, I can do that.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 2: Make SelectorMatchesTree take a flags argument.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 3: Restyle elements with modified attributes if we find a conditionally restyled ancestor while selector matching up the tree.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 4: Reftest.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8658532 [details] [diff] [review]
Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it.

>+  ELEMENT_IS_CONDITIONAL_RESTYLE_ANCESTOR = ELEMENT_FLAG_BIT(4),

This and the animation version need to be documented.

r=me with that

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8658533 [details] [diff] [review]
Part 2: Make SelectorMatchesTree take a flags argument.

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8658534 [details] [diff] [review]
Part 3: Restyle elements with modified attributes if we find a conditionally restyled ancestor while selector matching up the tree.

This seems ok as far as it goes, but why do we not need something similar in the HasStateDependentStyle handling?  I bet we do...

r=me assuming there's another patch to fix that (and adjust the comments in this patch accordingly).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8658535 [details] [diff] [review]
Part 4: Reftest.

r=me

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> This seems ok as far as it goes, but why do we not need something similar in
> the HasStateDependentStyle handling?  I bet we do...

Ah, of course we do.  I was thinking that state changes don't generate eRestyle_SomeDescendants hints, so we didn't need to care about it, but we need to look at anything that calls SelectorMatchesTree and assumes that ancestor state changes have already dealt with all descendants.

I'll add a patch/test for that.

Comment 16

[Cameron McCormack (:heycam)]

While looking into what kinds of restyle hints get added to animation-only restyles, I found that we should never have eRestyle_SomeDescendants hints in there.  That means we only need a single flag on Element, which also means we can avoid passing through the aForAnimationRestyle boolean through the rule processor data objects.  I'll get you to take a look at these simplifications; sorry for not realising this before putting the first patches up.

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

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it. (v2)

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 3: Restyle elements with attribute/state changes if we find a conditionally restyled ancestor while selector matching up the tree. (v2)

Comment 19

[Cameron McCormack (:heycam)]

Attachment: Part 4: Reftests. (v2)

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659073 [details] [diff] [review]
Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it. (v2)

>+  ELEMENT_TYPE_SPECIFIC_BITS_OFFSET = NODE_TYPE_SPECIFIC_BITS_OFFSET + 6

"+ 5", yes?

r=me with that fixed.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659074 [details] [diff] [review]
Part 3: Restyle elements with attribute/state changes if we find a conditionally restyled ancestor while selector matching up the tree. (v2)

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659075 [details] [diff] [review]
Part 4: Reftests. (v2)

r=me

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0903759746
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a682ef7ea95
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab69f1450786
https://hg.mozilla.org/integration/mozilla-inbound/rev/0105583c5ffd

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dc0903759746
https://hg.mozilla.org/mozilla-central/rev/9a682ef7ea95
https://hg.mozilla.org/mozilla-central/rev/ab69f1450786
https://hg.mozilla.org/mozilla-central/rev/0105583c5ffd

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8659073 [details] [diff] [review]
Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it. (v2)

(request applies to all patches on the bug)

Approval Request Comment
[Feature/regressing bug #]: bug 1180118
[User impact if declined]: incorrect styling of elements in some situations
[Describe test coverage new/current, TreeHerder]: new reftest added for the identified cases that regressed
[Risks and why]: low; the substantive change is in the last patch, and that only causes more cases to be restyled than before
[String/UUID change made/needed]: N/A

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8659073 [details] [diff] [review]
Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it. (v2)

OK, let's take it!

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/721ab475da3b
https://hg.mozilla.org/releases/mozilla-aurora/rev/99066172d2ff
https://hg.mozilla.org/releases/mozilla-aurora/rev/a8715ef5463b
https://hg.mozilla.org/releases/mozilla-aurora/rev/c50d87cbb33b