Bug 3428 – CSS2/3: Descendant and indirect adjacent selectors don't backtrack properly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dave Hyatt]

Both the descendant and indirect adjacent selectors do not backtrack properly.  It's possible for the 
element on the left to match the selector but then to fail the match later on as you continue to move to 
the left looking at other selectors.  When this happens WebKit just fails to match the rule, but instead you 
need to backtrack and keep trying more elements.

Should be fixable by splitting out the code to match these selectors into functions and getting some 
recursion in there. :)

We will need to test performance once the change is made, since making the descendant selector behave 
properly could have a negative performance impact.

[Dave Hyatt]

Created attachment 2243 [details]
Test showing the problem (works in Firefox)

[Evgeni Popov]

Created attachment 4059 [details]
Another test case (works with firefox / opera PC)

[Allan Sandfeld Jensen]

The same bug is tracked in KDE as http://bugs.kde.org/show_bug.cgi?id=101115 

[Allan Sandfeld Jensen]

Created attachment 15952 [details]
Quick patch

Here is a quick patch that fixes this bug isolated. 

The patch is not ready for review yet, but I will submit it later as part of a larger patch that fixes several serious issues in WebKit's CSS Selector implementation.

[Allan Sandfeld Jensen]

Created attachment 15980 [details]
Patch

A more complete patch. It cleans up minor crud in the CSS selector handling, but stays clear of any major changes. 

I decided a larger patch probably wasn't suitable at this time.

[Allan Sandfeld Jensen]

Comment on attachment 15980 [details]
Patch

forget it, it fails a few regression-tests, now that I got the tests working.

[Allan Sandfeld Jensen]

Created attachment 15995 [details]
New patch

Updated patch. I had missed how you handled pseudostyles differently.

I still get regression-failures, but I assume now that the rest are normal for the WebKitQt build.

[Dave Hyatt]

Comment on attachment 15995 [details]
New patch

This looks ok.  What did you mean when you said that there are still failures?  Are there tests we need to worry about (or do they just need to be fixed because they pass now)?

[Allan Sandfeld Jensen]

I have around 100 test that fails with small stuff like varying heights. I didn't read all of them. I assume that most are just the difference between Mac and Qt versions, but some are likely to be improvements too. I am still working on getting the tests to run properly.

[Cameron Zwarich (cpst)]

The only two failures I get on the Mac are fast/js/function-decompilation-operators.html and http/tests/security/cross-frame-access-protocol-explicit.html. I doubt these are failing due to the patch.

[Maciej Stachowiak]

Comment on attachment 15995 [details]
New patch

Given Cameron's comment and hyatt's previous comment, r=me for the feature-branch. We may need to roll feature branch forward for this to apply cleanly.

[Allan Sandfeld Jensen]

Created attachment 16305 [details]
Updated patch

This updated patch fixes a possible performance regression I thought would never occur in the wild.

The regression occured on a very deep tree with similar nested nodes, and a 3 or more part descent selector that matched these nodes. Unfortunately I found a case with "XHTML served as HTML" this week that did just that, because a lot of elements were never closed.

[Eric Seidel (no email)]

Just curious what the status here is.  Did you mean to mark that patch for review?

[Allan Sandfeld Jensen]

The old patch was approved. This new patch is just a speed-up. 

[Eric Seidel (no email)]

Comment on attachment 16305 [details]
Updated patch

I know mjs already reviewed the old one, but I figured I should at least look before landing.  Still looks sane.  r=me.

[Eric Seidel (no email)]

Landed on feature-branch.