Last Comment Bug 199692 - support document.elementFromPoint(x,y)
: support document.elementFromPoint(x,y)
Status: RESOLVED FIXED
[good first bug]
: dev-doc-complete, relnote
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla1.9alpha8
Assigned To: Ben Karel [eschew]
: gerardok
Mentors:
http://msdn2.microsoft.com/en-us/libr...
Depends on: 410463
Blocks: 423049
  Show dependency treegraph
 
Reported: 2003-03-28 12:43 PST by Peter Lubczynski
Modified: 2008-07-01 14:44 PDT (History)
24 users (show)
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial attempt (5.94 KB, patch)
2007-08-10 14:13 PDT, Ben Karel [eschew]
no flags Details | Diff | Splinter Review
take two, with tests (12.27 KB, patch)
2007-08-14 22:57 PDT, Ben Karel [eschew]
no flags Details | Diff | Splinter Review
#3: Fixes XUL crash, addresses review comments (21.15 KB, patch)
2007-08-18 00:05 PDT, Ben Karel [eschew]
jst: review+
Details | Diff | Splinter Review
Two extra null checks, and a new flag for nsDisplayListBuilder (21.88 KB, patch)
2007-08-22 00:26 PDT, Ben Karel [eschew]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Final version? (19.06 KB, patch)
2007-08-22 06:16 PDT, Ben Karel [eschew]
no flags Details | Diff | Splinter Review
final version 2, with expanded GetFrameForPoint signature (20.81 KB, patch)
2007-08-22 15:46 PDT, Ben Karel [eschew]
roc: review+
roc: superreview+
jst: approval1.9+
Details | Diff | Splinter Review
Update test to be more robust, try 1 (9.04 KB, patch)
2007-09-16 21:38 PDT, Ben Karel [eschew]
jwalden+bmo: review-
Details | Diff | Splinter Review
Update and reenable test (17.10 KB, patch)
2008-03-13 12:59 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
roc: review+
eschew: review+
Details | Diff | Splinter Review

Description Peter Lubczynski 2003-03-28 12:43:44 PST
In writing DHTML applications, it is sometimes desirable to get the DOM node
that is under a certain point. In IE, you can call
document.elementFromPoint(x,y) as MSDN describes here:
http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/elementfrompoint.asp
Comment 1 Boris Zbarsky [:bz] 2004-02-14 12:54:22 PST
The MS documentation does not describe this method well enough to make it
implementable... We could just map this to GetFrameForPoint, I suppose, but
there will probably be large behavior differences between the two.

The bigger question is, do we want to do this?
Comment 2 Erik Arvidsson 2004-03-05 09:33:42 PST
I just realized that I got stuck, because when doing a mouse down followed by a
mouse move there is no way to get the element below the pointer. With
elementAtPoint I could use document.elementAtPoint( e.clientX, e.clientY )

I guess I'll have to loop over all elements and get their box object (and loop
through all their parents to fix for the scrollable viewport bugs) to be able to
do this... yuk, that is going to be slow.
Comment 3 Erik Arvidsson 2004-03-05 09:34:31 PST
Opera supports this. Can anyone test with KHTML?
Comment 4 Yuh-Ruey Chen 2005-08-19 02:46:18 PDT
I'm not sure on the specifics of elementFromPoint, but it should do the exact
same calculation mouse events use to determine their target.

For example, a click event is processed like this:
1) OS sends click event to browser
2) browser calculates the DOM element target (taking into account z-index, etc.)
3) event flow

The method(s) responsible for step 2 must be available to script authors.

I'm apathetic concerning what API is used, since it's not a standardized feature
anyway, but along the lines of the non-standard createContextualFragment, an
elementFromPoint equivalent should exist.
Comment 5 Ish 2006-07-15 14:11:55 PDT
document.elementFromPoint currently works in:

* Opera 8+
* Safari
* ICab
* IE4+
* IE Mac 

Seems like Gecko is the only one left without support.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-07-15 17:56:00 PDT
This would be pretty easy to implement once we have a good spec.
Comment 7 Hixie (not reading bugmail) 2006-07-16 00:54:17 PDT
Anne? This would fit with the other CSSOM stuff.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-05-31 22:31:38 PDT
This would be fairly easy to do, at least if the spec is "pick exactly the element which would receive the mouse click".
Comment 9 Anne (:annevk) 2007-06-04 08:38:11 PDT
The problem with that is that browsers differ in "the element which would receive the mouse click" :-(

HTML5 has this block "[HIT TESTING TRANSPARENCY]" in the source code about what Internet Explorer implements (although I believe it misses a step about checking whether certain event listeners are registered).
Comment 10 Anne (:annevk) 2007-06-04 08:51:16 PDT
Some trivial testing shows that it does indeed use hit testing as it goes accross document boundaries and such.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-06-04 16:59:33 PDT
> The problem with that is that browsers differ in "the element which would
> receive the mouse click" :-(

That's bug 102695. dbaron pointed out the SVG pointer-events CSS property could be used here (bug 380573). I don't think we (Gecko) should slavishly copy IE's quirks here.

I definitely don't want to return elements from other documents under any circumstances!
Comment 12 Anne (:annevk) 2007-06-05 03:15:17 PDT
I was mistaken. IE and Opera do not return elements from other documents. Internet Explorer seems to throw an exception if there's no element at the position. I didn't copy that behavior into the specification:

  http://dev.w3.org/cvsweb/~checkout~/csswg/cssom/Overview.html?content-type=text/html;%20charset=utf-8#documentlayout-elementfrompoint

I left hit testing open for now. I believe Hixie plans to specify it as part of HTML5.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2007-06-11 00:59:28 PDT
I'll take this -- seems like a good way to familiarize myself with various aspects of layout.
Comment 14 Ben Karel [eschew] 2007-08-10 14:13:48 PDT
Created attachment 276177 [details] [diff] [review]
Initial attempt

Jeff said he wasn't going to be getting around to this for a while.

As bz suggested, this just wraps GetFrameForPoint. It's implemented for HTML documents only, and the IDL file updated is nsIDOMNSHTMLDocument.idl -- I wasn't sure where was the right place to put it. I moved nsDocument::CheckAncestryAndGetFrame from being private to protected, to use when the given point happens to be in an iframe or other subdocument.
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-10 16:43:55 PDT
Comment on attachment 276177 [details] [diff] [review]
Initial attempt

>Index: content/html/document/src/nsHTMLDocument.cpp

>+/* nsIDOMElement elementFromPoint (in long x, in long y); */
>+NS_IMETHODIMP
>+nsHTMLDocument::ElementFromPoint(PRInt32 aX, PRInt32 aY, nsIDOMElement** aReturn)
>+{
>+  NS_ENSURE_ARG_POINTER(aReturn);

Given that in non-void XPCOM methods the last argument is always an out pointer, I don't think the null-check is necessary.  Any code which does this can be and is easily fixed, and a crash is more likely to make that happen than a null pointer exception.

>+  // XXX what if the document is in the process of loading when this is called?

I doubt this is a problem; people already know DOM methods don't do what you expect them to do when they're called too early.

>+  if (currentDoc && currentDoc != this) {
>+	// If it's in a subdocument, return |this| doc's iframe, or null
>+    *aReturn = CheckAncestryAndGetFrame(currentDoc).get();
>+    return NS_OK;
>+  }
>+  else {
>+    nsCOMPtr<nsIDOMElement> ptElt(do_QueryInterface(ptCont));
>+    *aReturn = ptElt;
>+    NS_IF_ADDREF(*aReturn);
>+    return NS_OK;
>+  }
>+}

Moz code generally frowns on else-after-unconditional-return; just remove the else, moving everything inside it to the top level of the function.

>+  // returns the element that would recieve a mouse click at the given point
>+  nsIDOMElement             elementFromPoint(in long x, in long y);

This seems overly terse, and it ignores subframes, etc.  We really should say exactly what we do here, so far as that's possible, and probably link to the spec mentioned in comments.  Also, IDL comment style is javadoc-style /** docs */, which is then parsable by some documentation tools to produce nicely formatted IDL documentation (see the XPCOM reference on xulplanet.com).


You should also include a set of Mochitests (quite possibly just one test file, but definitely doing multiple tests of this functionality) with your patch.  This seems like a reasonably comprehensive set of things to test:

- a point within an element containing text
- a point within an element not containing text (<div style="..."></div>)
- a point within an element containing only whitespace (<p style="..."> </p>)
- points within position:static, fixed, absolute, and relative elements
- a point within a floated element
- a point within an element with display:none, so the element returned is the one underneath it
- a point within an element with visibility:hidden, so the element returned is the hidden one
- a point within an iframe in the page
- a point within an iframe within an iframe within the page

Details on Mochitests and how to write them are at <http://developer.mozilla.org/en/docs/Mochitest>, and feel free to ping me on IRC if you have any questions on doing so.
Comment 16 Eli Friedman 2007-08-12 16:55:55 PDT
> +  // XXX what if the document is in the process of loading when this is called?

You need to add a reflow flush somewhere to get up-to-date data. (Something like "FlushPendingNotifications(Flush_Layout);".)
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-08-14 09:24:12 PDT
+  NS_ENSURE_ARG_POINTER(aReturn);

This is actually OK, we do this in many similar places already.

+  PRInt32 x = nsPresContext::CSSPixelsToAppUnits(aX);
+  PRInt32 y = nsPresContext::CSSPixelsToAppUnits(aY);

These should be nscoord

+  // Text frames aren't DOMElements, we want the parent instead.
+  if (ptFrame->GetType() == nsGkAtoms::textFrame)
+	  ptFrame = ptFrame->GetParent();

Instead of this, it would be cleaner to search for the nearest enclosing element using nsINode::IsNodeOfType starting with ptFrame->GetContent().

I suggest putting this in nsIDOMNSDocument so it works in all documents. I think we want to reduce the differences between document types. CCing DOM peers.
Comment 18 Ben Karel [eschew] 2007-08-14 22:57:16 PDT
Created attachment 276744 [details] [diff] [review]
take two, with tests

Moves from DOMNSHTMLDocument to DOMNSDocument. Also includes more comments and documentation, as well as Mochitests, and fixes handling of anonymous/generated content.

A question about this:

>  PRBool suppressed;
>  ps->IsPaintingSuppressed(&suppressed);
>  if (suppressed) {
>    ps->UnsuppressPainting();
>  }

UnsuppressPainting() itself happens to make the same check internally, meaning that the "external" check is, strictly speaking, redundant. Is it okay to rely on that type of behavior and make the call without the three extra lines for the bool check? In this specific case only, or in general?
Comment 19 Ben Karel [eschew] 2007-08-14 23:08:22 PDT
Sorry for the bugspam, just wanted to note that in the IDL comment, the words "view frame" should be replaced with "document".
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-15 08:36:04 PDT
Looks good from a DOM point of view. The only issue is that you'll need higher up approval as this is a new feature which we've frozen for for 1.9 :(
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-15 10:16:30 PDT
Comment on attachment 276744 [details] [diff] [review]
take two, with tests

>Index: content/base/src/nsDocument.cpp

>+  nsCOMPtr<nsIDOMElement> ptElt(do_QueryInterface(ptCont));
>+  *aReturn = ptElt;
>+  NS_IF_ADDREF(*aReturn);
>+  return NS_OK;
>+}

I think (not entirely sure whether the nsCOMPtr -> nsIContent* conversion will happen automatically) this can just be:

  CallQueryInterface(ptCont, aReturn);
  return NS_OK;
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-08-15 16:04:05 PDT
+  // If painting is suppressed, GetFrameForPoint() will always return null,
+  // because BuildDisplayListForChild() returns early. If we didn't unsuppress
+  // painting, script callers would need to wait for the background paiting
+  // timer to fire, with a call to alert() or setTimeout(), or trigger layout
+  // themselves. Notably, without unsuppressing painting, this method will
+  // (probably unexpectedly) return null during onload.

You shouldn't unsuppress painting here. Instead, nsDisplayListBuilder should set mBackgroundOnly to false when aIsForEvents is true, so GetFrameForPoint works even when painting is suppressed. You should test that that works and that your layout flush actually works when painting is suppressed -- it should.

+  nsCOMPtr<nsIContent> ptCont(ptFrame->GetContent());
+  NS_ENSURE_STATE(ptCont);

Don't use an nsCOMPtr here, just use an nsIContent* --- you're not doing anything that could destroy the element.

+  while (ptCont->IsNativeAnonymous() ||
+         !(NODE_FROM(ptCont, currentDoc)->IsNodeOfType(nsINode::eELEMENT))) {
+    ptFrame = ptFrame->GetParent();

Why are you updating ptFrame? We don't need it. And why do you need NODE_FROM, nsIContent is a subclass of nsINode, right?

+  nsCOMPtr<nsIDOMElement> ptElt(do_QueryInterface(ptCont));
+  *aReturn = ptElt;
+  NS_IF_ADDREF(*aReturn);

What Jeff said.
Comment 23 Ben Karel [eschew] 2007-08-18 00:05:24 PDT
Created attachment 277206 [details] [diff] [review]
#3: Fixes XUL crash, addresses review comments

* Integrates review comments by jwalden and roc. roc, is the change to nsDisplayList to your satisfaction?

* Adds more detailed comments about what we do. For example, notes explicitly that the given point is taken to be relative to the upper-left-most *visible* point in the document. Thus, scrolling may cause different elements to be selected for the same point. NOT noted in the comment is that this behavior is the same as Safari and Internet Explorer, and not the same as Opera (which ignores scrolling, and will return elements that are not visible).

* Also noted in the comments is the approach taken with anonymous content: the first non-anonymous parent element is returned. I think we definitely don't want to expose any sort of anonymous content to unexpecting HTML documents. While it might be feasible to keep track of the original anonymous content node and return it instead of an XUL parent element, I think that's unnecessarily complex at the moment. If, in the future, XUL callers (extensions, perhaps?) find that getting direct access to anonymous elements would be useful, they're free to explain what behavior would be most convenient. Until then, consistency seems like the most attractive strategy. 

* Also in the doc comment is a note saying that XUL callers should wait until the onload event has fired before calling elementFromPoint(). The reason for this is that for XUL documents only, layout defers creation of the frame tree until after everything in the document has loaded. And no frame tree means no root frame, which means we can't use GetFrameForPoint. So we do the next-best thing, which is return the document element. That's the "XUL crash" referenced in the patch description -- in the previous patch, I hadn't considered the possibility that GetRootFrame() could return null.

* Fixes errant spaces in a Makefile, as bz so kindly pointed out had crept in...

* Includes an XUL mochitest, as well as HTML.
Comment 24 Boris Zbarsky [:bz] 2007-08-18 03:34:48 PDT
One quick nit.  As written, the code can crash due to ptContent being null if the click is on the viewport scrollbars and someone's gone and removed the documentElement from the document.  You probably want to add a 
|if (ptContent) {}| around that line.

Oh, and outdent the GetBindingParent() check one space to line up with the node type check?
Comment 25 Ben Karel [eschew] 2007-08-18 11:04:08 PDT
bz, I'm not quite clear on what you mean by "that line" -- do you mean inside the loop, or before the CallQueryInterface, or somewhere else?

So, like this:
>  while (ptContent &&
>         ptContent->GetBindingParent() ||
>         !ptContent->IsNodeOfType(nsINode::eELEMENT)) {
>    ptContent = ptContent->GetParent();
>  }

or this, or both?:
>  if (ptContent)
>    CallQueryInterface(ptContent, aReturn);

I couldn't get a crash by removing the document element through |document.removeChild(document.documentElement)| and calling elementFromPoint so I'm not sure which part of the code the problem would be in.
Comment 26 Boris Zbarsky [:bz] 2007-08-18 11:49:47 PDT
Good point. Both.  I think we might be able to end up with the document scrollbar anon content not having any content ancestors...
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-08-19 18:18:30 PDT
+  // If we have an anonymous element (such as an internal div from a textbox),
+  // or a node that isn't an element (such as a text frame node),
+  // replace it with the first non-anonymous parent node of type element.
+  while (!ptContent->IsNodeOfType(nsINode::eELEMENT) ||
+          ptContent->GetBindingParent()) {
+    ptContent = ptContent->GetParent();
+  }

Does this actually detect native anonymous content like scrollbars? I would gess not. Check nsIContent::IsNativeAnonymous() as well.

About nsDisplayList, I think I was wrong: we probably don't want to enable events to be dispatched to elements while painting suppression is active. So we probably need a new flag in the display list builder to say "ignore paint suppression". This shouldn't be in the constructor but default to false with a helper method to set it. And of course you'll need to propagate that through nsLineLayout::GetFrameForPoint.

Also, sicking should do the review instead of jst since he's already looked at the code.
Comment 28 Boris Zbarsky [:bz] 2007-08-19 18:45:39 PDT
> Does this actually detect native anonymous content like scrollbars?

Yes.  In fact it does so more reliably than IsNativeAnonymous(), which is basically broken for XUL elements (and especially XUL scrollbars, for which fixing it actually breaks things).  I can't hurt to || together the two, of course.
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-20 16:13:45 PDT
Comment on attachment 277206 [details] [diff] [review]
#3: Fixes XUL crash, addresses review comments

- In nsDocument::ElementFromPoint():

+  // XUL docs, unlike HTML, have no frame tree until everything's done loading
+  // If someone calls us before onload fires, there's no frame tree to use, so
+  // we'll just go ahead and return the document element.
+  if (!rootFrame)
+    return GetDocumentElement(aReturn);

I wonder if this wouldn't be more useful if we simply returned null in this case so that XUL code could detect this etc and not be fooled into thinking all is good and that the root element really is what is and will be at that point. I can't say I feel strongly about this, but it would seem to make more sens to return null here to me.

r=jst either way.
Comment 30 Ben Karel [eschew] 2007-08-22 00:26:52 PDT
Created attachment 277684 [details] [diff] [review]
Two extra null checks, and a new flag for nsDisplayListBuilder

Changes from previous patch:

* Changes behavior in the no root frame case (premature XUL caller) to be to return null instead of return the document element. jst is right, fail-fast is the correct thing to do here. With corresponding tweak to XUL Mochitest file.

* Adds two null checks for ptContent and a check for ptContent->IsNativeAnonymous(). roc, I couldn't find any cases in which IsNativeAnonymous was true when GetBindingParent returned null. But, in case they do exist, now or in the future, the check is there.

* Undoes the heavy-handed manipulation of mIsBackgroundOnly in nsDisplayList, replacing it with a settable flag to override the result of calls to IsBackgroundOnly. Of note, the flag is settable but not unsettable. Given that (A) there's only one setter at this point, and (B) if left unset, nsDisplayList acts exactly as before, I don't think the lack of an unsetter is a problem. Since ElementFromPoint certainly isn't the only caller of nsLayoutUtils::GetFrameForPoint, would it perhaps be better to add a PRBool (with a default) to GetFrameForPoint's signature, and make ElementFromPoint the only caller to override the default? I'm not sure whether the flexibility of expanding GetFrameForPoint's signature is worth, well, expanding the signature. Thoughts?

Also, who should I request r/sr from? sicking, jst, someone else for r? bz, roc, someone else for sr?
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-08-22 01:55:06 PDT
Comment on attachment 277684 [details] [diff] [review]
Two extra null checks, and a new flag for nsDisplayListBuilder

Just make IgnorePaintSuppression() set mIsBackgroundOnly to PR_FALSE, no need for its own flag.

jst's review carries over. You've got all the reviews you need now. Just post the final version of the patch and then request 1.9 approval.
Comment 32 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-22 02:41:36 PDT
(In reply to comment #31)
> Just post the final version of the patch and then request 1.9 approval.

I added this bug to the 1.9 meeting agenda for tomorrow-ish, so hopefully we can get a decision then on whether this is go for 1.9.  Ben, feel free to jump on the call if you want; I suspect most of it will be discussions over patch invasiveness, potential for regressions, etc. where implementation knowledge might help.  On the other hand, other people familiar with the patch will be there and can cover most of that if needed, but it certainly wouldn't hurt if you have time to be there.

http://wiki.mozilla.org/Firefox3/StatusMeetings/2007-08-21#Gecko_1.9_Meeting
Comment 33 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-22 02:50:30 PDT
Comment on attachment 277684 [details] [diff] [review]
Two extra null checks, and a new flag for nsDisplayListBuilder

>Index: content/xul/document/test/test_bug199692.xul

>+  // before onload, XUL docs have no root frame, therefore the best
>+  // elementFromPoint can really do is just return the document element.
>+  is(document.elementFromPoint(10,10), null,
>+     "Calls to elementFromPoint before onload should return null");

This comment's out of sync with reality.  :-)
Comment 34 Ben Karel [eschew] 2007-08-22 06:16:58 PDT
Created attachment 277711 [details] [diff] [review]
Final version?

Simplifies nsDisplayListBuilder::IgnorePaintSuppression per roc's comment.
Tweaks a few code comments, e.g. "an textbox". Also, the comment in the XUL mochitest. Good catch, Waldo!

Other than that, I think this is good to go.

Jeff, thanks for the advocacy/support on this. I'm not sure if I'll be awake for the meeting, since I'm in the middle of switching my sleep schedule around. If I'm awake, I'll be in #granparadiso. I'm also not so sure about the conference-call bit. Are there instructions somewhere for someone who's never done one before?

Anyways, honestly, I think I'm too new to the world of Mozilla code to have a feel for the integration-type issues. Here are the only two things I could think of:
* GetFrameForPoint now ignores paint suppression, even when it's being called from nsPresShell::HandleEvent. I don't think it should be a problem, but, well, see above. roc would know better than me.
* Also, I tried to write the unit tests in a pixel-agnostic manner, but they only work if the browser window is large enough to contain the point being examined. Thus, really really big fonts, or a small browser window, would cause a test failure, even if elementFromPoint is doing exactly what the doc comment says it will.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-08-22 14:46:02 PDT
Sorry Ben, but I think we should add a boolean parameter to nsLayoutUtils::GetFrameForPoint to control whether suppression is honoured or not. The event handling code to pass TRUE, and elementFromPoint should pass false. I mentioned this earlier and then missed it in my subsequent review. Sorry for this last minute change!
Comment 36 Ben Karel [eschew] 2007-08-22 15:46:32 PDT
Created attachment 277797 [details] [diff] [review]
final version 2, with expanded GetFrameForPoint signature

Adds third boolean param, with default for all callers but ElementFromPoint, to nsLayoutUtils::GetFrameForPoint.
Comment 37 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-29 11:46:35 PDT
Comment on attachment 277797 [details] [diff] [review]
final version 2, with expanded GetFrameForPoint signature

a=jst per todays Gecko 1.9 meeting.
Comment 38 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-29 13:41:45 PDT
Brilliant!

Patch in on trunk, with tests (!), marking FIXED.
Comment 39 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-29 14:45:20 PDT
Hrm, so one of the tests in content/html/document/test/test_bug199692.html is failing right now.  I replaced it with a todo() that compared doc.pt(x+d,y+d) with c.  Ben, could you take a look at this and figure out what's wrong so we can replace that particular test with one that works?

*** 6902 ERROR FAIL | Error thrown during test: doc.pt(x + d, y + d) has no properties | got 0, expected 1
Comment 40 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-29 14:53:34 PDT
Also, when I run the test within an iframe, it fails for me, even tho it passes when run as a single test.  I removed the entire file from the test list for now, since I have no idea what's wrong with it.
Comment 41 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-29 15:05:35 PDT
If it's the test that really is busted then it's ok to keep things as is while fixing the test. However if it really is the patch that is the problem, we might want to back it out while fixing the bug.
Comment 42 Ben Karel [eschew] 2007-08-29 17:44:47 PDT
The iframe case is expected behavior, in that (as noted in the IDL doc comment) elements that are not visible will not be returned by elementFromPoint. This behavior matches Safari, but not IE or Opera. I could certainly add a check to make sure that the coordinates of the element don't lie outside the document, but that wouldn't be testing elementFromPoint so much as it would be testing the test itself...

As for why the one test from comment #39 was failing, I don't know. It works fine on my machine. The error message is from the loop that verifies that most of the elements on the page are retrievable based on their computed coordinates, and doesn't give much useful information (like which element the test was failing for...). The two things that spring to mind are (A) really small font sizes on the testing machine (the script assumes that all elements are at least 6x6 pixels) or (B) a viewport that's too small to display the entire test page (580 px tall by 500 pixels wide should be sufficient). (B) seems more likely offhand.

It would be easiest to remove the position: rel, position: fixed, and position:static elements. They take up the bottom 110 pixels, and I don't think that style information like that makes a difference to GetFrameForPoint; if it did, we'd have bigger problems because GetFrameForPoint is used for event dispatching!

Jeff, if you remove the strings "fixed", "relative", and "absolute" from |var elts| declaration, does that take care of the one failing test?

Advice on what to do about the iframe case would be appreciated. Just detect and note what's going on in a passing-test message? Make it a todo()?
Comment 43 Jeff Walden [:Waldo] (remove +bmo to email) 2007-08-29 19:48:37 PDT
(In reply to comment #42)
> Advice on what to do about the iframe case would be appreciated. Just detect
> and note what's going on in a passing-test message? Make it a todo()?

I think the simplest thing to do would be to move the test into another file, have the test open a popup at a specific size with the test in it, and have that test use window.opener to report results and finish the test.

Also, since this iframe behavior is intentional, I think it would be good to do at least a test or two in that popup within an iframe within that popup, verifying that if the point in the document is scrolled out of view, elementFromPoint returns null.  I didn't properly understand that behavior when following this bug, but if that's what we're doing, we want to make sure we keep doing it so that we only change the behavior by deliberate action.

Another question: what do we do for document.elementFromPoint within an iframe at a point where the iframe's enclosing window has absolutely positioned content there, e.g. below at the *?  "Not visible" says it should return null, I guess, and anality dictates we should test for that specific behavior too.

      +------------+
      | iframe     |
  +--------+       |
  | abspos |       |
  |      * |       |
  +--------+       |
      |            |
      +------------+
Comment 44 Jeff Walden [:Waldo] (remove +bmo to email) 2007-09-14 14:21:08 PDT
Ben: what's the current status on fixing the tests to work?
Comment 45 Ben Karel [eschew] 2007-09-16 21:38:48 PDT
Created attachment 281146 [details] [diff] [review]
Update test to be more robust, try 1

> I think the simplest thing to do would be to move the test into another file,
> have the test open a popup at a specific size with the test in it, and have
> that test use window.opener to report results and finish the test.

Done.

> Also, since this iframe behavior is intentional, I think it would be good to do
> at least a test or two in that popup within an iframe within that popup,
> verifying that if the point in the document is scrolled out of view,
> elementFromPoint returns null.  I didn't properly understand that behavior
> when following this bug, but if that's what we're doing, we want to make sure > we keep doing it so that we only change the behavior by deliberate action.

Added one scrolled-iframe test.

> Another question: what do we do for document.elementFromPoint within an iframe
> at a point where the iframe's enclosing window has absolutely positioned
> content there, e.g. below at the *?  "Not visible" says it should return null,
> I guess, and anality dictates we should test for that specific behavior too.

GetFrameForPoint ignores content from other documents. I reworded parts of nsIDOMNSDocument.idl's doc comment to make it clearer, and changed "visible portion" to "visible bounds," which is more accurate.

Jeff, I'm not sure if this will actually fix the issue you originally had with the test, since I don't know how to reproduce the test failure. Could you test it and make sure it's kosher? I mean, it works for me, but then, so did the original test...

Apologies for the delay, the return to school has been taking up an inordinate amount of my time. I'll add dev documentation once the test situation stabilizes.
Comment 46 Gérard Talbot 2007-09-21 11:18:35 PDT
Updating URL accordingly
Comment 47 Eric Shepherd [:sheppy] 2007-09-28 12:54:11 PDT
Documented:

http://developer.mozilla.org/en/docs/DOM:document.elementFromPoint
Comment 48 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-09-28 17:35:17 PDT
(In reply to comment #45)
> Created an attachment (id=281146) [details]
> Update test to be more robust, try 1

Did this ever land?  Should it be?  If so, please add checkin-needed.
Comment 49 Ben Karel [eschew] 2007-09-28 20:51:18 PDT
I don't know if it's okay to land or not. Jeff Walden reported a problem with the original test, which works fine for me. As far as I know, Jeff is the only one who can sign off on this.
Comment 50 Jeff Walden [:Waldo] (remove +bmo to email) 2007-09-29 18:54:13 PDT
I'm sans a machine to test this for the moment; I'll probably have it back either tomorrow or Monday, and I'll make sure to test again then, as soon as possible since it shouldn't take very long to do so.
Comment 51 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-03 14:43:29 PDT
The tests pass on Windows for me with this patch, although I had to remove the existing test_bug199692.html from my tree for the patch to apply (guessing the patch was generated from an out-of-date tree).  However, the following test fails for me on a Mac:

Point to right of #txt should be #content got "", expected "content"

I'll look into this later tonight when I have a little time; I have a test and a paper due tonight which take priority for the moment.
Comment 52 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-05 01:02:13 PDT
So, the call that's "failing" is:

 document.elementFromPoint(258, 29);

I added a mousemove handler that displayed x/y/elementFromPoint values in the popup within its status bar.  The 258 is way center -- there's nothing wrong with it at all.  The 29 is |document.getElementById("content").offsetTop|.  If I continue up the parent chain I get a series of zeros for the ancestor |offsetTop| values, so it seems like this call should be working.  I also ran this on Safari 2ish to see what it did; its behavior on this test is so whacked that I couldn't use its behavior at all for evaluating this.

Given that this works on Windows, I tend to think it's a Mac-specific bug in some part of the code beneath the stuff in elementFromPoint.  I think we should comment out that test, file a new bug to figure out what the problem is, and be done with this bug (but see the review comment on the patch from when I had to look through it, too, coming in a second -- might be that those changes would make this go away, although I rather doubt it).
Comment 53 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-05 01:51:58 PDT
Comment on attachment 281146 [details] [diff] [review]
Update test to be more robust, try 1

Do please take these comments with a grain of salt; I don't do heavy web programming, so some of these comments (particularly the height/width ones) might not be quite right.


>Index: dom/public/idl/core/nsIDOMNSDocument.idl

>   /**
>-  * Returns the element visible at the given point, relative to the
>-  * upper-left-most visible point in the document.
>+  * Returns the element from the caller's document at the given point,
>+  * relative to the upper-left-most point in the (possibly scrolled)
>+  * window or frame.

I think it would be good to clarify here that:

  // document scrolled to (SX, SY), where SX == window.scrollX and
  // SY == window.scrollY
  document.elementFromPoint(x, y)
  ==
  the visible element at (x + SX, y + SY) in an unscrolled window
  of the same dimensions, assuming each component of those
  coordinates isn't outside window bounds (and assuming no
  fixed-position content comes into play here)

...assuming I understand the comment correctly.  The way scrolling is described is still a little unclear right now to me (although I admit the originally-simple description I had envisioned for the above note bloomed substantially when I tried to thoroughly reason about correctness, so maybe this is just really hard to do rigorously and concisely :-) ).


>Index: content/html/document/test/bug199692-popup.html

>+This file is designed to be oopened from test_bug199692.html in a popup

"oopened"


>+    window.opener.ok('elementFromPoint' in document, "document.elementFromPoint() must exist");

I'd probably add local ok() and is() functions which call the ones in window.opener so that the interface is consistent across the test files (and to save typing window.opener a lot).


>+            // The upper left corner of an element (with a moderate offset) will usually contain text,
>+            // and the upper right corner usually won't.
>+            var x = elt.offsetLeft, y = elt.offsetTop, w = elt.scrollWidth, h = elt.scrollHeight;

I could be wrong, but don't you want to get the offsetWidth and offsetHeight values for w/h, and don't you want to get the sum of offset* on the element and its chain of offsetParents for x/y?  The same holds several other places in the patch, so if I'm right helper functions used a bunch of places would be good.


>+            var d = 5;
>+            window.opener.is(doc.pt(x+d,y+d).id, id, "("+(x+d)+","+(y+d)+") IDs should match (upper left corner of "+id+")");

You could compare elt and the return of doc.pt directly, although maybe you're doing this for the debug messages.  I don't actually know offhand what output is() would give for comparing different elements.


>+        // content
>+        var c = document.getElementById('content');
>+        x = c.offsetLeft + c.clientWidth/2, y = c.offsetTop;

Ooh, and now it's clientWidth.  I don't really know which is the right one to use, but maybe use the same one for everything to reduce confusion?  MDC doesn't really give enough explanation for me to know what's right here and what's wrong.  :-\



Finally,

(In reply to comment #45)
> Added one scrolled-iframe test.

Looking again, it seems either I didn't know what I wanted, or I did, but I didn't communicate clearly.  I think we want a test where the iframe has been scrolled to (SX, SY) that ensures that the return of eFP(0,0) is the same as at eFP(SX,SY) in the same window unscrolled, with SX<the window's width and SY<the window's height (so basically the situation I mentioned at the start, as a test).

Also, maybe it's just really late here and my brain's already shut down, but did the test get forgotten from the patch?  I don't see any test that deals with a scrolled frame here.


And thanks for putting up with my review tardiness and incessant requests for tests!  I have a semi-bogus school holiday through this coming Tuesday, so if you can get an updated patch before then I can promise fast testing turnaround on it.
Comment 54 Jeff Walden [:Waldo] (remove +bmo to email) 2007-11-30 12:46:58 PST
Ben: ping?
Comment 55 Ben Karel [eschew] 2007-12-11 23:53:50 PST
Ugh, sorry for vanishing for so long. I just finished with this semester's final exams. As it turns out, graduate-level classes are harder and more time-intensive than undergrad classes! I never would have guessed...

Anyways, now that I have a a break and thus much more free time, I should be able to get back up to speed on Mozilla.

Initial, way-overdue thoughts: 
* Should detailed documentation about the behavior of the function go in comments, or should it go in e.g. devmo? Or perhaps both?
* "oopened," window.opener: easy fixes, yeah.
* offsetLeft, scrollWidth, clientWidth: I honestly don't remember exactly why those values were chosen. As far as I can recall, it was pretty much trial-and-error, except without the trial or error: I created the page layout structure, then used DOM Inspector to figure out what particular DOM properties would give me the offsets I wanted for each element. Is being "consistent" worth changing the code for, do you think? I'm tempted to say "leave it alone if it works," but maybe that's just me being lazy...
* Re: scrolled-iframe test: ah, yeah, I misunderstood. I thought you meant that you wanted to confirm that, if the frame was 100x100 (and not scrolled), a point at 200x200 would return null. I'll look into implementing your suggestion, now that (I think) I understand it better.
Comment 56 Jeff Walden [:Waldo] (remove +bmo to email) 2007-12-16 07:04:09 PST
(In reply to comment #55)
> Ugh, sorry for vanishing for so long. I just finished with this semester's
> final exams. As it turns out, graduate-level classes are harder and more
> time-intensive than undergrad classes! I never would have guessed...

Not a problem; I'm running low on free time myself lately.


> * Should detailed documentation about the behavior of the function go in
> comments, or should it go in e.g. devmo? Or perhaps both?

Ugh, I hate our current docs situation.  Maybe we should leave the nitty-gritty description for MDC and just add a link to the page there in IDL.  Certainly MDC's more useful for web developers, and they're going to use it more than people looking at Mozilla source will.


> * offsetLeft, scrollWidth, clientWidth: Is being "consistent" worth
> changing the code for, do you think? I'm tempted to say "leave it alone
> if it works," but maybe that's just me being lazy...

I think it'd be worthwhile to reduce the number of independent variables affecting whether or not the test passes, yes.  I don't imagine it'd be too difficult to drop in the following function and replace most of the property-accesses with calls to it:

function offsetOf(node, isTop)
{
  var v = 0, prop = isTop ? "offsetTop" : "offsetLeft";
  while (node)
  {
    v += node[prop];
    node = node.parentNode;
  }
  return v;
}


> * Re: scrolled-iframe test: ah, yeah, I misunderstood. I thought you meant
> that you wanted to confirm that, if the frame was 100x100 (and not
> scrolled), a point at 200x200 would return null. I'll look into
> implementing your suggestion, now that (I think) I understand it better.

I'm busy through late afternoon Thursday at least, but after that I should have a few hours to page this bug back into memory to double-check that your new understanding matches what I suggested -- my memory's way too hazy to say anything about it now.
Comment 57 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-13 12:59:19 PDT
Created attachment 309193 [details] [diff] [review]
Update and reenable test

I went ahead and re-updated this with the scrolled-iframe test, as far as I can remember it from the descriptions in the comments here -- this is all hideously paged out of my memory, jogged again by <http://www.quirksmode.org/blog/archives/2008/02/the_cssom_view.html>.

If you have time to quickly give this a once-over, roc, that'd be good; Ben, too, if you have the time.  I haven't run the tests on anything other than OS X this time around, so I'll have to do some careful tree-watching when I land it to make sure everybody stays happy, if necessary adding todos to do so.  At the moment I don't know the criteria that cause random_fail to fail right now, and I think we'll make more traction if we can just have the tinderboxen tell us who fails and how.
Comment 58 Ben Karel [eschew] 2008-03-13 14:35:10 PDT
Comment on attachment 309193 [details] [diff] [review]
Update and reenable test

Looks good to me, at least superficially. I'm also suffering from acute paging-out syndrome...

Anyways, a few nits:
* "tess" should probably be "test" or "tests."
* In addition to testing that negative coordinates return null when there's no element at the coordinates, it might also be good to include a test (in the scrolled iframe?) verifying that null is returned even if there "is" an element at those coords (lightblue).
* "upper right corner" should be "lower right corner" -- it was wrong in the original file too.

Also, I updated MDC to match the patch's superior wording for elementFromPoint's behavior.
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-03-13 16:58:25 PDT
Comment on attachment 309193 [details] [diff] [review]
Update and reenable test

Looks OK to me
Comment 60 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-14 17:13:10 PDT
I landed the patch on trunk.  Everything passes on Windows, but on Linux and Mac the tests that use random_fail fail and call todo_is.  I opened bug 423049 to fix those todos.
Comment 61 Lukas Blakk [:lsblakk] use ?needinfo 2008-07-01 11:05:28 PDT
The test cases for this bug are being extremely noisy on qm-win2k3-pgo01:

*** 12174 ERROR FAIL | Element from doubly nested iframe returned is from calling document | got [object HTMLHtmlElement], expected [object HTMLIFrameElement] | /tests/content/html/document/test/test_bug199692.html
*** 12172 ERROR FAIL | Hit testing should bypass hidden elements. | got [object HTMLIFrameElement], expected [object HTMLDivElement] | /tests/content/html/document/test/test_bug199692.html
*** 12166 ERROR FAIL | (327,313) IDs should match (upper left corner of textbox) | got [object HTMLIFrameElement], expected [object HTMLInputElement] | /tests/content/html/document/test/test_bug199692.html

For example.

There are almost 70 lines of errors like this from yesterdays builds and already 21 today.
Comment 62 Ben Karel [eschew] 2008-07-01 11:43:05 PDT
Unfortunately I'm flying across the country tomorrow afternoon for 10 days of family vacation.

I can't reproduce these failures on my Windows build (Firefox 3.0, not mozilla-central). How does one go about debugging unit test failures when you can't reproduce locally?

I'm not sure what the best thing to do is, considering that I won't be able to start looking into this for ~2 weeks, and even then I might not be able to reproduce the errors. Would someone more experienced like to weigh in?
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 12:22:29 PDT
Don't reopen the bug please, file a new bug about those test failures, and CC me, I'll look at it.
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 14:44:40 PDT
Bug 443040 was filed about that. Thanks.

Note You need to log in before you can comment on or make changes to this bug.