Closed Bug 1222226 Opened 4 years ago Closed 4 years ago

CSS class applied to wrong elements

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- unaffected
firefox42 + wontfix
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed
b2g-v2.5 --- fixed

People

(Reporter: evilpie, Assigned: heycam)

References

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(2 files)

Attached file test case
Reported here: https://www.reddit.com/r/firefox/comments/3roday/firefox_42_dom_ready_issue/

Boris thinks this might be caused by bug 1180118.
Attachment #8683912 - Attachment filename: file_1222226.txt → file_1222226.html
Attachment #8683912 - Attachment mime type: text/plain → text/html
Assignee: nobody → cam
Status: NEW → ASSIGNED
The conditions we're checking to see if it's safe to return eRestyleResult_StopWithStyleChange are insufficient.

We check that CalcStyleDifference() told us that only reset structs changed, and assume that means inherited style can't have changed, and that if we know that no descendants have explicitly inherited reset properties, then it is safe not to restyle them.

The frames that we're restyling are two of the <span>s.  We don't have a Color struct cached on their style contexts (we're restyling before we build display lists, so we won't have looked up Color structs yet, and in any case the <span>s themselves doesn't need a Color, their child nsTextFrames do).

In this case, all four <span>s frames share the same style context, which means that when we move the child style context (there's only one, as the nsTextFrames also share the same style context), we'll resolve the same red Color struct for all four of them.

So I think we could either (a) prevent eRestyleResult_StopWithStyleChange from being returned when oldContext is shared, or (b) allow eRestyleResult_StopWithStyleChange for shared style contexts but only if we know that there is no inherited style difference between the old and new style contexts.

(a) is easy and we should maybe do that to begin with, but we should consider whether (b) is feasible.  We could compare the old and new style contexts' rule nodes -- if we find their nearest common ancestor, and didn't find any inherited properties on the rule nodes on those two branches (excluding the common ancestor itself), then we know we're safe.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #2)
> We could compare the old and new style
> contexts' rule nodes -- if we find their nearest common ancestor, and didn't
> find any inherited properties on the rule nodes on those two branches
> (excluding the common ancestor itself), then we know we're safe.

We have mStyleBits on nsCSSCompressedDataBlock, so for StyleRules it would be easy to look that up, but we'd need to add something to other types of nsIStyleRules to store which structs' properties they can have...
Attached patch patchSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5537781be9ca

Let's fault out when we find the old context is shared, and work out how to do the better thing in another bug.
Attachment #8684049 - Flags: review?(dbaron)
So is the idea here that the tradeoff we're making is that in order to be able to reparent *children* that are shared, we can't use stopWithStyleChange on a parent that's shared?  (We can reparent children that are shared, right?)

How does this affect the effectiveness of the original optimization?
Flags: needinfo?(cam)
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #5)
> So is the idea here that the tradeoff we're making is that in order to be
> able to reparent *children* that are shared, we can't use
> stopWithStyleChange on a parent that's shared?  (We can reparent children
> that are shared, right?)

I'm not exactly sure what you mean about the tradeoff -- what's the other choice?  But yes if the parent is unshared and the children are shared, you will be able to reparent them all (because you're sure that it is correct for all of them to move).

> How does this affect the effectiveness of the original optimization?

It does weaken it; shared style contexts are pretty common.  So I think it does make sense as a followup to try and restore it with a stronger check for whether we have changed inherited data.  I figured it'd be better to get things correct first, though, since it wasn't immediately straightforward to do option (b).
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #5)
> > So is the idea here that the tradeoff we're making is that in order to be
> > able to reparent *children* that are shared, we can't use
> > stopWithStyleChange on a parent that's shared?  (We can reparent children
> > that are shared, right?)
> 
> I'm not exactly sure what you mean about the tradeoff -- what's the other
> choice?  But yes if the parent is unshared and the children are shared, you
> will be able to reparent them all (because you're sure that it is correct
> for all of them to move).

I think this would be ok if we forbade reparenting of shared children -- but I think reparenting of shared children is even more important than this, so it feels like the right tradeoff to me.
Attachment #8684049 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #7)
> I think this would be ok if we forbade reparenting of shared children -- but
> I think reparenting of shared children is even more important than this, so
> it feels like the right tradeoff to me.

I see, and I agree.
Comment on attachment 8684049 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1180120
[User impact if declined]: incorrect styling of elements if restyled after frame construction but before display list construction
[Describe test coverage new/current, TreeHerder]: try run passed; just landed on inbound
[Risks and why]: low, this just disables an optimisation in certain situations
[String/UUID change made/needed]: N/A
Attachment #8684049 - Flags: approval-mozilla-beta?
Attachment #8684049 - Flags: approval-mozilla-aurora?
Attachment #8684049 - Flags: approval‑mozilla‑b2g44?
https://hg.mozilla.org/mozilla-central/rev/0733342d8c56
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8684049 [details] [diff] [review]
patch

Patch is a one-liner fix that has been in Nightly for 2 days and includes automated reftests. Let's uplift to Aurora44.
Attachment #8684049 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Recent regression in 42, since 41 shipped. Tracked for 44 to make sure we don't re-regress.
Comment on attachment 8684049 [details] [diff] [review]
patch

Low risk, has tests, let's take this for beta also.
Attachment #8684049 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8684049 [details] [diff] [review]
patch

Approved for 2.5 uplift. Seems to have landed in Aurora and Beta too.
Attachment #8684049 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Belatedly adding tracking flags since this was a regression - in case it reopens.
Duplicate of this bug: 1228791
You need to log in before you can comment on or make changes to this bug.