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()
Created attachment 2250 [details] patch (includes 3375)
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.
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.
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.
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.
This would also work without needing any code in close().
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.
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 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 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 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 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.
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 on attachment 4308 [details] *-of-type patch part 1, v2 has typo, new diff coming
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.
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.
Or author sheet. I'll attach a test case.
Created attachment 4315 [details] Test case that shows why isElementNode checks are needed
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 on attachment 4325 [details] *-of-type patch part 1, v2.0.2 Looks good now. Nice code cleanup too.
I'm landing this.
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 on attachment 4325 [details] *-of-type patch part 1, v2.0.2 This doesn't fix last-of-type and only-of-type.
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.)
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
Please verify.