Bug 18027 - CSS3 Selector Test: combination of hover and multiple chained sibling selector fails in Webkit
Summary: CSS3 Selector Test: combination of hover and multiple chained sibling selecto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://disruptive-innovations.com/zoo...
Keywords: HasReduction
Depends on:
Blocks: 93304 12520
  Show dependency treegraph
 
Reported: 2008-03-23 14:10 PDT by Robert Blaut
Modified: 2016-06-07 20:12 PDT (History)
11 users (show)

See Also:


Attachments
test case (426 bytes, text/html)
2008-03-23 14:10 PDT, Robert Blaut
no flags Details
Mac OS X-like dock tryout with 2 nextsibling selectors (like b:hover + b + b). (647 bytes, text/html)
2008-10-01 15:03 PDT, Teun
no flags Details
hover-adjacent-v1 (19.82 KB, patch)
2009-07-27 00:11 PDT, Yusuke Sato
no flags Details | Formatted Diff | Diff
hover-adjacent-v2 (13.80 KB, patch)
2009-08-24 04:10 PDT, Yusuke Sato
no flags Details | Formatted Diff | Diff
hover-adjacent-v3 (13.18 KB, patch)
2009-10-22 11:17 PDT, Yusuke Sato
eric: review-
Details | Formatted Diff | Diff
Test page to reproduce problem. (1.51 KB, text/html)
2010-04-18 20:48 PDT, Stephanie Hobson
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Blaut 2008-03-23 14:10:27 PDT
The CSS3 selector test suite check this particular case: 

.t1:hover + .unitTest + .unitTest { background-color: red; }

It fails in Webkit.
Comment 1 Robert Blaut 2008-03-23 14:10:51 PDT
Created attachment 19987 [details]
test case
Comment 2 Teun 2008-10-01 15:03:37 PDT
Created attachment 23996 [details]
Mac OS X-like dock tryout with 2 nextsibling selectors (like b:hover + b + b).

Added this attachment o show why this bug is important (to me).
If you want to create a mac os x-style dock, you need the span:hover+span+span selector to work.

I removed all the styling to make the item under the mouse pointer the largest, for easier testing.

Move the mouse from a to g to see the effect. The 3rd letter should be bigger.
Comment 3 M.K. van Achterberg 2008-11-24 23:26:59 PST
Ok, first post, I'll prolly do a lot of things wrong here : )
I ran into this bug while building a CSS map, and figured it was a bug and wrote it off.  But making more maps showed that *sometimes* webkit DOES work, though not well.
I made a simple test case, however I have not been able to discover what makes it *kinda* work-- I've tried to undo every difference between my two pages.  I still have a few ideas to try out but it will take a long time.
My test page with the reduced bug has links to the two complicated cases, "Europa" where it completely doesn't work, and "North America" where it works partially.  Both cases work completely in Konqueror and fail/half-work in Chrome.
http://stommepoes.nl/Tests/safaricombinator.html
Comment 4 Yusuke Sato 2009-07-27 00:11:39 PDT
Created attachment 33521 [details]
hover-adjacent-v1


---
 15 files changed, 299 insertions(+), 5 deletions(-)
Comment 5 Yusuke Sato 2009-07-27 00:58:26 PDT
I've confirmed that overall rendering performance is not degraded by this patch, using iBench 5.0.

Here is the result:

(1) without the patch

 HTML Load Speed                     
   All iterations      16.49            
   First iteration (downloaded)      4.6            
   Subsequent iteration (cached)      1.7            
 XML / CSS Load Speed                     
   All iterations      12.24            
   First iteration (downloaded)      2.55            
   Subsequent iteration (cached)      1.94            
 JavaScript         0.17
 HTML W3C DOM
   Overall      0.34            
   Sorting      0.25            
   DOM      0.9    

(2) with the patch

 HTML Load Speed                     
   All iterations      15.51            
   First iteration (downloaded)      3.89            
   Subsequent iteration (cached)      1.66    
 XML / CSS Load Speed                     
   All iterations      12.19            
   First iteration (downloaded)      2.57            
   Subsequent iteration (cached)      1.92       
 JavaScript         0.15
 HTML W3C DOM
   Overall      0.35            
   Sorting      0.26            
   DOM      0.9


--Yusuke


(In reply to comment #4)
> Created an attachment (id=33521) [details]
> hover-adjacent-v1
> 
> 
> ---
>  15 files changed, 299 insertions(+), 5 deletions(-)
Comment 6 Eric Seidel (no email) 2009-08-06 16:56:56 PDT
Comment on attachment 33521 [details]
hover-adjacent-v1

I'm not sure why your'e using such a super-wide indent her:
+            function log(msg) {
+                var console = document.getElementById('console');
+                console.innerText += msg;
+            }
Tabs?

FAILED/PASSED stuff is done for you if you use the js testing framework in fast/js/resources.  Not necessary here, but you should know for the future.

It would be better to test these with dumpAsText tests if possible.  window.getComputedStyle() should be able to help you there.

Hyatt will likely be needed to review the actual code change.
Comment 7 Eric Seidel (no email) 2009-08-07 12:21:28 PDT
Comment on attachment 33521 [details]
hover-adjacent-v1

r- for tabs.
Comment 8 Yusuke Sato 2009-08-24 04:10:52 PDT
Created attachment 38476 [details]
hover-adjacent-v2


---
 9 files changed, 238 insertions(+), 5 deletions(-)
Comment 9 Yusuke Sato 2009-08-24 04:14:36 PDT
Thanks for your review! I've attached the v2 patch:
- added dumpAsText().
- removed PNGs and its checksums.
- fixed indent level.

Could you take another look?
Comment 10 Eric Seidel (no email) 2009-09-03 00:23:09 PDT
In general this looks fine to me.  The code change itself is about 5 lines.  I would like Hyatt to look at the code change and give the final r+.

You will have the best luck reaching hyatt via irc or email.
Comment 11 Adam Barth 2009-10-13 13:03:05 PDT
Hyatt, this 3-line patch has been waiting for over a month.
Comment 12 Eric Seidel (no email) 2009-10-21 12:44:49 PDT
Comment on attachment 38476 [details]
hover-adjacent-v2

No objections or commentary in the 3 months that this patch has been up for review.

As far as I can tell, this looks fine to me.

Yusuke, this will cause failure on other platforms since you're only adding Mac results.

We have 3 options:
1.  Move the results next to the tests instead of in platform/mac.
2.  Add results for other platforms (only necessary if they differ, otherwise #1 is better).
3.  Make these tests dumpAsText (possible by using getComputedStyle).

If the tests were OK, I would r+ this.
Comment 13 Yusuke Sato 2009-10-22 11:17:46 PDT
Created attachment 41669 [details]
hover-adjacent-v3


---
 9 files changed, 232 insertions(+), 5 deletions(-)
Comment 14 Yusuke Sato 2009-10-22 11:20:26 PDT
Thanks Eric. Did #1 and #3.
Please take another look.
Comment 15 Eric Seidel (no email) 2009-11-25 22:22:53 PST
Comment on attachment 41669 [details]
hover-adjacent-v3

Sorry.  Bugs tend to get lost in the queue. :(

OK, tests are better, but still not as easy to read as they should be.

+
+The green square should turn red when the textarea is focused.
+
+Test passed.

You could easily hide the "green square" stuff when in DumpRenderTree, you can look for layoutTestController.  it's OK as is, but could be better.

Also:

PASS -- explanation why it passed

is better than putting "pass" latter in the sentence as it's easier for people to scan.  See the script-test examples in the tree, their output is PASS : reason and FAIL : reason.

Likewise, this is also confusing when reading as text:
+The third square should be green and turn red when the pointer hovers over the first black square.
+
+Test for https://bugs.webkit.org/show_bug.cgi?id=18027 - CSS3 Selector Test: combination of hover and multiple chained sibling selector fails in Webkit.
+
+The hover effect works!

Extra junk not needed in tests:
+<!DOCTYPE html>
+<html>
+<head>
+<title></title>

Overall this looks like a good change.  I really need to write a document about writing great layout tests.  Your tests should be as short as possible and super-easy to read/understand.  The problem is that most people won't have nearly the level of understanding that you do about the issue, so you need to make your tests super-easy to read/understand so that they can make quick judgements about what is passing/failing or what might be wrong.  This is why we try to encourage things like script-tests as they're generally easy to read. :)


Why do the tests need to be run from onload?  It's OK for them to be run from onload, but moving the <script> tag to the end of the body instead would have the same effect (or very similar).

I think we should go one more round on improving the tests here.  Please just email me when you update it and I'll be happy to review it right away!  Again, my apologies for the horrible delay!
Comment 16 Stephanie Hobson 2010-04-18 20:48:15 PDT
Created attachment 53654 [details]
Test page to reproduce problem.

Test file to reproduce bug. Also reproduces Bug 12520 and I attached it there too.
Comment 17 Divya Manian 2010-11-02 08:54:00 PDT
I just want to mention that this applies to hover with general sibling combinator as well. An example here:

http://dl.dropbox.com/u/952/cssvocab/index.html

Hover on the left curly bracket in Webkit browsers and on non-webkit browsers to see the bug.
Comment 18 Chris Rebert 2016-06-07 20:12:44 PDT
Unable to reproduce in current WebKit Nightly.