Bug 29245 – elementFromPoint() and caretRangeFromPoint() work only in initial containing block

• Component: DOM
• Status: RESOLVED FIXED

[Andrew Richards]

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.

[Andrew Richards]

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 on attachment 39549 [details]
Patch to use different bounds checking and extend tests

Nice catch. r=me.

[Eric Seidel (no email)]

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]

Created attachment 39571 [details]
Same, but hopefully removed tab chars from ChangeLogs

Don't know how that happened. Fixed now?

[Eric Seidel (no email)]

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 on attachment 39571 [details]
Same, but hopefully removed tab chars from ChangeLogs

Ah, gotcha. Thanks :)

[Xiaomei Ji]

*** Bug 29247 has been marked as a duplicate of this bug. ***

[Mark Rowe (bdash)]

(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)]

(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 on attachment 39571 [details]
Same, but hopefully removed tab chars from ChangeLogs

Sam reviewed this.

[Mark Rowe (bdash)]

Landed in r48398.