Closed Bug 199692 Opened 21 years ago Closed 16 years ago

support document.elementFromPoint(x,y)

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha8

People

(Reporter: peterlubczynski-bugs, Assigned: eschew)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, relnote, Whiteboard: [good first bug])

Attachments

(2 files, 6 obsolete files)

final version 2, with expanded GetFrameForPoint signature
20.81 KB, patch
roc
: review+
roc
: superreview+
jst
: approval1.9+
Details | Diff | Splinter Review
Update and reenable test
17.10 KB, patch
roc
: review+
eschew
: review+
Details | Diff | Splinter Review 
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
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?
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.
Opera supports this. Can anyone test with KHTML?
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.
document.elementFromPoint currently works in:

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

Seems like Gecko is the only one left without support.
This would be pretty easy to implement once we have a good spec.
Anne? This would fit with the other CSSOM stuff.
This would be fairly easy to do, at least if the spec is "pick exactly the element which would receive the mouse click".
Whiteboard: [good first bug]
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).
Some trivial testing shows that it does indeed use hit testing as it goes accross document boundaries and such.
> 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!
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.
I'll take this -- seems like a good way to familiarize myself with various aspects of layout.
Assignee: general → jwalden+bmo
Attached patch Initial attempt (obsolete) — — Splinter Review 
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.
Assignee: jwalden+bmo → web+moz
Status: NEW → ASSIGNED
Attachment #276177 - Flags: review?(roc)
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.
> +  // 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);".)
+  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.
Attached patch take two, with tests (obsolete) — — Splinter Review 
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?
Attachment #276177 - Attachment is obsolete: true
Attachment #276744 - Flags: superreview?(roc)
Attachment #276744 - Flags: review?(jst)
Attachment #276177 - Flags: review?(roc)
Sorry for the bugspam, just wanted to note that in the IDL comment, the words "view frame" should be replaced with "document".
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 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;
+  // 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.
* 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.
Attachment #276744 - Attachment is obsolete: true
Attachment #277206 - Flags: superreview?(roc)
Attachment #277206 - Flags: review?(jst)
Attachment #276744 - Flags: superreview?(roc)
Attachment #276744 - Flags: review?(jst)
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?
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.
Good point. Both.  I think we might be able to end up with the document scrollbar anon content not having any content ancestors...
+  // 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.

> 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 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.
Attachment #277206 - Flags: review?(jst) → review+
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?
Attachment #277206 - Attachment is obsolete: true
Attachment #277206 - Flags: superreview?(roc)
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.
Attachment #277684 - Flags: superreview+
Attachment #277684 - Flags: review+
(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 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.  :-)
Attached patch Final version? (obsolete) — — Splinter Review 
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.
Attachment #277684 - Attachment is obsolete: true
Attachment #277711 - Flags: approval1.9?
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!
Adds third boolean param, with default for all callers but ElementFromPoint, to nsLayoutUtils::GetFrameForPoint.
Attachment #277711 - Attachment is obsolete: true
Attachment #277797 - Flags: approval1.9?
Attachment #277711 - Flags: approval1.9?
Comment on attachment 277797 [details] [diff] [review]
final version 2, with expanded GetFrameForPoint signature

a=jst per todays Gecko 1.9 meeting.
Attachment #277797 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Brilliant!

Patch in on trunk, with tests (!), marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Keywords: checkin-needed → dev-doc-needed, relnote
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
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
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.
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.
Flags: in-testsuite+ → in-testsuite?
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()?
(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 |       |
  |      * |       |
  +--------+       |
      |            |
      +------------+
Ben: what's the current status on fixing the tests to work?
> 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.
Attachment #281146 - Flags: review?(jwalden+bmo)
(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.
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.
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.
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.
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 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.
Attachment #281146 - Flags: review?(jwalden+bmo) → review-
Ben: ping?
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.
(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.
Depends on: 410463
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.
Attachment #281146 - Attachment is obsolete: true
Attachment #309193 - Flags: review?(web+moz)
Attachment #309193 - Flags: review?(roc)
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.
Attachment #309193 - Flags: review?(web+moz) → review+
Comment on attachment 309193 [details] [diff] [review]
Update and reenable test

Looks OK to me
Attachment #309193 - Flags: review?(roc) → review+
Blocks: 423049
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.
Flags: in-testsuite? → in-testsuite+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Don't reopen the bug please, file a new bug about those test failures, and CC me, I'll look at it.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 16 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: