Closed Bug 899808 Opened 6 years ago Closed 6 years ago

Pseudoelements in scoped style rules ignored

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ben, Assigned: heycam)

References

Details

Attachments

(1 file)

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 &mdash; 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&hellip;</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.
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
Attached patch patchSplinter Review
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)
We should consider adding versions of the Probe methods that take a tree match context, fwiw, in a separate bug.
(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?
...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.
Review ping?
Flags: needinfo?(dbaron)
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)
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.
https://hg.mozilla.org/mozilla-central/rev/c7f0cd7515ad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 926717
Depends on: 930270
Depends on: 927734
You need to log in before you can comment on or make changes to this bug.