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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

({regression})

Trunk
mozilla43
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ fixed, firefox43+ fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8657959 [details]
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.
(Assignee)

Comment 1

3 years ago
[Tracking Requested - why for this release]:
Blocks: 1180118
status-firefox41: --- → unaffected
status-firefox42: --- → affected
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
Keywords: regression
(Assignee)

Comment 2

3 years ago
(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.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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
(Assignee)

Comment 6

3 years ago
And I think we'll only want to uplift to Aurora, at this point, not Beta.
(Assignee)

Comment 7

3 years ago
Created attachment 8658532 [details] [diff] [review]
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8658532 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

3 years ago
Created attachment 8658533 [details] [diff] [review]
Part 2: Make SelectorMatchesTree take a flags argument.
Attachment #8658533 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

3 years ago
Created 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.
Attachment #8658534 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

3 years ago
Created attachment 8658535 [details] [diff] [review]
Part 4: Reftest.
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+
tracking-firefox42: ? → +
tracking-firefox43: ? → +
(Assignee)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
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
(Assignee)

Comment 17

3 years ago
Created attachment 8659073 [details] [diff] [review]
Part 1: Add Element flags to record whether an eRestyle_SomeDescendants restyle is pending for it. (v2)
Attachment #8658532 - Attachment is obsolete: true
Attachment #8659073 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

3 years ago
Created 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)
Attachment #8658534 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
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+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 25

3 years ago
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.