534804 - :empty + E and :empty ~ E selectors don't cause required style changes on later siblings of element that changes emptiness

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jared Roberts]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2b6pre) Gecko/20091214 Ubuntu/9.10 (karmic) Namoroka/3.6b6pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2b6pre) Gecko/20091214 Ubuntu/9.10 (karmic) Namoroka/3.6b6pre

If I have a selector such as:
#older-sibling:not(:empty) + .test

in a document containing:
<div id="older-sibling"></div>
<div class="test">...</div>

and then I add content to the initially empty "older-sibling", the "test" div does not immediately reflect the style rule given.

Reproducible: Always

Steps to Reproduce:
I will attach a test case.
Actual Results:  
The younger sibling is not updated to the new style.

Expected Results:  
Since the older sibling is no longer empty, the style rule should apply to the younger sibling.

This bug can be masked as certain events can cause the rule to suddenly kick in-- presumably the next thing that causes styling to be recalculated.  Specifically, if I have Firebug open and I hover over the parent node of these divs (I don't even have to click!), suddenly the styling fixes itself.

Comment 1

[Jared Roberts]

Attachment: Testcase showing incorrect style when content is added by a script

Comment 2

[Jared Roberts]

The testcase also shows that the :not(:empty) part of the rule _does_ get updated properly.  I have not tried with other combinators besides the two in the testcase.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You found a testcase I didn't think of when I was fixing bug 401291.  I have a relatively straightforward (although somewhat ugly) fix at:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/59ec2da5a270/more-restyling-for-empty
but I need to write a bunch of tests before requesting review.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is it ok with you if I put your testcase into our source tree?  That is, are you willing to license it under the tri-license described at http://www.mozilla.org/MPL/ ?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I think this should be sufficient, although I should perhaps figure out why, when I revert the reordering changes in nsCSSFrameConstructor.cpp (the ones to check :empty first because it can cause more restyling), I get different test failures from the ones I was expecting.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 417640 [details] [diff] [review]
patch

>+run1();
>+styletwo.firstChild.data = "#e:nth-child(2n+1) { color: green }";
>+run1();
>+styletwo.firstChild.data = "#e:first-child { color: green }";
>+run1();
>+styletwo.firstChild.data = "#e:nth-last-child(2n+1) { color: green }";
>+run1();
>+styletwo.firstChild.data = "#e:last-child { color: green }";
>+run1();

which still happens even after I correct these selectors to be #e > span:...

It's possible there's just something in the test harness causing us to take the SLOW_SELECTOR codepath for everything.  That's not so great for testing selectors, though.

Comment 7

[Jared Roberts]

(In reply to comment #4)
> Is it ok with you if I put your testcase into our source tree?  That is, are
> you willing to license it under the tri-license described at
> http://www.mozilla.org/MPL/ ?

Yes, do whatever you want with it provided that I'm still allowed to give the testcase to other people/browsers on whatever terms they need (Webkit/chrome chokes on the same testcase in a slightly different way).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> (In reply to comment #4)
> > Is it ok with you if I put your testcase into our source tree?  That is, are
> > you willing to license it under the tri-license described at
> > http://www.mozilla.org/MPL/ ?
> 
> Yes, do whatever you want with it provided that I'm still allowed to give the
> testcase to other people/browsers on whatever terms they need (Webkit/chrome
> chokes on the same testcase in a slightly different way).

Yes, absolutely.  You still hold copyright and can license it as many ways as you want.  (Or something like that; I'm not a lawyer.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> It's possible there's just something in the test harness causing us to take the
> SLOW_SELECTOR codepath for everything. 

That would be very odd... and yes, unfortunate.  The harness test.css doesn't have anything like that obviously in it.  Is the symptom that we match when we really shouldn't?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The symptom was that when I tested the patch without the moved chunks (moving the SLOW_SELECTOR check below the EMPTY check), I got the same number of failures as without the patch (although different ones, I think).

Comment 11

[Timothy Nikkel (:tnikkel)]

Is this comment in nsCSSFrameConstructor::ContentInserted/Removed

  // XXXldb Do we need to re-resolve style to handle the CSS2 + combinator and
  // the :empty pseudo-class?

still needed?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

merged to trunk

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: only restyle later siblings for the 'noappend' case

Additional performance optimization, at bzbarsky's request.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 445457 [details] [diff] [review]
patch

You should probably null-check aContainer->GetParent() before calling AsElement() on it.  r=me with that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 445458 [details] [diff] [review]
only restyle later siblings for the 'noappend' case

>+  if (selectorFlags & NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS) {
>+    // Restyle all later siblings. (aChild might not be an element).
>+    RestyleSiblingsStartingWith(this, aChild->GetNextSibling());

Not sure what the comment is trying to say.  We could just check whether aChild is an element if we want....

r=bzbarsky with that part explained or changed.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, I somewhat mangled that comment.  I replaced it with:
    // Restyle all later siblings. (We can't use PostRestyleEvent directly
    // since aChild->GetNextSibling() might not be an element).

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*And* I actually meant the parenthetical to be on the other occurrence, referring to aFollowingSibling, not aChild->GetNextSibling().

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I'll just put:
// Needed since we can't use PostRestyleEvent on non-elements (with
// eRestyle_LaterSiblings or nsRestyleHint(eRestyle_Self |
// eRestyle_LaterSiblings) as appropriate).
above RestyleSiblingsStartingWith itself.  (The history is odd because I realized I needed to use a loop for one of the functions immediately, but didn't realize for the other until I got an nsIContent vs. Element compiler error.)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/597dc66c8cae
http://hg.mozilla.org/mozilla-central/rev/c7eb9c5230f0

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And thanks for reporting this bug.  It was a tricky one to find!