Bug 3442

Summary: CSS3: first-of-type selector
Product: WebKit Reporter: Nicholas Shanks <nickshanks>
Component: CSSAssignee: Dave Hyatt <hyatt>
Severity: Normal    
Priority: P2    
Version: 412   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on: 3375    
Bug Blocks: 5468    
Description Flags
patch (includes 3375)
new patch, part 1
darin: review-
*-of-type patch part 1, v2
nickshanks: review-
*-of-type patch part 1, v2.0.1
hyatt: review-
Test case that shows why isElementNode checks are needed
*-of-type patch part 1, v2.0.2 timothy: review-

Description Nicholas Shanks 2005-06-11 12:30:46 PDT
2005-06-11  Nicholas Shanks  <contact@nickshanks.com>

        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:
        Adds support for:
        And builds foundations for forthcoming patches supporting:
Comment 1 Nicholas Shanks 2005-06-11 12:34:10 PDT
Created attachment 2250 [details]
patch (includes 3375)
Comment 2 Dave Hyatt 2005-06-11 13:09:34 PDT
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.
Comment 3 Nicholas Shanks 2005-06-11 13:21:03 PDT
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 

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.
Comment 4 Dave Hyatt 2005-06-11 13:51:45 PDT
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.
Comment 5 Dave Hyatt 2005-06-11 23:31:36 PDT
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.
Comment 6 Dave Hyatt 2005-06-11 23:55:43 PDT
This would also work without needing any code in close().
Comment 7 Allan Sandfeld Jensen 2005-06-13 03:44:15 PDT
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. 
Comment 8 Nicholas Shanks 2005-06-13 11:05:32 PDT
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:
Comment 9 Nicholas Shanks 2005-08-08 14:46:35 PDT
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 :)
Comment 10 Eric Seidel (no email) 2005-09-17 14:21:04 PDT
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:
Likewise the * goes next to the n, according to the guidelines.
Otherwise the code look sane.  But I don't have much context.
Comment 11 Eric Seidel (no email) 2005-09-17 14:21:34 PDT
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.
Comment 12 Darin Adler 2005-09-24 03:09:42 PDT
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.
Comment 13 Nicholas Shanks 2005-10-11 06:27:52 PDT
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
Comment 14 Nicholas Shanks 2005-10-11 06:58:41 PDT
Comment on attachment 4308 [details]
*-of-type patch part 1, v2

has typo, new diff coming
Comment 15 Nicholas Shanks 2005-10-11 08:08:07 PDT
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.
Comment 16 Dave Hyatt 2005-10-11 10:15:38 PDT
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.
Comment 17 Dave Hyatt 2005-10-11 10:16:38 PDT
Or author sheet.  I'll attach a test case.
Comment 18 Dave Hyatt 2005-10-11 10:19:39 PDT
Created attachment 4315 [details]
Test case that shows why isElementNode checks are needed
Comment 19 Nicholas Shanks 2005-10-12 11:11:02 PDT
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.
Comment 20 Dave Hyatt 2005-10-17 20:42:44 PDT
Comment on attachment 4325 [details]
*-of-type patch part 1, v2.0.2

Looks good now.  Nice code cleanup too.
Comment 21 Timothy Hatcher 2005-10-22 17:53:11 PDT
I'm landing this.
Comment 22 Timothy Hatcher 2005-10-22 18:00:52 PDT
This patch only seems to fix first-of-type.

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

Comment 23 Timothy Hatcher 2005-10-22 18:02:28 PDT
Comment on attachment 4325 [details]
*-of-type patch part 1, v2.0.2

This doesn't fix last-of-type and only-of-type.
Comment 24 Dave Hyatt 2005-10-22 22:40:52 PDT
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.)
Comment 25 Timothy Hatcher 2005-10-23 08:24:55 PDT

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
Comment 26 Timothy Hatcher 2005-10-23 08:48:59 PDT
Please verify.