Bug 96541 - REGRESSION: hit test doesn't take iframe scroll position into account
Summary: REGRESSION: hit test doesn't take iframe scroll position into account
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-12 12:13 PDT by Rick Byers
Modified: 2012-09-14 12:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.23 KB, patch)
2012-09-12 15:42 PDT, Rick Byers
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2012-09-13 01:52 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2012-09-13 04:14 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Byers 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.
Comment 1 Rick Byers 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.
Comment 2 Allan Sandfeld Jensen 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.
Comment 3 Allan Sandfeld Jensen 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?
Comment 4 Rick Byers 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.
Comment 5 Rick Byers 2012-09-12 15:42:42 PDT
Created attachment 163722 [details]
Patch
Comment 6 Rick Byers 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).
Comment 7 Allan Sandfeld Jensen 2012-09-13 01:52:10 PDT
Created attachment 163816 [details]
Patch
Comment 8 WebKit Review Bot 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
Comment 9 Allan Sandfeld Jensen 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.
Comment 10 Rick Byers 2012-09-13 07:44:36 PDT
Thanks Allen.  I've verified this patch fixes the original issue as well.
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2012-09-13 08:03:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Julien Chaffraix 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.