Bug 3442

Summary:

CSS3: first-of-type selector

Product:

WebKit

Reporter:

Nicholas Shanks <nickshanks>

Component:

CSS

Assignee:

Dave Hyatt <hyatt>

Status:

VERIFIED FIXED

Severity:

Normal

Priority:

Version:

412

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

3375

Bug Blocks:

5468

Attachments:

Description	Flags
patch (includes 3375)	none
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	none
*-of-type patch part 1, v2.0.2	timothy: review-

Nicholas Shanks

Reported 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()

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Nicholas Shanks

Comment 1 2005-06-11 12:34:10 PDT

Created attachment 2250 [details] patch (includes 3375)

Dave Hyatt

Comment 2 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.

Nicholas Shanks

Comment 3 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.

Dave Hyatt

Comment 4 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.

Dave Hyatt

Comment 5 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.

Dave Hyatt

Comment 6 2005-06-11 23:55:43 PDT

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

Allan Sandfeld Jensen

Comment 7 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.

Nicholas Shanks

Comment 8 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

Nicholas Shanks

Comment 9 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 :)

Eric Seidel (no email)

Comment 10 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.

Eric Seidel (no email)

Comment 11 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.

Darin Adler

Comment 12 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.

Nicholas Shanks

Comment 13 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

Nicholas Shanks

Comment 14 2005-10-11 06:58:41 PDT

Comment on attachment 4308 [details] *-of-type patch part 1, v2 has typo, new diff coming

Nicholas Shanks

Comment 15 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.

Dave Hyatt

Comment 16 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.

Dave Hyatt

Comment 17 2005-10-11 10:16:38 PDT

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

Dave Hyatt

Comment 18 2005-10-11 10:19:39 PDT

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

Nicholas Shanks

Comment 19 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.

Dave Hyatt

Comment 20 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.

Timothy Hatcher

Comment 21 2005-10-22 17:53:11 PDT

I'm landing this.

Timothy Hatcher

Comment 22 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

Timothy Hatcher

Comment 23 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.

Dave Hyatt

Comment 24 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.)

Timothy Hatcher

Comment 25 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

Timothy Hatcher

Comment 26 2005-10-23 08:48:59 PDT

Please verify.

Note You need to log in before you can comment on or make changes to this bug.