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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jaredr, Assigned: dbaron)

Tracking

unspecified
mozilla1.9.3a5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
Created attachment 417624 [details]
Testcase showing incorrect style when content is added by a script
(Reporter)

Comment 2

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: nobody → dbaron
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Priority: -- → P3
Hardware: x86_64 → All
Target Milestone: --- → mozilla1.9.3a1
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.
Summary: Sibling selectors fails to account for dynamically added content in some situations → :empty + E and :empty ~ E selectors don't cause required style changes on later siblings of element that changes emptiness
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/ ?
Created attachment 417640 [details] [diff] [review]
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.
Attachment #417640 - Flags: review?(bzbarsky)
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.
(Reporter)

Comment 7

9 years ago
(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).
(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.)
> 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?
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).
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?
Created attachment 445457 [details] [diff] [review]
patch

merged to trunk
Attachment #417640 - Attachment is obsolete: true
Attachment #445457 - Flags: review?(bzbarsky)
Attachment #417640 - Flags: review?(bzbarsky)
Created attachment 445458 [details] [diff] [review]
only restyle later siblings for the 'noappend' case

Additional performance optimization, at bzbarsky's request.
Attachment #445458 - Flags: review?(bzbarsky)
Attachment #445457 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #445458 - Flags: review?(bzbarsky) → review+
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).
*And* I actually meant the parenthetical to be on the other occurrence, referring to aFollowingSibling, not aChild->GetNextSibling().
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.)
http://hg.mozilla.org/mozilla-central/rev/597dc66c8cae
http://hg.mozilla.org/mozilla-central/rev/c7eb9c5230f0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a1 → mozilla1.9.3a5
And thanks for reporting this bug.  It was a tricky one to find!
You need to log in before you can comment on or make changes to this bug.