Closed Bug 71647 Opened 24 years ago Closed 23 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: glazou, Assigned: glazou)

References

(Blocks 1 open bug)

Details

(Keywords: css3, testcase)

Attachments

(6 files)

Patch for 71647
49.57 KB, patch
Details | Diff | Splinter Review
new patch from last versions
39.74 KB, patch
Details | Diff | Splinter Review
test case for namespace bug
524 bytes, text/html
Details
testcase using element names in :not()
418 bytes, text/html
Details
easy fix for bug detected by Boris
634 bytes, patch
Details | Diff | Splinter Review
after 72302 integration, this patch makes all Hixie's tests become green
4.70 KB, patch
Details | Diff | Splinter Review 
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().
accepting bug
Status: NEW → ASSIGNED
I believe Daniel is working on this now -- setting to moz0.9
Priority: -- → P3
Target Milestone: --- → mozilla0.9
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.
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 ;-).

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).
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 !!!
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 !


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

r= and sr= ?
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. :)

sr=hyatt

Confirming that outliner.xul still ok with patch in :-)))
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.

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.

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
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. :-)
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.
Keywords: css3, mozilla1.0, patch, testcase
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 ]

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

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)
Depends on: 72302
Filed namespace bug as bug 72302.
checked in :-)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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
Is this a consequence of the namespace issue perhaps?  Negated class selectors
seem to be working fine, as do negated attribute selectors.
??? investigating ....
confirming ; error found ; reopening. All my apologies, that bug was hidden
behind the namespace bug (see above) and hard to test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...
[s]r=attinasi
ok, fixed. Sorry again.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
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.

testcase fixed
Checking in 72302 has a side effect on the current RFE ; reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 ?
Status: REOPENED → ASSIGNED
[s]r=attinasi
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

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
fixed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
VERIFIED over the past few days
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: