Bug 3442 – CSS3: first-of-type selector

• Component: CSS
• Status: VERIFIED FIXED

[Nicholas Shanks]

2005-06-11  Nicholas Shanks  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        - Implement structural pseudo-selectors from CSS3.

        * khtml/css/css_base.{h,cpp}: added :*-of-type selectors
        * khtml/css/cssparser.cpp: ditto
        * khtml/css/cssstyleselector.cpp: ditto + added auxillary Nth function
        * khtml/css/parser.y: renamed FUNCTION to NOTFUNCTION for :not()
        * khtml/css/tokenizer.flex: added {nth} regex and NOTFUNCTION
        * khtml/html/htmlparser.cpp: minor edit to fix :last-* and :only-*
        * khtml/xml/dom_elementimpl.{h,cpp}: ditto
        * khtml/xml/dom_nodeimpl.{h,cpp}: ditto
        * khtml/xml/xml_tokenizer.cpp: ditto

        Partial merge of svn log -v -r 365434 svn://anonsvn.kde.org/home/kde
                     and svn log -v -r 371719 svn://anonsvn.kde.org/home/kde
        
        This fixes bugs in:
          :last-child
          :only-child
        Adds support for:
          :first-of-type
          :last-of-type
          :only-of-type
        And builds foundations for forthcoming patches supporting:
          :nth-child()
          :nth-last-child()
          :nth-of-type()
          :nth-last-of-type()
          :enabled
          :disabled
          :indeterminate
          :contains()
          :lang()

[Nicholas Shanks]

Created attachment 2250 [details]
patch (includes 3375)

[Dave Hyatt]

This patch contains a bunch of seemingly unrelated changes (like your other pseudoelement/class patch) 
and also has a bunch of whitespace changes (some for the better but some for the worse as well).

Adding three bools to DOM elements adds too much footprint overhead.  Even if you make them bits, 
you're stil bloating every DOM element by 4 bytes.

Depending on close() to do this styling correctly also just punts on the dynamic case and will force two 
completely different code paths for dynamic vs. non-dynamic.

[Nicholas Shanks]

All whitespace changes were intended to bring code closer to your style guide. Apologies if some changes 
went the other way. A new diff will go up once Maciej lands 3375 so those changes won't be there 
anymore.

Other changes (three bools, close() etc.) were simply because that was what was in KDE's branch. I will 
look into ways of eliminating or reducing them.

[Dave Hyatt]

Generally we have tried to avoid any code that depends on close() if it is possible to simply write correct 
dynamic code (e.g., that responds to childrenChanged()) that can be generally applicable to both.  That 
said, I think close() may be necessary here.

Priority one should be avoiding a footprint increase.  Every DOM element getting four bytes bigger needs 
to be avoided.

[Dave Hyatt]

Instead of setting bits on the DOM element, I think some sort of hash in the cssstyleselector would be 
better.  It could also be smarter and not just assume a restyle is going to be needed.  It could cache the 
type of selector that matched and "invalidate" upon discovery of a later child that invalidates the previous 
match.  Then when some previous resolve was found to be wrong, it could just call setChanged.

[Dave Hyatt]

This would also work without needing any code in close().

[Allan Sandfeld Jensen]

Depending on close() is done for efficiency of the common case. There is no 
reason to invalidate or restyle while we are still parsing and more siblings 
are to be expected. 
 

[Nicholas Shanks]

Created attachment 2296 [details]
new patch, part 1

This patch just implements first-, last- and only-of-type, and does not fix the
bug that last-child and only-child exhibit, add anything to the footprint of
DOM elements, change many whitespace adjustments or any of that. It should be
easy to review and merge and will be built upon more in the coming days.
Regression test is CSS3 selector test #34:
http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/html/tests/css3-modsel-34.html

[Nicholas Shanks]

Comment on attachment 2296 [details]
new patch, part 1

set the review flag for this patch, since it's been sat here for two months and
no-one's looked at it :)

[Eric Seidel (no email)]

Comment on attachment 2296 [details]
new patch, part 1

A couple comments:
1.  while ( n ), there shoudl be no space around the n, even though other parts
of the file do this, this is against the style guidelines:
http://webkit.opendarwin.org/blog/?page_id=25
Likewise the * goes next to the n, according to the guidelines.
Otherwise the code look sane.  But I don't have much context.

[Eric Seidel (no email)]

Comment on attachment 2296 [details]
new patch, part 1

Another CSS issue, thus being passed to Beth as DRI.  Hyatt could/should also
take a look here.  I've taken a look at this patch as well, but dont' have a
lot of context w/ which to comment.  It needs a style update before landing.

[Darin Adler]

Comment on attachment 2296 [details]
new patch, part 1

The isElementNode() checks added on parent are unnecesssary. You can't have a
parent that's not an element.

The comment for PseudoLastOfType is the copied and pasted PseudoLastChild
comment, so it's wrong.

The formatting for the while () and if () don't match the coding style
guidelines as mentioned by Eric above.

The tagName locals should be AtomicString & rather than DOMString &, because
that way the == will be a simple pointer compare rather than a string compare.
In fact, hasTagName(xxx) should be used instead of tagName() == xxx.

I think this does look pretty good. Almost ready to go.

[Nicholas Shanks]

Created attachment 4308 [details]
*-of-type patch part 1, v2

Comments addressed and style amendments made, DOMString changed to AtomicString
(though it now conflicts with KHTML source), also all tabs changed to spaces in
css_base.h

[Nicholas Shanks]

Comment on attachment 4308 [details]
*-of-type patch part 1, v2

has typo, new diff coming

[Nicholas Shanks]

Created attachment 4310 [details]
*-of-type patch part 1, v2.0.1

fixed typo and made DOM/Atomic Strings into QualifiedNames instead, to match
the return and argument type of the ElementImpl methods i'm using.

[Dave Hyatt]

The isElementNode checks are necessary.  The root node's parentNode is the document, so without those 
checks it's possible for a rule like :first-child { color: red } to match the <html> element if used in a user 
agent or user stylesheet.

[Dave Hyatt]

Or author sheet.  I'll attach a test case.

[Dave Hyatt]

Created attachment 4315 [details]
Test case that shows why isElementNode checks are needed

[Nicholas Shanks]

Created attachment 4325 [details]
*-of-type patch part 1, v2.0.2

restored element check per hyatt's comments, passes both the WC3 selector test
(number 34, linked above) and hyatt's isElementNode test case (attached to
bug), where the previous patch only passed the former.

[Dave Hyatt]

Comment on attachment 4325 [details]
*-of-type patch part 1, v2.0.2

Looks good now.  Nice code cleanup too.

[Timothy Hatcher]

I'm landing this.

[Timothy Hatcher]

This patch only seems to fix first-of-type.

last-of-type and only-of-type don't pass the W3C tests.

http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/html/tests/css3-modsel-35.html
http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/html/tests/css3-modsel-36.html

[Timothy Hatcher]

Comment on attachment 4325 [details]
*-of-type patch part 1, v2.0.2

This doesn't fix last-of-type and only-of-type.

[Dave Hyatt]

Yup, they won't work yet, primarily because they require you to do re-resolves, and the patch doesn't 
attempt to solve that problem yet.  (I rejected the solution in the current KHTML tree because it added 4 
bytes to all nodes.)

[Timothy Hatcher]

Landed.

Checking in khtml/css/css_base.cpp;
/cvs/root/WebCore/khtml/css/css_base.cpp,v  <--  css_base.cpp
new revision: 1.21; previous revision: 1.20

Checking in khtml/css/css_base.h;
/cvs/root/WebCore/khtml/css/css_base.h,v  <--  css_base.h
new revision: 1.19; previous revision: 1.18

Checking in khtml/css/cssstyleselector.cpp;
/cvs/root/WebCore/khtml/css/cssstyleselector.cpp,v  <--  cssstyleselector.cpp
new revision: 1.213; previous revision: 1.212

Checking in fast/selectors/034.html;
/cvs/root/LayoutTests/fast/selectors/034.html,v  <--  034.html
initial revision: 1.1

[Timothy Hatcher]

Please verify.