Closed
Bug 1222226
Opened 9 years ago
Closed 9 years ago
CSS class applied to wrong elements
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: evilpie, Assigned: heycam)
References
Details
(Keywords: dev-doc-complete, regression, site-compat)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
mpotharaju
:
approval‑mozilla‑b2g44+
|
Details | Diff | Splinter Review |
Reported here: https://www.reddit.com/r/firefox/comments/3roday/firefox_42_dom_ready_issue/
Boris thinks this might be caused by bug 1180118.
Comment 1•9 years ago
|
||
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=993384a9dc2ea2af1c86d841ed78698ebc028dea&tochange=25b33fb7dc51
Regressed by: Bug 1180120
status-firefox41:
--- → unaffected
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
Flags: needinfo?(cam)
Keywords: regression
Updated•9 years ago
|
Attachment #8683912 -
Attachment filename: file_1222226.txt → file_1222226.html
Attachment #8683912 -
Attachment mime type: text/plain → text/html
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
(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...
Assignee | ||
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
(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)
Comment 7•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8684049 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/css-classes-applied-to-wrong-elements/
tracking-firefox42:
--- → ?
tracking-firefox43:
--- → ?
tracking-firefox44:
--- → ?
tracking-firefox45:
--- → ?
Keywords: dev-doc-complete,
site-compat
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Attachment #8684049 -
Flags: approval‑mozilla‑b2g44?
Comment 12•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 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 16•9 years ago
|
||
bugherder uplift |
Comment 17•9 years ago
|
||
bugherder uplift |
Comment 18•9 years ago
|
||
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.
Comment 20•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
Comment 22•9 years ago
|
||
42 is gone.
You need to log in
before you can comment on or make changes to this bug.
Description
•