Closed Bug 1202512 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- unaffected
firefox42 + fixed
firefox43 + fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

(Keywords: regression)

Attachments

(6 files, 2 obsolete files)

Attached file 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.
[Tracking Requested - why for this release]:
(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.
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.
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
And I think we'll only want to uplift to Aurora, at this point, not Beta.
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8658532 - Flags: review?(bzbarsky)
Attached patch Part 4: Reftest.Splinter Review
Attachment #8658535 - Flags: review?(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
Attachment #8658532 - Flags: review?(bzbarsky) → review+
Comment on attachment 8658533 [details] [diff] [review]
Part 2: Make SelectorMatchesTree take a flags argument.

r=me
Attachment #8658533 - Flags: review?(bzbarsky) → review+
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).
Attachment #8658534 - Flags: review?(bzbarsky) → review+
Comment on attachment 8658535 [details] [diff] [review]
Part 4: Reftest.

r=me
Attachment #8658535 - Flags: review?(bzbarsky) → review+
(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.
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
Attachment #8659074 - Flags: review?(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.
Attachment #8659073 - Flags: review?(bzbarsky) → review+
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
Attachment #8659074 - Flags: review?(bzbarsky) → review+
Comment on attachment 8659075 [details] [diff] [review]
Part 4: Reftests. (v2)

r=me
Attachment #8659075 - Flags: review?(bzbarsky) → review+
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
Attachment #8659073 - Flags: approval-mozilla-aurora?
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!
Attachment #8659073 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: