Bug 3442 - CSS3: first-of-type selector
Summary: CSS3: first-of-type selector
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 412
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on: 3375
Blocks: 5468
  Show dependency treegraph
 
Reported: 2005-06-11 12:30 PDT by Nicholas Shanks
Modified: 2005-10-23 08:48 PDT (History)
0 users

See Also:


Attachments
patch (includes 3375) (39.00 KB, patch)
2005-06-11 12:34 PDT, Nicholas Shanks
no flags Details | Formatted Diff | Diff
new patch, part 1 (7.07 KB, patch)
2005-06-13 11:05 PDT, Nicholas Shanks
darin: review-
Details | Formatted Diff | Diff
*-of-type patch part 1, v2 (19.05 KB, patch)
2005-10-11 06:27 PDT, Nicholas Shanks
nickshanks: review-
Details | Formatted Diff | Diff
*-of-type patch part 1, v2.0.1 (19.06 KB, patch)
2005-10-11 08:08 PDT, Nicholas Shanks
hyatt: review-
Details | Formatted Diff | Diff
Test case that shows why isElementNode checks are needed (133 bytes, patch)
2005-10-11 10:19 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
*-of-type patch part 1, v2.0.2 (20.23 KB, patch)
2005-10-12 11:11 PDT, Nicholas Shanks
timothy: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:
          :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()
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 
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.
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:
http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/html/tests/css3-modsel-34.html
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:
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.
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
css_base.h
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.

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