| Summary: | :hover selector fails when hovering over a child select element with size attribute | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vincent Woo <webkit> | ||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | darin, dglazkov, jchaffraix, mkrp87, webkit.review.bot | ||||||||||||||
| Priority: | P2 | Keywords: | HasReduction | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Confirmed on Chromium-linux beta & ToT. FF, Opera and IE properly handles the test case. It does not seem to be a regression. Created attachment 106739 [details]
Proposed patch
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.
Created attachment 106862 [details]
Updated patch
Adding Layout test.
Darin can you please review the patch? 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. Created attachment 107488 [details]
Updated patch
Adding updated patch.
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. 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 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 Created attachment 107843 [details]
Updated patch
Null check required for result.innerNode(), some layout test does crash because innerNode might be null.
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. 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:3862 > Thanks for review. Updating patch as per your suggestion. Created attachment 107969 [details]
Updated patch
Comment on attachment 107969 [details] Updated patch Clearing flags on attachment: 107969 Committed r95694: <http://trac.webkit.org/changeset/95694> All reviewed patches have been landed. Closing bug. |
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.