RESOLVED FIXED 29245
elementFromPoint() and caretRangeFromPoint() work only in initial containing block
https://bugs.webkit.org/show_bug.cgi?id=29245
Summary elementFromPoint() and caretRangeFromPoint() work only in initial containing ...
Andrew Richards
Reported 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.
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-
Same, but hopefully removed tab chars from ChangeLogs (17.01 KB, patch)
2009-09-14 13:20 PDT, Andrew Richards
mrowe: review+
Andrew Richards
Comment 1 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.
Sam Weinig
Comment 2 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.
Eric Seidel (no email)
Comment 3 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!
Andrew Richards
Comment 4 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?
Eric Seidel (no email)
Comment 5 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. :)
Andrew Richards
Comment 6 2009-09-14 14:53:13 PDT
Comment on attachment 39571 [details] Same, but hopefully removed tab chars from ChangeLogs Ah, gotcha. Thanks :)
Xiaomei Ji
Comment 7 2009-09-14 14:53:54 PDT
*** Bug 29247 has been marked as a duplicate of this bug. ***
Mark Rowe (bdash)
Comment 8 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.
Eric Seidel (no email)
Comment 9 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.
Mark Rowe (bdash)
Comment 10 2009-09-15 11:40:39 PDT
Comment on attachment 39571 [details] Same, but hopefully removed tab chars from ChangeLogs Sam reviewed this.
Mark Rowe (bdash)
Comment 11 2009-09-15 11:40:56 PDT
Landed in r48398.
Note You need to log in before you can comment on or make changes to this bug.