71647 - [CSS3] RFE : add support for the new :not() pseudo-class

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

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

Add support for the new :not() pseudo-class. This support is needed for
some very specific bugs in the editor hard to resolve without :not().

Comment 1

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

accepting bug

Comment 2

[rubydoo123]

I believe Daniel is working on this now -- setting to moz0.9

Comment 3

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

Attachment: Patch for 71647

Comment 4

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

Patch proposed for this bug. Seems to work absolutely fine. I added the rules
fixing bug 57686 to my own build and the pleasure to see that :not() thing
solve that editor's problem.

I added comments in the code to make it as understandable as I could.

David Hyatt: could you please specifically check ParsePseudoSelector and verify
your code needed for XUL is still ok ? Thanks a lot.

Comment 5

[Peter Van der Beken [:peterv]]

Small nit but this seems wrong
@@ -1593,7 +1607,7 @@
       else {
         REPORT_UNEXPECTED_TOKEN(
           NS_LITERAL_STRING("Expected element name or '*' but found"));
-        UngetToken();
+        UngetToken()
         aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
         return;
       }

Oh, and don't delete the BIDI stuff ;-).

Comment 6

[timeless]

ARG!! MERGE WARNING. It looks like you're backing out IBM BIDI. DON'T DO THAT.

+                              PRInt8 aNegationIndex
If you can't justify 8bit numbers then you shouldn't use them.
Is it meaningful for aNegationIndex to be < 0, it looks like you use 0 and 
aNegationIndex++, which is hardly justification for a signed 8 bit number. [i'm 
suggesting PRUInt]
+  PRBool  localFalse = PRBool(0 < aNegationIndex);
^ Do you need PRBool(), you don't use it below.
+  PRBool  localTrue = !localFalse;
+  PRBool  checkType = (0 == aNegationIndex) || (1 < aNegationIndex);
^ this is wierd i think i would prefer this:
+  PRBool  checkType = localFalse && (1 != aNegationIndex);

/* intermediary code unreviewed, 
i need to pull it up in a side by side compare */

nsIContent* firstChild = nsnull;
nsIContent* parent = data.mParentContent;
if (parent) {
 PRInt32 index = -1;
// ^ yuck,
// PRUInt32 index = 0; // <- better
 do {
  parent->ChildAt(++index, firstChild);
// parent->ChildAt(index++, firstChild); // <- equivalent after taxes ^^
  if (firstChild) { 
   if (IsSignificantChild(firstChild, 
    (nsCSSAtoms::firstNodePseudo == pseudoClass->mAtom))) {
     break;
   }
   NS_RELEASE(firstChild);
  }
  else {
   break;
  }
 } while (1 == 1);
}
1==1 is bad. for now please use |while (1)|, unless you can rewrite this loop 
in some saner fassion.

+        if (data.mParentContent) {
+          result = localFalse;
+        }
+        else {
+          result = localTrue;
+        }
result = (data.mParentContent)?localFalse:localTrue;

...

+            result = PRBool(localTrue == (0 != (data.mEventState & 
NS_EVENT_STATE_ACTIVE)));
/*style{please consider}*/
result = (localTrue == !(data.mEventState & NS_EVENT_STATE_ACTIVE));


+      if (nsnull == aSelector.mNegations) {
+      while (nsnull != negations->mNegations) {
/*general,style{please consider}*/
+      if (!aSelector.mNegations) {
+      while (negations->mNegations) {

are these strings going to be seen by real users?
+    REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Missing argument in negation 
pseudo-class"));

@@ -342,7 +351,8 @@
-
+  NS_IF_DELETE(mNegations);
+  
please try to do avoid adding spaces on newlines, the above should be just one 
insertion [ie:
+  NS_IF_DELETE(mNegations);

no, this should not be considered a complete code review, as I skipped a major 
portion of your code. more later (or possibly from someone else).

Comment 7

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

Peterv : this missing ";" does not appear in my local rcs archive and I correctly
compiled and run the app... It seems that it is a typo, I mean that I accidentally
hit the backspace key _after_ the end of my code and save all files when I left
my source editor !

And I yes, I saw the BIDI stuff doing my cvs diff.

Thanks a lot for you excellent review !!!

Comment 8

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

timeless : you are making a review of my additions but also of portions of code
that are not mine, that I did not touch. The coding style of the css parser and
of the selection mechanism in SelectorMatches is kind of special : it is
designed for efficiency and sometimes less for readability or correctness.
SelectorMatches is, clue given by Marc, called **hundreds of millions of
times**. I chose to use exactly the coding style of the portions of code around
because of the very high sensitivity of the style engine in the app.

About BIDI : yes, I am never checking in without updating first...
About PRInt32 : this choice discussed with Marc Attinasi last week in SD
About PRBool(..) : homogeneity with code around
About (1==1) : not my code !
About CSS error reporting : not my code !

Comment 9

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

Attachment: new patch from last versions

Comment 10

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

Last patch above produced after merging with last versions. Safe for BIDI and
XUL :-) Tested and working on Linux and Windows.

r= and sr= ?

Comment 11

[David Hyatt]

Daniel.  If you look in your source tree at

mozilla/layout/xul/base/src/outliner.xul

just load it in the browser window.  Click on the "Attach View" button at the
bottom of the screen.  Select a few outliner items.  

If this all works, then everything's ok. :)

Comment 12

[David Hyatt]

sr=hyatt

Comment 13

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

Confirming that outliner.xul still ok with patch in :-)))

Comment 14

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

ok, so here it is the thing : a new member mNegations is added to the structure
of selectors. It contains a chain of negated selectors :

  * negated classes, IDs, attributes and pseudo-classes will be stored
    in the first selector in the chain of mNegations (eventually empty but
    always present if there is at least 1 negated simple selector)
  * type element selectors and universal selectors will be individually
    stored after the first selector in the chain of mNegations. This is
    necessary due to the impossibility to make the difference between a
    universal selector and the lack of universal selector...
  * pseudo-elements cannot be negated

I had at least two others options but decided to give precedence to
simplicity and absence of perf impact on existing stylesheets w/o negated
selectors.

The parsing makes no problem and should be very easily understood. It relies
on my split of ParseSelector described by bug 71100.

Negated selectors are not hashed because :not(p) selects all elements BUT
paragraphs. It makes then no sense to hash based on the tag name !

SelectorMatches has been changed in a very simple way : PR_TRUE and PR_FALSE
are switched in the case of a negated selector. Then each test is compared
to |localTrue| or |localFalse| in order to get the std or negated result.
If the negated selector is the first one in the chain of mNegations, we must not 
look at tag and element namespace (cf. supra).

Pierre, can you r= please ? Composer's team would like to check in as soon as 
the tree reopens. Thanks.

Comment 15

[Pierre Saslawsky]

This line:

  PRBool  localTrue = !localFalse;

should be changed to

  PRBool localTrue = (localFalse ? PR_FALSE : PR_TRUE);

because we compare localTrue to the result of logical expressions as in:

  if (localTrue == ((nsnull != aSelector->mTag) &&
     (aSelector->mTag != data.mContentTag)))

and compilers usually return 0x0001 when the expression is true, which means that 
the result of the test will be false when we expect true because with the current 
code, localTrue contains 0xFFFF when localFalse is 0x0000.

Even after making the change above, I recommend verifying all the tests where we 
compare either localTrue or localFalse with the result of a function, to make 
sure that these functions return a PRBool and not an integer (like the offset of 
a string inside another one, for instance).

Another possibility would be to change all the tests and cast all the expressions 
with bool() as in:

  if (bool(localTrue) == bool(whatever))...

or:

  result = PRBool(bool(localTrue) == bool(whatever));

Note in that case that we should be using "bool()", not PRBool(), to cast both 
sides of the expressions.

Comment 16

[Brendan Eich [:brendan]]

Pierre: !0 == 1 and !1 == 0, (42==42) == 1, and (42!=42) == 0 -- all guaranteed
by the C and C++ standards.  Correct compilers able to deal with any real world
code do not "usually" conform to these parts of the standard -- they *always*
conform.

I can't understand your comment unless you were confusing ! for ~, and only then
(in light of your 0xFFFF comment) if you were thinking of 16-bit int systems
(Windows 3.1, e.g.), which Mozilla does not support.

PRBool's legal values are 0 and 1, it is an int typedef.  It's true that there
might be bugs where int-returning functions assign 42, or some other int value
than 0 or 1, into a PRBool, but I see no such calls in the patched function. 
What's more, I've never seen a bug in Mozilla due to lack of PRBool vs. int type
safety -- not to say it couldn't happen, but it's red herring here.

Using ?: is bad style (verbose, needlessly complex) and may cause weak compilers
to generate needless branches.

Mozilla code should not use bool for portability reasons, AFAIK.  Cc'ing scc.

/be

Comment 17

[Hixie (not reading bugmail)]

Just for the record, since this is not yet at CR, we should be implementing
this as :-moz-not(), but since it will hit CR within a few days, I really won't
bother asking that in reality. :-)

Comment 18

[Hixie (not reading bugmail)]

Daniel: Could you please try out the tests in
   http://www.hixie.ch/tests/adhoc/css/selectors/not/
...with your patch to make sure they all go green? Cheers.

Comment 19

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

Ok, I had plenty of red on Ian's test 001.xml and we were together wondering
why... Diving into gdb, I think that we have a bug in the namespace
parsing and storage in CSS. Please note that it is not my bug. I never touched
it in the current proposed patch nor in 71100.

Explanation : given the sheet

    @namespace html url(http://www.w3.org/1999/xhtml);
    @namespace test url(http://www.example.org/);
    testA  { color: green; }

where testA is an element in the namespace http://www.example.org/, our CSS
parser generates the following type element selector :

   mTag          "testA"
   mNameSpace    0x00000003  (and that is kNameSpaceID_HTML ;
                              see content/base/public/nsINameSpaceManager.h)

so the sheet above is stored as :

    @namespace html url(http://www.w3.org/1999/xhtml);
    @namespace test url(http://www.example.org/);
    html|testA  { color: green; }

which is false. Without default namespace declaration in CSS, "testA" should be
interpreted as "*|testA" !

Since this bug is not related to the current proposed implementation, I am
still requesting r= and will work on the namespace bug separately.

[ REMINDER : the Composer team has set high priority on this bug ]

Comment 20

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

Attachment: test case for namespace bug

Comment 21

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

Try the test case attached above with viewer and dump the stylesheet !
You'll see clearly the bug !

Comment 22

[Pierre Saslawsky]

Brendan: I don't know why I kept the feeling over the years that !0 could in some 
environments return 0xffff (or 0xff or whatever number of f's the field can fit).  
I guess it must date from an old compiler bug, not me confusing with ~0, and I 
can now wipe it off the list of defensive behaviors I adopted after beeing badly 
bitten.

Daniel: r=pierre for this bug.  If you want to work on the namespace bug, open a 
separate bug report, or check whether it's not a dup of existing namespace bugs 
(bug 35847, bug 61244)

Comment 23

[Hixie (not reading bugmail)]

Filed namespace bug as bug 72302.

Comment 24

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

checked in :-)

Comment 25

[Boris Zbarsky [:bzbarsky]]

I just tried the following style in this morning's build:

  *:not(a) { background: red }

and it turned _only_ <a> elements red.  Happens in strict and quirks mode both

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase using element names in :not()

Comment 27

[Boris Zbarsky [:bzbarsky]]

Is this a consequence of the namespace issue perhaps?  Negated class selectors
seem to be working fine, as do negated attribute selectors.

Comment 28

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

??? investigating ....

Comment 29

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

confirming ; error found ; reopening. All my apologies, that bug was hidden
behind the namespace bug (see above) and hard to test.

Comment 30

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

Attachment: easy fix for bug detected by Boris

Comment 31

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

proposed easy fix for bug found by Boris. All Ian's tests (but 001.xml due
to namespace bug) are now green...

Marc: reviews needed please ;-)

Boris: your test is not helpful because it assigns a red background to all
elements including html and body ! Then all the document has a red background,
including anchors because you don't specify another background.

You should write :

  * { background-color : white }
  *:not(a) { background-color : red }

to see that anchors have a white background. Tested with my fix : it works.

Good night...

Comment 32

[Marc Attinasi]

[s]r=attinasi

Comment 33

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

ok, fixed. Sorry again.

Comment 34

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

Ian : your test http://www.hixie.ch/tests/adhoc/css/selectors/not/005.xml is
buggy : the html namespace is not declared in the stylesheet so the rule

   element2 > :not(html|test) { color: green; }

is invalid.

Comment 35

[Hixie (not reading bugmail)]

testcase fixed

Comment 36

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

Checking in 72302 has a side effect on the current RFE ; reopening.

Comment 37

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

Attachment: after 72302 integration, this patch makes all Hixie's tests become green

Comment 38

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

Proposing new fix for :not() after integration of 72302.
Tested on Hixie's tests :

   http://www.hixie.ch/tests/adhoc/css/selectors/not/

r= and sr= please ?

Comment 39

[Marc Attinasi]

[s]r=attinasi

Comment 40

[Pierre Saslawsky]

I guess everybody got confused with localTrue/localFalse and with when 'result' 
should be tested against them or directly against PR_TRUE.  Everything looks fine 
now but I recommend running the usual test suites about selectors if you haven't 
done so already. r=pierre

Comment 41

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

Pierre : absolutely, and I got confused myself. I ran all my css2 test
suite and all my applicable css3 selectors tests ; they are all green.
I am going to make even more tests before any check-in.

Marc,Pierre : thanks for reviews

Comment 42

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

fixed

Comment 43

[Hixie (not reading bugmail)]

VERIFIED over the past few days