73586 - matching of :first-child, :only-child, and :last-child not dynamically updated [SELECTORS-DYNAMIC]

Status: RESOLVED FIXED

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

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

the attached testcase offers two links. Clicking on the first one will insert a
paragraph as first child of body. That paragraph should get the style from

  p:first-child { border : thick green solid }

A second link appends a paragraph to the children of body and the following rule
applies to it :

  p:last-child { border : thick red solid }

*** NOTE : this last rule only running if bug 42916 checked in ***

Inserting new paragraphs at the beginning or at the end of body should make the
paragraph who had the green or red border loose that border in favor of the
newly created one. It does no work.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: DHTML test case

Comment 2

[James Lariviere]

I think dan means bug 46916 (last-child) not bug 42916?

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

yes, 46916 ; sorry.

Comment 4

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

I don't see how this has anything to do with reflow.  Isn't it just that we
don't do enough dynamic re-styling when the content model is changed?  As we
add new selectors, we're probably going to run into more problems like this
unless we do more re-resolution.  (I wrote a comment to myself in
nsCSSFrameConstructor::ContentInserted that it may not re-resolve enough for
the CSS2 '+' combinator, although I never had a chance to test that.  Would
that be the right place to do it?  How much of a performance cost would such
changes be?  What optimizations could we make to avoid such a cost (particularly
for :subject)?)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

related to bug 98997

Comment 6

[Hixie (not reading bugmail)]

The "+" issue is bug 110972.

Comment 7

[Kevin McCluskey (gone)]

Bulk moving Moz1.2 bugs to future-P2. I will pull from this list when scheduling
work post Mozilla1.0

Comment 8

[Madhur Bhatia]

cc'ing myself

Comment 9

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

Assigning pierre's remaining Style System-related bugs to myself.

Comment 10

[Boris Zbarsky [:bzbarsky]]

*** Bug 192151 has been marked as a duplicate of this bug. ***

Comment 11

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

Bug 73586, bug 98997, and bug 229915 would be fixed by what I described in step
3 of bug 15608 comment 28:

  When adding/removing content, just reresolve style on the parent (to handle
  empty, +, and all the new CSS3 variants).  (Perhaps there are good ways to
  optimize this, but it might just not be that bad.)

Comment 12

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

The problem is that that would be O(N^2) for long pages during load.

A more detailed approach might look something like this:
 * maintain a hashtable in nsCSSStyleSheet.cpp much like the one used for event
state selectors or attribute selectors, hashing all selectors with any of these
"child-counting" selectors (in which I include :empty).  There would also be a
global variable(s) to indicate whether any nth-last, last-of-type, or
nth-last-of-type were used.  This would probably allow for the necessary
optimizations.  (Consider that we use :-moz-last-node extensively in quirk.css,
and this actually leads to real bugs due to this bug.)

I should probably describe this in a little more detail at some point, or just
implement it.

It would be pretty easy to fix bug 73586 with this type of approach, but bug
229915 would probably be a little more work.  I need to think about that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Clearer first-child testcase

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Clearer last-child testcase

Comment 15

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

Some other approaches might be feasible now that
nsIDocumentObserver::ContentReplaced no longer exists (which simplifies the
invariants for observer code).

Comment 16

[Boris Zbarsky [:bzbarsky]]

*** Bug 254598 has been marked as a duplicate of this bug. ***

Comment 17

[bugzilla]

I was just wondering how this bug is being fixed.  Is it planned to be in for
Firefox 1.1?  Or is it for a latter date?

Comment 18

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

*** Bug 303775 has been marked as a duplicate of this bug. ***

Comment 19

[Arpad Borsos [:Swatinem]]

This bug also applies to :only-child. Could someone change the summary accordingly?

Comment 20

[Justin Wood (:Callek)]

boris/david, should I file a new bug re: c#19; or morph this one to include it, I am unsure exactly how similar the issue is in your mind(s).

Comment 21

[Timwi]

This bug affects not only pages that change dynamically; it also affects pages that are simply slow to load. The :last-child CSS rule is (wrongly) applied to all elements that were the last child at some point in the middle of rendering an unfinished page, even if additional elements followed it later.

Comment 22

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

So I think I have a strategy for fixing this bug, bug 98997, and bug 229915.  We should set bits on nsINode (which means we need to mark some more nsINode flags an not reserved for implementations) that mean:
 1. element has an :empty or :-moz-only-whitespace selector
 2. arbitrary operations on children of the element (including append) require a reresolve on the element
 3. append requires a reresolve on the element's last child; other operations on children of the element require a reresolve of the element
 4. append requires no changes, but other operations on children of the element require a reresolve of the element

I'm not sure if we really need the distinction between (3) and (4).  We need (1) separate because of the pile of :-moz-only-whitespace rules in quirk.css -- this would be much easier if we moved that margin collapsing quirk into C++.

SelectorMatches and SelectorMatchesTree would then set these bits *any* time they tried to match one of the problematic selectors (match or no).  This will guarantee that we'll always have them set if the change might matter.  We should move the problematic selectors to the end of SelectorMatches so they get set less often (from least to most problematic).

Comment 23

[Timwi]

Correct me if I'm wrong, but the difference between (3) and (4) appears to be that under (4), :last-child would still be broken.

Comment 24

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

:last-child would set (3).  If we didn't make the distinction, there would only be (3) and not (4).

Comment 26

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

Fix by checkin of bug 401291 to trunk, 2008-02-18 22:17 -0800.

I included tests for this bug in that landing.

Unfortunately when I mentioned this bug in the checkin comment, I cited the wrong bug number (bug 75386) in the checkin comment.