Closed
Bug 899808
Opened 11 years ago
Closed 11 years ago
Pseudoelements in scoped style rules ignored
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: ben, Assigned: heycam)
References
Details
Attachments
(1 file)
8.69 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1 Steps to reproduce: <aside> <style scoped type="text/css">p::first-line { font-variant: small-caps; } </style> <p>It was in Warwick Castle that I came across the curious stranger whom I am going to talk about. He attracted me by three things: his candid simplicity, his marvelous familiarity with ancient armor, and the restfulness of his company — for he did all the talking. We fell together, as modest people will, in the tail of the herd that was being shown through, and he at once began to say things which interested me…</p> </aside> Actual results: ...Nothing. Expected results: The first line of the paragraph in the source block above should have expressed the font-variant value. When I moved that style into the head of the document, it rendered without a hitch.
Assignee | ||
Comment 1•11 years ago
|
||
The problem is that when we call ProbePseudoElementStyle under nsCSSFrameConstructor::ShouldHaveFirstLineStyle, we haven't set up TreeMatchContext.mStyleScopes with the list of scopes the element is in. So we return early from ContentEnumFunc, without matching any rules, and we then return false from ShouldHaveFirstLineStyle.
Assignee: nobody → cam
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•11 years ago
|
||
I feel like we should be calling InitAncestors in all of these nsStyleSet methods that create a new TreeMatchContext, when the element is in a style scope. But I couldn't manage to create a test that fails currently with an attribute selector or :focus.
Attachment #783494 -
Flags: review?(dbaron)
Comment 3•11 years ago
|
||
We should consider adding versions of the Probe methods that take a tree match context, fwiw, in a separate bug.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #3) > We should consider adding versions of the Probe methods that take a tree > match context, fwiw, in a separate bug. There is one already, isn't there?
Reporter | ||
Comment 5•11 years ago
|
||
...And while the ship in question has pretty much sailed, I want to apologize for not mentioning the useragent I was operating when I found the bug (as opposed to my everyday browser): Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:22.0) Gecko/20100101 Firefox/22.0 Soyeah. Oops. My bad.
Comment on attachment 783494 [details] [diff] [review] patch There ought to be assertions that fire when this bug happens. If there aren't, can you add some? (Seems like you could perhaps assert that !IsElementInStyleScope() || mAncestorFilter.mFilter in SetStyleScopeForSelectorMatching. I think that seems like the right place since it's where the dependency on having done the InitAncestors is, though it's also posssible the code could go in nsStyleSet::WalkRuleProcessors and/or in ContentEnumFunc. Worth testing that the assertion actually would have caught the bug, though.) I'm not sure that this would assert *every* time you hit the bug, for the case of two nested elements with style scopes, and starting the matching operation exactly on the inner one. But I'm also not sure if that's a problem. Then again, if it's not a big performance problem to just walk up the tree if IsElementInStyleScope and check that everything expected is in mStyleScopes, maybe it's better to do *that*, #ifdef DEBUG, in SetStyleScopeForSelectorMatching, just like AncestorFilter::AssertHasAllAncestors does. And I think it shouldn't be a big performance problem I'm not 100% convinced that the mTreeMatchContext in nsFrameConstructorState is always initialized correctly. There are some cases in nsCSSFrameConstructor where we construct an nsFrameConstructorState without calling InitAncestors right afterwards. Now that things actually depend on that for correctness in addition to performance, it might be worth restructuring that code so that the InitAncestors happens in the constructor of nsFrameConstructorState. That should be a separate patch, and it's probably also worth asking bz what he thinks about it first. (Alternatively, you could call InitAncestorsIfInStyleScope in the functions taking a TreeMatchContext from the outside, conditionally on not having an ancestor filter set up. But it seems better to be in the state where we always get the performance optimization.) As far as tests, you should be able to test the state and attribute change optimizations; not sure what you tried. But it's possible that the mochitest harness has a style sheet that interferes, and you'd need to test them inside a clean iframe. I also wouldn't pick :focus since there are :-moz-focusring styles in the UA style sheet. (HasStateDependentStyle and HasAttributeDependentStyle are used to figure out whether to restyle a subtree as a result of a state or attribute change; they examine a list of all the selectors for that state or attribute, starting from the combinator-separated piece with the state attribute and moving to the left.) It would be good to have a test for that case, but it's also probably not worth spending a huge amount of time on. r=dbaron with those three things, though the second should be a followup and the third isn't required if it turns out to be hard Sorry for taking so long to get to this.
Attachment #783494 -
Flags: review?(dbaron) → review+
Flags: needinfo?(dbaron)
Assignee | ||
Comment 8•11 years ago
|
||
I'll add the assertions looking up the tree in SetStyleScopeForSelectorMatching. For considering calling InitAncestors from the TreeMatchContext constructors, I'll file a separate bug. I managed to make a test for the attribute dependent style, but not the state dependent style. It's not clear to me why that works, but I'll leave it for now.
Assignee | ||
Comment 9•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c3e9c3a926d3
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f0cd7515ad
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c7f0cd7515ad
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•