77519 - [FIXr][CSS3] incorrect matching in SelectorMatches()

Status: RESOLVED FIXED

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

Description

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

Looking back at the code of SelectorMatches for 60290, I discovered two errors in
the code. I am responsible for these errors since they come from or should
have been fixed by :not() integration

  a) an unknown pseudo-class generates result = localFalse instead of PR_FALSE

  b) negated ID and class selectors matching is incorrect, in particular in case
     of multiple negated ID and class selectors in same sequence of simple
     selectors.

Comment 1

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

Attachment: proposed patch

Comment 2

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

patch attached ; pierre and marc, can you r= and sr= please ?

Comment 3

[Marc Attinasi]

Daniel, do we now have testcases for this? I'm curious why it was not caught before.

Comment 4

[Pierre Saslawsky]

Same here.  If we didn't catch it before, my guess is that everybody got confused 
with the localTrue/localFalse vs PR_TRUE/PR_FALSE again.  It was the same problem 
last time around in bug 71647.

I would like to have testcases that go through all the paths in the code.  
Hopefully there won't be many other cases to check, so it would be nice to put 
them all under the same place, attached to this bug for instance.

Comment 5

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

Pierre is right for the first part, it comes from my localFalse/localTrue thing
again. And I apologize again.

The second one is not related directly to that. In fact I missed something in the
selection code for IDs and classes, that's all (but that's a lot).

Both had no incidence on existing stylesheets and existing testcases and that's
why it was hard to see it. To see it, you have to have a test with

   a) a non-existing pseudo-class like |:foobar| for instance

   b) multiple negated ID or classes selectors in the same sequence of simple
      selectors; for instance |p:not(.warning):not(.important)|

Comment 6

[rubydoo123]

moving to moz1.0 to get it off the un-milestoned list, move it to the 
appropriate milestone when you are ready

Comment 7

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 8

[Boris Zbarsky [:bzbarsky]]

ping?  Daniel, could you attach a testcase that fails with the current code?

Comment 9

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

At least part of this patch was superseded by bug 83616.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, issue (b) from comment 0 was resolved by bug 83616.

Issue (a) is more or less a non-issue -- the only case in which that code would
be hit is if the CSS parser messes up.  Attaching updated patch covering that
case, though.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: glazou's patch updated to tip

Comment 12

[Boris Zbarsky [:bzbarsky]]

I should just take this.

Comment 13

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

Comment on attachment 122418 [details] [diff] [review]
glazou's patch updated to tip

r=glazman

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 122418 [details] [diff] [review]
glazou's patch updated to tip

Could this please be approved for 1.4b?  This just makes sure we have saner
behavior in an error case.

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 122418 [details] [diff] [review]
glazou's patch updated to tip

a=sspitzer, but it might be too late for 1.4 beta.

Comment 16

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

Comment on attachment 122418 [details] [diff] [review]
glazou's patch updated to tip

It is too late.  Changing approval from 1.4b to 1.4.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.4 final.