Bug 29245 - elementFromPoint() and caretRangeFromPoint() work only in initial containing block
Summary: elementFromPoint() and caretRangeFromPoint() work only in initial containing ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 29247 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-09-14 09:21 PDT by Andrew Richards
Modified: 2009-09-15 11:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch to use different bounds checking and extend tests (16.98 KB, patch)
2009-09-14 10:00 PDT, Andrew Richards
sam: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Same, but hopefully removed tab chars from ChangeLogs (17.01 KB, patch)
2009-09-14 13:20 PDT, Andrew Richards
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Richards 2009-09-14 09:21:27 PDT
Calling Document.elementFromPoint() and Document.caretRangeFromPoint() both will return null instead of a valid Range object when used on client coordinates that fall outside of the initial containing block. That is, any points which are not visible when the browser viewport is scrolled up and to the right all the way will yield null if you scroll the browser window and call either of the two functions on them.

Reproduce:
Create a page with enough content so that scrollbars are created. Scroll until some area not initially visible is now visible in the viewport. Call elementFromPoint or caretRangeFromPoint anywhere in this area.

Actual result:
null is returned.

Expected result:
return a valid Range object.

Revision 48354 on 10.5.7 (Intel)

You can see a demo of the problem here: http://emgio.com/caretrangefrompointbug/
Scroll the page down and you will the cursor chaser report which it receives null.
Comment 1 Andrew Richards 2009-09-14 10:00:30 PDT
Created attachment 39549 [details]
Patch to use different bounds checking and extend tests

I haven't worked with WebKit before, but after reading a bit I believe the bounds checking in Document::caretRangeFromPoint() and elementFromPoint() is mistakenly using boundsRect() from a FrameView to find if the point is in the viewport. This was causing it to return null if you called it with a point that had to be scrolled to. I changed it to use visibleContentRect() instead, which seems to have fixed it.

I also extended the tests to include checking for this issue. There are now hit tests which require a page to be scrolled before their points are visible.
Comment 2 Sam Weinig 2009-09-14 10:31:25 PDT
Comment on attachment 39549 [details]
Patch to use different bounds checking and extend tests

Nice catch. r=me.
Comment 3 Eric Seidel (no email) 2009-09-14 13:11:00 PDT
Comment on attachment 39549 [details]
Patch to use different bounds checking and extend tests

Tabs in your ChangeLog make this impossible to land using the commit-queue.  Either you can post a new patch, or someone can manually commit this and fix the ChangeLogs in the process.

Thanks for the patch!
Comment 4 Andrew Richards 2009-09-14 13:20:36 PDT
Created attachment 39571 [details]
Same, but hopefully removed tab chars from ChangeLogs

Don't know how that happened. Fixed now?
Comment 5 Eric Seidel (no email) 2009-09-14 14:29:48 PDT
If you want your patch reviewed and landed, you'll need to mark it r=? and commit-queue=? :)  Bug comments tend to get ignored since there are thousands made every day and it's hard to stand out in the sea of noise. :)
Comment 6 Andrew Richards 2009-09-14 14:53:13 PDT
Comment on attachment 39571 [details]
Same, but hopefully removed tab chars from ChangeLogs

Ah, gotcha. Thanks :)
Comment 7 Xiaomei Ji 2009-09-14 14:53:54 PDT
*** Bug 29247 has been marked as a duplicate of this bug. ***
Comment 8 Mark Rowe (bdash) 2009-09-15 09:47:25 PDT
(In reply to comment #5)
> If you want your patch reviewed and landed, you'll need to mark it r=? and
> commit-queue=? :)  Bug comments tend to get ignored since there are thousands
> made every day and it's hard to stand out in the sea of noise. :)

Please stop stating that contributors need to set the commit-queue flag in order to get a patch landed.  That has never been our policy.
Comment 9 Eric Seidel (no email) 2009-09-15 10:02:05 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > If you want your patch reviewed and landed, you'll need to mark it r=? and
> > commit-queue=? :)  Bug comments tend to get ignored since there are thousands
> > made every day and it's hard to stand out in the sea of noise. :)
> 
> Please stop stating that contributors need to set the commit-queue flag in
> order to get a patch landed.  That has never been our policy.

Nor am I suggesting that it's policy.  But if they want me to land it, that is how I land patches.  It was simply a personal comment.
Comment 10 Mark Rowe (bdash) 2009-09-15 11:40:39 PDT
Comment on attachment 39571 [details]
Same, but hopefully removed tab chars from ChangeLogs

Sam reviewed this.
Comment 11 Mark Rowe (bdash) 2009-09-15 11:40:56 PDT
Landed in r48398.