67594 – :hover selector fails when hovering over a child select element with size attribute

RESOLVED FIXED 67594

:hover selector fails when hovering over a child select element with size attribute

https://bugs.webkit.org/show_bug.cgi?id=67594

Summary :hover selector fails when hovering over a child select element with size att...

Vincent Woo

Reported 2011-09-05 03:29:11 PDT

Created attachment 106318 [details] Example html to trigger bug When a select element has a size attribute, hovering the mouse cursor over any of its options will deactivate any ancestor element :hover rules. Bug is not triggered if the size attribute is removed or if hovering over a part of the select element not covered by an option. I attached a html file to demonstrate the issue. The form has a light gray background that turns dark gray when the mouse cursor hovers over it because of a simple CSS :hover rule. the form background reverts to light gray when hovering the mouse cursor over any of the select options.

Attachments
Example html to trigger bug (683 bytes, text/html) 2011-09-05 03:29 PDT, Vincent Woo	no flags	Details
Proposed patch (1.47 KB, patch) 2011-09-08 08:15 PDT, Sameer Patil	darin: review- darin: commit-queue-	Details Formatted Diff Diff
Updated patch (4.43 KB, patch) 2011-09-09 05:02 PDT, Sameer Patil	darin: review-	Details Formatted Diff Diff
Updated patch (5.13 KB, patch) 2011-09-15 06:29 PDT, Sameer Patil	darin: review- webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Updated patch (5.25 KB, patch) 2011-09-19 06:08 PDT, Sameer Patil	no flags	Details Formatted Diff Diff
Updated patch (5.13 KB, patch) 2011-09-19 23:31 PDT, Sameer Patil	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Julien Chaffraix

Comment 1 2011-09-07 14:52:27 PDT

Confirmed on Chromium-linux beta & ToT. FF, Opera and IE properly handles the test case. It does not seem to be a regression.

Sameer Patil

Comment 2 2011-09-08 08:15:24 PDT

Created attachment 106739 [details] Proposed patch

Darin Adler

Comment 3 2011-09-08 08:24:24 PDT

Comment on attachment 106739 [details] Proposed patch Bug fixes need to come along with regression test cases. We don’t just change the code without adding a regression test.

Sameer Patil

Comment 4 2011-09-09 05:02:14 PDT

Created attachment 106862 [details] Updated patch Adding Layout test.

Sameer Patil

Comment 5 2011-09-12 00:36:51 PDT

Darin can you please review the patch?

Darin Adler

Comment 6 2011-09-12 16:22:26 PDT

Comment on attachment 106862 [details] Updated patch 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.

Sameer Patil

Comment 7 2011-09-15 06:29:26 PDT

Created attachment 107488 [details] Updated patch Adding updated patch.

Sameer Patil

Comment 8 2011-09-15 06:41:33 PDT

Comment on attachment 106862 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=106862&action=review >> Source/WebCore/rendering/RenderLayer.cpp:3811 >> + 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>. Incorporated additional focusable elements test cases in test. >> LayoutTests/ChangeLog:9 >> + * 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. done. >> LayoutTests/fast/css/hover_affects_ancestor.html:2 >> + "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. done. >> LayoutTests/fast/css/hover_affects_ancestor.html:29 >> + its child element.</p> > > I don’t see any code that “focuses” a child element. I mean hovering elements contained by div container here in test, made changes in test case. >> 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. Removed hardcoded values from test, now finding center point for element to send mouse hover.

WebKit Review Bot

Comment 9 2011-09-18 19:21:28 PDT

Comment on attachment 107488 [details] Updated patch Rejecting attachment 107488 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: indow.html = CRASH fast/events/mouseover-button.html = CRASH fast/events/mouseover-mouseout.html = CRASH fast/events/mouseover-mouseout2.html = CRASH fast/frames/frame-programmatic-noresize.html = CRASH inspector/debugger/source-frame.html = CRASH inspector/network-status-non-http.html = CRASH scrollbars/scrollbar-click-does-not-blur-content.html = CRASH scrollbars/scrollbar-iframe-click-does-not-blur-content.html = CRASH scrollbars/scrollbar-middleclick-nopaste.html = CRASH Full output: http://queues.webkit.org/results/9734811

WebKit Review Bot

Comment 10 2011-09-19 00:40:42 PDT

Comment on attachment 107488 [details] Updated patch Attachment 107488 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9747018 New failing tests: fast/events/frame-click-focus.html fast/events/mousedown-in-subframe-scrollbar.html fast/frames/frame-programmatic-noresize.html fast/events/mouseover-mouseout.html inspector/network-status-non-http.html fast/events/mouse-focus-imagemap.html platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html http/tests/inspector/network/network-initiator-from-console.html fast/events/mouseover-button.html platform/chromium/fast/text/text-stroke-with-border.html inspector/debugger/source-frame.html http/tests/inspector/network/network-sidebar-width.html fast/events/mouseout-on-window.html fast/events/mouseover-mouseout2.html fast/events/drag-file-crash.html

Sameer Patil

Comment 11 2011-09-19 06:08:23 PDT

Created attachment 107843 [details] Updated patch Null check required for result.innerNode(), some layout test does crash because innerNode might be null.

Darin Adler

Comment 12 2011-09-19 16:10:19 PDT

Comment on attachment 107843 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=107843&action=review > Source/WebCore/rendering/RenderLayer.cpp:3861 > + Node* newHoverNode = result.innerNode() ? (result.innerNode()->renderer() ? result.innerNode() : result.innerNonSharedNode()) : > + result.innerNode(); The formatting here is wrong. We don’t indent the second line like this. But also, this is evaluating result.innerNode() too many times. For example, the one after the final : could just be a zero. I suggest: Node* newHoverNode = result.innerNode(); if (newHoverNode && !newHoverNode->renderer()) newHoverNode = result.innerNonSharedNode(); I think that’s easier to read.

Sameer Patil

Comment 13 2011-09-19 23:29:30 PDT

Sameer Patil

Comment 14 2011-09-19 23:31:11 PDT

Created attachment 107969 [details] Updated patch

WebKit Review Bot

Comment 15 2011-09-21 18:51:38 PDT

Comment on attachment 107969 [details] Updated patch Clearing flags on attachment: 107969 Committed r95694: <http://trac.webkit.org/changeset/95694>

WebKit Review Bot

Comment 16 2011-09-21 18:51:43 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component CSS

Assignee

Nobody

Reported

2011-09-05 03:29 PDT

Modified

2011-09-21 18:51 PDT History

CC List

5 users Show

URL

Keywords HasReduction

Depends on

Blocks