461803 - need elementFromPoint that optionally ignores scrollframe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

See discussion in bug 459604.

Fennec needs to be able to find elements at a given point in the window, which may or may not be visible in the viewport. document.elementFromPoint only works for elements that are scrolled into view. The current proposal is to add an elementFromPoint method to nsIDOMWindowUtils that takes an optional "ignoreScrollFrame" argument.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Is there a good way to share this code between nsDOMWindowUtils and nsDocument? A helper on nsContentUtils maybe? nsDocument::ElementFromPoint does some layout-y stuff like FlushPendingNotifications, and I'm not sure how doable that is from nsDOMWindowUtils. Is it even necessary?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It certainly is doable from nsDOMWindowUtils, and it is necessary in general to make sure that the layout is up to date. Now, it may be that for your needs you don't need the layout to be up to date and you're happy to query the current frame tree. If so, taking out the FPN will improve performance.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you want to share code, moving code to nsContentUtils is a reasonable way to go. Another way would be to define an ElementFromPoint helper in nsIDocument.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP

This seems to work, but I'm not sure I like it very much. I had to make some changes to CheckAncestryAndGetFrame and move it to nsContentUtils along with most of ElementFromPoint, and have nsContentUtils include nsLayoutUtils.h.

Thoughts?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Can we change nsIDocument for 1.9.1? That would probably make things simpler.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This is simpler, and works. Not sure who the best person to review this is... I'll take r+sr from anyone, though!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 346069 [details] [diff] [review]
patch

>+++ b/content/base/public/nsIDocument.h
>+   * Helper for nsIDOMNSDocument::elementFromPoint implementation that allows
>+   * ignoring the scroll frame and/or avoiding layout flushes.

This should probably document the args.  An @see pointing to the nsIDOMWindowUtils docs is likely sufficient documentation.

>+++ b/dom/public/idl/base/nsIDOMWindowUtils.idl
>+  nsIDOMElement elementFromPoint(in long aX,
>+                                 in long aY,
>+                                 [optional] in boolean aIgnoreScrollFrame,
>+                                 [optional] in boolean aFlushLayout);

This needs documentation (e.g. which scrollframe is being ignored? Does passing false for aFlushLayout flush anything else?).

I'm also not so happy about having the default value of aFlushLayout being false.  Since we have no good way to change that, maybe flip the args and make that one required?  Callers that don't want to pass anything for these two booleans can just use the nsIDOMNSDocument version of this method.

With those nits, r+sr=bzbarsky

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

Changes:
-s/ignoreScrollFrame/ignoreRootScrollFrame/ per bug 462887
-made both parameters non-optional
-added comments to nsIDOMWindowUtils, and reference in nsIDocument.h

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: real updated patch

Comment 11

[:Gavin Sharp [email: [e-mail]]]

bz, do the comments look OK to you?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Looks good to me.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/bfed44de0709