899808 - Pseudoelements in scoped style rules ignored

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ben Henick]

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.

Comment 1

[Cameron McCormack (:heycam) (away)]

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.

Comment 2

[Cameron McCormack (:heycam) (away)]

Attachment: patch

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We should consider adding versions of the Probe methods that take a tree match context, fwiw, in a separate bug.

Comment 4

[Cameron McCormack (:heycam) (away)]

(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?

Comment 5

[Ben Henick]

...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 6

[Cameron McCormack (:heycam) (away)]

Review ping?

Comment 7

[David Baron :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.

Comment 8

[Cameron McCormack (:heycam) (away)]

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.

Comment 9

[Cameron McCormack (:heycam) (away)]

https://tbpl.mozilla.org/?tree=Try&rev=c3e9c3a926d3

Comment 10

[Cameron McCormack (:heycam) (away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f0cd7515ad

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7f0cd7515ad