Bug 23557 - Allow hit testing of page content without clipping to visible
Summary: Allow hit testing of page content without clipping to visible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-26 15:46 PST by Adam Treat
Modified: 2009-01-28 08:40 PST (History)
4 users (show)

See Also:


Attachments
Don't clip to visible option (6.13 KB, patch)
2009-01-26 15:48 PST, Adam Treat
zimmermann: review+
Details | Formatted Diff | Diff
Fixes bug and also adds test (7.45 KB, patch)
2009-01-27 06:10 PST, Adam Treat
no flags Details | Formatted Diff | Diff
Previous patches + mac export (8.26 KB, patch)
2009-01-27 09:36 PST, Adam Treat
staikos: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-01-26 15:46:47 PST
Currently all hit testing clips the test to the visible content of the frame.  QWebFrame::hitTestContent should not clip to the visible content, rather it should return the hit test result regardless of the currently visible viewport.  This patch adds a simple option that allows performing hit tests without clipping it to the visible viewport.
Comment 1 Adam Treat 2009-01-26 15:48:04 PST
Created attachment 27053 [details]
Don't clip to visible option

Implements the no clip option for hit tests.
Comment 2 Nikolas Zimmermann 2009-01-26 17:24:23 PST
Comment on attachment 27053 [details]
Don't clip to visible option

Patch looks fine, despite the fact that HitTestRequest is so ugly - it should really use enums as flags, instead of a wild series of booleans.
It would be great to get that fixed - but that's out of the scope for this patch. r=me.

Would you file a bug on HitTestRequest uglyness?
Comment 3 Adam Treat 2009-01-26 19:08:38 PST
Are you thinking of an enum that can be OR'd together to generate the appropriate HitTestRequest?  If so I'll come up with a patch... :)
Comment 4 Adam Treat 2009-01-26 22:03:17 PST
Reopened for layout test.
Comment 5 Adam Treat 2009-01-27 06:10:09 PST
Created attachment 27072 [details]
Fixes bug and also adds test

Added a test to qwebframe to guard against regression.  I don't see a clean way of adding hit testing layout tests, but if anyone can think of a good way let me know.
Comment 6 Adam Treat 2009-01-27 06:11:07 PST
Add Simon
Comment 7 Adam Treat 2009-01-27 06:11:55 PST
Add David Hyatt.
Comment 8 Nikolas Zimmermann 2009-01-27 07:35:18 PST
(In reply to comment #3)
> Are you thinking of an enum that can be OR'd together to generate the
> appropriate HitTestRequest?  If so I'll come up with a patch... :)
> 
Yes, exactly.
Comment 9 Simon Fraser (smfr) 2009-01-27 08:42:58 PST
Would using elementFromPoint() be a better way to achieve what you are trying to do here? Maybe it clips to the viewport, and should not? At least it might give you a way to write a LayoutTest without adding code to tst_qwebframe.
Comment 10 Adam Treat 2009-01-27 09:08:54 PST
We could certainly modify elementFromPoint to use a HitTestRequest that does not clip to the viewport and thereby achieve a LayoutTest using JS, however this is a significant behavioral change that I was not sure is correct.

The purpose of my patch is to make QWebFrame::hitTestContent method work correctly given its name and purpose.

If you like I can modify 'elementFromPoint' accordingly, but are you sure this behavior change is correct and won't break JS DOM API standards?
Comment 11 Simon Fraser (smfr) 2009-01-27 09:18:50 PST
Actually elementFromPoint takes "client" (viewport) coords, so only finds things that are visible. Darn.

Please update the patch to include a change to the WebCore.base.exp file for Mac, like:

__ZN7WebCore12EventHandler14scrollOverflowENS_15ScrollDirectionENS_17ScrollGranu
  __ZN7WebCore12EventHandler20handleTextInputEventERKNS_6StringEPNS_5EventEbb
-__ZN7WebCore12EventHandler20hitTestResultAtPointERKNS_8IntPointEb
+__ZN7WebCore12EventHandler20hitTestResultAtPointERKNS_8IntPointEbb
 __ZN7WebCore12EventHandler20sendContextMenuEventERKNS_18PlatformMouseEventE

and file another bug for the 'bool' cleanup.
Comment 12 Darin Adler 2009-01-27 09:21:40 PST
(In reply to comment #5)
> I don't see a clean way
> of adding hit testing layout tests, but if anyone can think of a good way let
> me know.

Generally you can make hit testing layout tests with eventSender, ensuring the correct element is hit. But perhaps you mean something different. This may be a fix that only affects some specific Qt API, not the behavior of the browser? I'm not really clear here on what the issue is.
Comment 13 Adam Treat 2009-01-27 09:31:28 PST
Darin, you have it right.  The Qt API exposes a hitTestContent method that should return a valid HitTestResult regardless of the current viewport.  As such, this patch is intended to fix the behavior of this one specific Qt API method.
Comment 14 Adam Treat 2009-01-27 09:36:08 PST
Created attachment 27075 [details]
Previous patches + mac export

Add the mac export line from Simon although I have no way to test if this is correct.
Comment 15 Adam Treat 2009-01-28 08:40:38 PST
Committed with r40311.

I'll open a bug report and make a patch for the bools next.