Bug 96541

Summary: REGRESSION: hit test doesn't take iframe scroll position into account
Product: WebKit Reporter: Rick Byers <rbyers>
Component: UI EventsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, dglazkov, eric, jchaffraix, kevers, rbyers, tonikitoo, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Rick Byers
Reported 2012-09-12 12:13:26 PDT
On current chromium builds, we're seeing EventHandler::hitTestResultAtPoint get the wrong location when inside an iframe that is scrolled (behaves as if the iframe scroll position was at the top). I suspect r127457 <http://trac.webkit.org/changeset/127457>, testing that now and working on a layout test.
Attachments
Patch (4.23 KB, patch)
2012-09-12 15:42 PDT, Rick Byers 		no flags
DetailsFormatted DiffDiff
Patch (6.11 KB, patch)
2012-09-13 01:52 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (6.21 KB, patch)
2012-09-13 04:14 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Rick Byers
Comment 1 2012-09-12 12:16:06 PDT
Yep, works fine if I revert r127457. I'll upload a layout test that demonstrates the issue. Allen, do you have time to take this? We're branching for Chrome M23 this weekend, so we'd like a fix (or revert) this week.
Allan Sandfeld Jensen
Comment 2 2012-09-12 12:20:14 PDT
That makes sense. The new code does not adjust for the scrollOffset in FrameView(). It should be simple to fix though by adding frameView()->scrollOffset() to the point passed to the child frame. It would be nice also add a test for it.
Allan Sandfeld Jensen
Comment 3 2012-09-12 12:20:48 PDT
(In reply to comment #1) > Yep, works fine if I revert r127457. I'll upload a layout test that demonstrates the issue. Allen, do you have time to take this? We're branching for Chrome M23 this weekend, so we'd like a fix (or revert) this week. I will fix it tomorrow if that is okay with you?
Rick Byers
Comment 4 2012-09-12 12:33:12 PDT
Thanks Allan. It looks simple enough, I'll take a crack at uploading a fix for you to review. If I don't have the CL in good enough shape tomorrow, then given the urgency, I'll gladly hand it off to you.
Rick Byers
Comment 5 2012-09-12 15:42:42 PDT
Created attachment 163722 [details] Patch
Rick Byers
Comment 6 2012-09-12 15:45:41 PDT
Looks like I'm not going to get this done today - I'm a little confused about the co-ordinate systems and the right place to apply the scroll offset, will need to dig deeper to understand it fully. So I'll just reassign to Allan. I've included a layout test that demonstrates the problem (I've verified it fails in ToT, but passes with r127457 reverted).
Allan Sandfeld Jensen
Comment 7 2012-09-13 01:52:10 PDT
Created attachment 163816 [details] Patch
WebKit Review Bot
Comment 8 2012-09-13 03:40:18 PDT
Comment on attachment 163816 [details] Patch Attachment 163816 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13827932 New failing tests: http/tests/security/feed-urls-from-remote.html editing/selection/drag-in-iframe.html http/tests/local/drag-over-remote-content.html http/tests/misc/policy-delegate-called-twice.html http/tests/misc/redirect-to-external-url.html touchadjustment/scroll-delegation/iframe-with-mainframe-scroll-offset.html fast/files/null-origin-string.html fast/dom/nodesFromRect/nodesFromRect-child-frame-content.html touchadjustment/iframe-boundary.html editing/pasteboard/drag-drop-dead-frame.html
Allan Sandfeld Jensen
Comment 9 2012-09-13 04:14:00 PDT
Created attachment 163834 [details] Patch Do not use FrameView locations since it is relative to parent frameview, instead calculate the location based on border and padding.
Rick Byers
Comment 10 2012-09-13 07:44:36 PDT
Thanks Allen. I've verified this patch fixes the original issue as well.
WebKit Review Bot
Comment 11 2012-09-13 08:03:28 PDT
Comment on attachment 163834 [details] Patch Clearing flags on attachment: 163834 Committed r128462: <http://trac.webkit.org/changeset/128462>
WebKit Review Bot
Comment 12 2012-09-13 08:03:33 PDT
All reviewed patches have been landed. Closing bug.
Julien Chaffraix
Comment 13 2012-09-14 12:24:38 PDT
Please include the revision that regressed in the bug title next time. That helps find out what a change broke without having to do trips to bugzilla.
Note You need to log in before you can comment on or make changes to this bug.