Bug 18027 - CSS3 Selector Test: combination of hover and multiple chained sibling selector fails in Webkit
: CSS3 Selector Test: combination of hover and multiple chained sibling selecto...
Status: NEW
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://disruptive-innovations.com/zoo...
: HasReduction
:
: 12520 93304
  Show dependency treegraph
 
Reported: 2008-03-23 14:10 PST by
Modified: 2012-08-07 11:38 PST (History)


Attachments
test case (426 bytes, text/html)
2008-03-23 14:10 PST, 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 PST, Teun
no flags Details
hover-adjacent-v1 (19.82 KB, patch)
2009-07-27 00:11 PST, Yusuke Sato
no flags Review Patch | Details | Formatted Diff | Diff
hover-adjacent-v2 (13.80 KB, patch)
2009-08-24 04:10 PST, Yusuke Sato
no flags Review Patch | Details | Formatted Diff | Diff
hover-adjacent-v3 (13.18 KB, patch)
2009-10-22 11:17 PST, Yusuke Sato
eric: review-
Review Patch | Details | Formatted Diff | Diff
Test page to reproduce problem. (1.51 KB, text/html)
2010-04-18 20:48 PST, Stephanie Hobson
no flags Details


Note

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


Description From 2008-03-23 14:10:27 PST
The CSS3 selector test suite check this particular case: 

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

It fails in Webkit.
------- Comment #1 From 2008-03-23 14:10:51 PST -------
Created an attachment (id=19987) [details]
test case
------- Comment #2 From 2008-10-01 15:03:37 PST -------
Created an attachment (id=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 From 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 From 2009-07-27 00:11:39 PST -------
Created an attachment (id=33521) [details]
hover-adjacent-v1


---
 15 files changed, 299 insertions(+), 5 deletions(-)
------- Comment #5 From 2009-07-27 00:58:26 PST -------
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] [details]
> hover-adjacent-v1
> 
> 
> ---
>  15 files changed, 299 insertions(+), 5 deletions(-)
------- Comment #6 From 2009-08-06 16:56:56 PST -------
(From update of attachment 33521 [details])
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 From 2009-08-07 12:21:28 PST -------
(From update of attachment 33521 [details])
r- for tabs.
------- Comment #8 From 2009-08-24 04:10:52 PST -------
Created an attachment (id=38476) [details]
hover-adjacent-v2


---
 9 files changed, 238 insertions(+), 5 deletions(-)
------- Comment #9 From 2009-08-24 04:14:36 PST -------
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 From 2009-09-03 00:23:09 PST -------
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 From 2009-10-13 13:03:05 PST -------
Hyatt, this 3-line patch has been waiting for over a month.
------- Comment #12 From 2009-10-21 12:44:49 PST -------
(From update of attachment 38476 [details])
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 From 2009-10-22 11:17:46 PST -------
Created an attachment (id=41669) [details]
hover-adjacent-v3


---
 9 files changed, 232 insertions(+), 5 deletions(-)
------- Comment #14 From 2009-10-22 11:20:26 PST -------
Thanks Eric. Did #1 and #3.
Please take another look.
------- Comment #15 From 2009-11-25 22:22:53 PST -------
(From update of attachment 41669 [details])
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 From 2010-04-18 20:48:15 PST -------
Created an attachment (id=53654) [details]
Test page to reproduce problem.

Test file to reproduce bug. Also reproduces Bug 12520 and I attached it there too.
------- Comment #17 From 2010-11-02 08:54:00 PST -------
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.