135141 - [RFE] implement CSS 3 indirect adjacent combinator

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

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

CSS 3 introduces a new combinator '~' for indirect adjacent sibling matching.
For instance :

   foo ~ bar

should match all bar elements following, not necessarily immediately, a foo
element in the list of their parent's children.
We should implement it.

Comment 1

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

Attachment: test case (not dynamic)

Comment 2

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

Attachment: test case (dynamic)

Comment 3

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

Attachment: patch 1.0, partial fix, not handling dynamic case

Comment 4

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

Can you convince me that there aren't any cases where this causes greedy
matching rather than nondeterministic matching?  (I haven't thought it through
yet.  Perhaps it's not that hard to demonstrate.)

Comment 5

[Bradley Baetz (:bbaetz)]

Heh. I just implemented this myself (before searching for the bug). My patch is
the same as glazou's except for whitespace issues, and I moved the
NS_IS_GREEDY_OPERATOR defintion into nsCSSStyleSheet.cpp, which is the only
place which actually uses that macro.

The dynamic case for '+' is bug 110972, and I imagine that the same fix would
fix '~', too.

dbaron: re comment #4, code later in the function (which you added for bug
24031) tests against NS_IS_GREEDY_OPERATOR, which is modified in this patch to
match '~' as well. The comment currently refers to the descendent combinator,
but it does use the macro to test (I've updated the comment in my tree). IOW, if
we DTRT for descendent selectors, I think that the patch makes us DTRT for ~.

Comment 6

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

It's clear to me that the macro would no longer be appropriate if someone came
up with a generic non-adjacent first cousin combinator.  I think it will still
work with the non-adjacent sibling combinator (indirect adjacent is a horrible
name -- the point is that the need *not* be adjacent).  I think it should still
be correct in this case, but only because I can't think of a way to break it. 
Perhaps I'll try to write a more formal proof later and see if I can...

Comment 7

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

Comment on attachment 77449 [details] [diff] [review]
patch 1.0, partial fix, not handling dynamic case

r+sr=dbaron if you fix the 80th-column violations (i.e., remove the space
inside the parentheses in the definition of NS_IS_GREEDY_OPERATOR and break
lines in nsCSSStyleSheet.cpp.)

Comment 8

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

(Note that I just fixed the dynamic issues in my patch for bug 15608.)

Comment 9

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

Attachment: patch that doesn't break 80th column

(Since I wrote the patch myself, as bbaetz did, before finding this bug, I'll
attach my version, which doesn't go past the 80th column.)

Comment 10

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

Although, actually, it's worth fixing the "there are three combinators" comment
in nsICSSStyleRule.h.

Comment 11

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

Attachment: patch with comments

Comment 12

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

Fix checked in to trunk, 2004-01-14 12:12 -0800.

I also took bbaetz's suggestion in comment 5 of moving the macro to the .cpp file.

Comment 13

[Anne (:annevk)]

-> VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040116

Comment 14

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

(In reply to comment #5)
> IOW, if
> we DTRT for descendent selectors, I think that the patch makes us DTRT for ~.

(In reply to comment #6)
> I think it should still
> be correct in this case, but only because I can't think of a way to break it. 

We were wrong.  See bug 420814.