[webkit-reviews] review denied: [Bug 67594] :hover selector fails when hovering over a child select element with size attribute : [Attachment 106862] Updated patch

Mon Sep 12 16:22:26 PDT 2011

Darin Adler <darin at apple.com> has denied Sameer Patil <mkrp87 at motorola.com>'s
request for review:
Bug 67594: :hover selector fails when hovering over a child select element with
size attribute
https://bugs.webkit.org/show_bug.cgi?id=67594

Attachment 106862: Updated patch
https://bugs.webkit.org/attachment.cgi?id=106862&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106862&action=review

Thanks. This looks OK. But you need to fix the test case.

> Source/WebCore/rendering/RenderLayer.cpp:3811
> -    Node* newHoverNode = result.innerNode();
> +    Node* newHoverNode = result.innerNode()->renderer() ? result.innerNode()
: result.innerNonSharedNode();

This change is broad enough that I think we need more test cases than just the
one with <select> and <option>.

> LayoutTests/ChangeLog:9
> +	   * fast/css/hover_affects_ancestor-expected.txt: Added.
> +	   * fast/css/hover_affects_ancestor.html: Added.

This filename includes underscores. If you look at the other tests in this
directory you’ll see they use dashes, not underscores.

> LayoutTests/fast/css/hover_affects_ancestor.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
> +    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

This test is in a .html file bug is an .xhtml test. That most likely means the
code is actually not parsed as XHTML at all. The test should either be changed
to HTML (preferred) or put in a file with an XHTML extension.

You should just put an HTML5 doctype here. Or none at all.

> LayoutTests/fast/css/hover_affects_ancestor.html:29
> +    <p>This test ensures that ancestor element hover rules are not affected
when we focus
> +    its child element.</p>

I don’t see any code that “focuses” a child element.

> LayoutTests/fast/css/hover_affects_ancestor.html:51
> +	   eventSender.mouseMoveTo(143, 180);

Does this work cross-platform? It seems to me that the hardcoded coordinates
are unlikely to be correct on all platforms.