315567 - Multiple :not selectors doesn't apply properly

Status: RESOLVED FIXED

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

Description

[Alfonso Martinez]

Setting a rule like
p:not([class]):not([title]) 

is applied to the same elements than

p:not([class]) 

and if I read correctly http://www.w3.org/TR/2001/CR-css3-selectors-20011113/#negation it should select elements without class and title attributes. (this is almost the first time that I try to work with this selector so I might be wrong about how it works)

tested in Firefox 1.5RC1 windows XP
Testcase coming.

Comment 1

[Alfonso Martinez]

Attachment: testcase of 3 p with several :not selectors, the last one fails.

testcase showing the bug.

Comment 2

[Anne (:annevk)]

From one of the examples it seems that subsequent :not pseudo-classes is some "OR" based matching (as you assume) but that would class with the idea of pseudo-classes in general. So probably a bug in the example (which has no normative status whatsoever).

Comment 3

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

You're misinterpreting the example; it doesn't even suggest that this is correct.  (:link and :visited are mutually exclusive.)

Comment 4

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

Attachment: patch that fixes just this bug

(Patch to get rid of the crazy way we handle :not() coming up, though...)

Comment 5

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

Attachment: patch to simplify :not() handling

Comment 6

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

Hrm, that actually breaks the event state and attribute change handling code...

Comment 7

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

Attachment: patch to simplify :not() handling

I fixed the event state and attribute thing by introducing the aDependence argument to SelectorMatches.  A also made STATE_CHECK not be a macro, for what is presumably significant codesize reduction, and removed IsEventPseudo since it served no purpose whatsoever (the only difference for what was inside it was a check specific to two of the event pseudos).

Comment 8

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

Attachment: patch to simplify :not() handling (diff -ub version of previous)

Comment 9

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

Oh, and I almost forgot to mention:  I ran through the css3-selectors test suite (20051019) version, XHTML version only.  I didn't see anything unexpected except for a bug with :empty and CDATA sections and a few bugs in the test suite I hadn't noticed before.

Comment 10

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

Attachment: patch to simplify :not() handling

A few minor tweaks, mainly removing duplication between my two added parameters to SelectorMatches (since the pointer being non-null was the same as the boolean; now a little more efficient and a little more confusing, but a little better commented too).

Comment 11

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

Attachment: patch to simplify :not() handling (diff -ub version of previous)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 202320 [details] [diff] [review]
patch to simplify :not() handling (diff -ub version of previous)

>Index: nsCSSParser.cpp
>   // ID, class and attribute selectors and pseudo-classes are stored in
>   // the first mNegations attached to a selector

This is no longer true, is it?  That is, given the selector:

  foo.x:not(.bar):not(.baz)

we will now create three nsCSSSelectors (one for foo.x, one for :not(.bar), one for :not(.baz)) where before we created two (one for foo.x, one for :not(.bar):not(.baz)).

That's probably an acceptable change (in fact, looks like it lets you do cleanup elsewhere), but we should fix the comment accordingly.

We should probably add an assert in here somewhere that every selector in mNegations only has one item of information in it, right?

>Index: nsCSSStyleRule.cpp
>@@ -602,79 +590,69 @@ void nsCSSSelector::ToStringInternal(nsA
>+    // XXXldb Or if we've written a namespace.

File a followup on that, please? Should be easy to fix.

This method should probably also assert that not more than one thing is present in the selector if aIsNegated is true.  Perhaps there should be a method on nsCSSSelector to assert this?

>Index: nsCSSStyleSheet.cpp
> static PRBool SelectorMatches(RuleProcessorData &data,

If aDependence != nsnull, this should assert that *aSelector only has one piece of data in it...

>       nsAtomList* IDList = aSelector->mIDList;
>+  if (result && aSelector->mIDList) {

You don't use IDList till much lower than this...  Want to declare it right outside the loop that actually uses it (or rather outside the |if (isCaseSensitive)|?

>+      if (!result && dependence)
>+        result = PR_TRUE;

  result = result || dependence;

might be faster... Not sure.  Both are about equally clear (and equally deserving of a comment explaining why dependence trumps result), I think.

With those nits picked, this looks great.  Getting rid of that localTrue/localFalse stuff is wonderful!

Comment 13

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

I'm actually not a big fan of littering the tree with asserts that :not() only contains a simple selector (css3 definition), since with this patch the only change that's required to allow :not() to contain a sequence of simple selectors (css3 definition) is the parser change.  The parser does enforce the current rules, and I think that's enough, because nothing now depends on them.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Actually, I think changing :not() to contain a sequence of simple selectors would need changes to SelectorMatches.  For example, consider the selector:

  :not(.foo.bar)

If we support such a thing, it should match a <div class="foo">.  But with the current (post this patch) implementation of SelectorMatches (so if only change the parser), ".foo.bar" will not match, hence the negation _will_ match.

So I still think we should have an assert in SelectorMatches, precisely in case someone changes the parser without changing SelectorMatches.

I agree that if we support parsing something like this then ToString won't need any changes either.

Comment 15

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

(In reply to comment #12)
> >Index: nsCSSStyleRule.cpp
> >@@ -602,79 +590,69 @@ void nsCSSSelector::ToStringInternal(nsA
> >+    // XXXldb Or if we've written a namespace.
> 
> File a followup on that, please? Should be easy to fix.

Filed bug 315648.

> >       nsAtomList* IDList = aSelector->mIDList;
> >+  if (result && aSelector->mIDList) {
> 
> You don't use IDList till much lower than this...  Want to declare it right
> outside the loop that actually uses it (or rather outside the |if
> (isCaseSensitive)|?

I *meant* to use it in the second line quoted; that's why I moved it.  I just didn't make that change.

> >+      if (!result && dependence)
> >+        result = PR_TRUE;
> 
>   result = result || dependence;
> 
> might be faster... Not sure.  Both are about equally clear (and equally
> deserving of a comment explaining why dependence trumps result), I think.

Yeah, I was thinking a comment would be good:

      // If the selector does match due to the dependence on aStateMask
      // or aAttribute, then we want to keep result true so that the
      // final result of SelectorMatches is true.  Doing so tells
      // StateEnumFunc or AttributeEnumFunc that there is a dependence
      // on the state or attribute.
      result = result || dependence;

I'm annoyed that C doesn't have ||=.

And in response to comment 14, which I'll shortly mid-air-collide with, :not(.foo.bar) *should* match <div class"foo"> if :not() were allowed to have sequences, since <div class="foo"> doesn't match .foo.bar .  That would be the point of extending :not().

Comment 16

[Boris Zbarsky [:bzbarsky]]

Right.  No asserts needed.  I need sleep.  ;)

Comment 17

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

Fix checked in to trunk.

Comment 18

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

Attachment: final patch as landed

Comment 19

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

Codesize savings: 1128 bytes on luna, 1531 bytes on brad.