Closed Bug 1164427 Opened 10 years ago Closed 9 years ago

Support elementsFromPoint

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed
relnote-firefox --- 46+

People

(Reporter: cwiiis, Assigned: qdot)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, feature)

Attachments

(3 files, 6 obsolete files)

Patch 1 (v6) - Implement ElementsFromPoint
8.59 KB, patch
roc
: review+
khuey
: review+
lizzard
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta-
Details | Diff | Splinter Review
Patch 2 (v2) - Web Platform Test for elementFromPoint/elementsFromPoint dealing with negative margins; r=roc
2.79 KB, patch
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta-
Details | Diff | Splinter Review
Patch 2 (v2 AURORA VERSION) - Web Platform Test for elementFromPoint/elementsFromPoint dealing with negative margins; r=roc
2.75 KB, patch
Details | Diff | Splinter Review
Microsoft have two very useful extensions that allow you to check what elements (plural) intersect with a point or a rect in screen coordinates. Doing the same thing in JS is slow and error-prone, these would be very useful APIs.
We should just implement this part of CSS-OM http://dev.w3.org/csswg/cssom-view/#dom-document-elementsfrompointx-y And about elementsFromRect - better to get it to the spec first before implementing it. could you file a spec bug?
Summary: Support msElementsFromPoint and msElementsFromRect → Support elementsFromPoint (and elementsFromRect?)
Has there been any movement around this? Continues to be a stumbling block for writing any kind of UI that involves drag-and-drop interaction :(
The spec seems to have elementsFromPoint, but not elementsFromRect. But I don't think anyone has looked at implementing this in Gecko? Want to take this bug? ;)
n?myself to take a look at at least elementsFromPoint when I have spare time... I remember looking at hit detection years ago and if my memory isn't too fuzzy, I don't think it'd be hard to implement...
Flags: needinfo?(chrislord.net)
Bug 1164427: Implement elementsFromPoint This gives us the ability to get a NodeList of elements that are at point x,y in paint order from the top most frame and is the target for hit testing. http://www.w3.org/TR/cssom-view/#dom-document-elementsfrompoint
This is far from complete but it is a start so that people can start giving me feedback (especially since I have not worked on this area of the code base before) things left to do: 1) What is the best way to test this. I couldnt find specific tests for elementFromPoint but have seen uses in other tests 2) Update Layout code to give me the elements in that frame
Nice start :)
Flags: needinfo?(chrislord.net)
I have added tests to Web-Platform-Tests since there are no tests for elementFromPoint/elementsFromPoint https://github.com/w3c/web-platform-tests/commit/68823b89d97b51927051a73161a43a72181edc54 https://github.com/w3c/web-platform-tests/commit/846c2a11736fd0d9bc9dd09f60a38ab0b4b1dc56 These will be merged with the next WPT merge (probably next week)
Comment on attachment 8683930 [details] MozReview Request: Bug 1164427: Implement elementsFromPoint Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24469/diff/1-2/
Got it closer to compiling now!
David, any chance we can get this fixed soon? :) IRCCloud is sluggish in Firefox, and one of their devs said one issue is they have to polyfill elementsFromPoint in Firefox. This function is called when scrolling, when new messages arrive, etc. It'd be great to have this natively so we can see if it helps...
Flags: needinfo?(dburns)
Comment on attachment 8683930 [details] MozReview Request: Bug 1164427: Implement elementsFromPoint Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24469/diff/2-3/
I am trying to do this in my spare time. Since I was PTO for the last 2 weeks I havent looked at it. My latest code is up on reviewboard if someone wants to carry it on.
Flags: needinfo?(dburns)
Assignee: nobody → kyle
Ok, building on the excellent work done by :automatedtester, I've made a new patch that compiles and seems to pass all of the w-p-ts for ElementsFromPoint (I removed our meta file that turned those tests off). This is still a work in progress because I need to finish up the XPCOM side, which will really just be turning the nsTArray<RefPtr<Element>> into a nsIDOMNodeList**, which isn't that big of a deal. After that, I'll put this in for review.
Attachment #8683930 - Attachment is obsolete: true
Why do we need this to be exposed via XPCOM?
Comment on attachment 8708179 [details] [diff] [review] Patch 1 (v2) - WIP: Implement ElementsFromPoint Review of attachment 8708179 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +3377,5 @@ > + if (node && !node->IsElement()) { > + node = node->GetParent(); > + } > + if (node && node != lastAdded) { > + aElements.AppendElement(node->AsElement()); This can still return the same element twice. Something like this: <div id="outer" style="background:yellow"> <div id="inner" style="width:100px; height:100px; margin-top:-100px; background:lime;"></div> Hello </div> Your elementsFromPoint() on "Hello" would return "outer", "inner", "outer" AFAICT. The spec seems to say that boxes for non-elements should just be ignored, in which case you would return "outer", "inner", though who knows what browsers actually do. Certainly our elementFromPoint() implementation has the behavior you're copying here.
(In reply to Robert O'Callahan (:roc) (offline Jan 16-24) (Mozilla Corporation) from comment #16) > Why do we need this to be exposed via XPCOM? I was going to ask about that too, heh. I was just getting what was done to compile, while guessing that the XPCOM implementation was there because it also existed for ElementFromPoint. Not sure how we'd hit this from XPCOM now though? (In reply to Robert O'Callahan (:roc) (offline Jan 16-24) (Mozilla Corporation) from comment #17) > > Your elementsFromPoint() on "Hello" would return "outer", "inner", "outer" > AFAICT. The spec seems to say that boxes for non-elements should just be > ignored, in which case you would return "outer", "inner", though who knows > what browsers actually do. Certainly our elementFromPoint() implementation > has the behavior you're copying here. Ok. I'll add this case to tests, possibly as a w-p-t.
If there is a web exposed DOM API also in xpcom form, that is most probably because it was implemented before webidl bindings. New APIs don't need that, by default.
Release Note Request (optional, but appreciated) [Why is this notable]: We don't currently support documents.elementsFromPoint, while IE and Chrome do (I'm not sure if Opera and Safari have gotten around to it yet). It's making important things like IRCCloud angry. There's also an elementsFromRect, but I think I may wait on that since it doesn't even have a spec yet (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27837). [Suggested wording]: Added support for documents.elementsFromPoint [Links (documentation, blog post, etc)]: http://dev.w3.org/csswg/cssom-view/#dom-document-elementfrompoint *Estimated or target release*: Firefox 46, might see about uplifting farther if it lands quickly (w-p-t's at least pass with current patch)
relnote-firefox: --- → ?
(In reply to Robert O'Callahan (:roc) (offline Jan 16-24) (Mozilla Corporation) from comment #17) > This can still return the same element twice. > > Something like this: > <div id="outer" style="background:yellow"> > <div id="inner" style="width:100px; height:100px; margin-top:-100px; > background:lime;"></div> > Hello > </div> Ok, I tried to test this via jsfiddle in my local build: http://jsfiddle.net/r7awz6ym/ It doesn't actually seem to be returning the same element twice, but also doesn't seem to return "inner" at all when running elementsFromPoint on Hello. Should the "hello" and the "inner" div be overlapping somehow?
Flags: needinfo?(roc)
Attachment #8708179 - Attachment is obsolete: true
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #21) > (In reply to Robert O'Callahan (:roc) (offline Jan 16-24) (Mozilla > Corporation) from comment #17) > > This can still return the same element twice. > > > > Something like this: > > <div id="outer" style="background:yellow"> > > <div id="inner" style="width:100px; height:100px; margin-top:-100px; > > background:lime;"></div> > > Hello > > </div> > > Ok, I tried to test this via jsfiddle in my local build: Sorry, that should be 'margin-bottom' not 'margin-top'. We want the inner DIV background to cover the outer DIV background but render under 'Hello'.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > Sorry, that should be 'margin-bottom' not 'margin-top'. We want the inner > DIV background to cover the outer DIV background but render under 'Hello'. Ok, yeah, that made the problem you mentioned show up. Thanks!
Ok, looks like the original code for this came from NodesFromRectHelper, which wants ALL the nodes, not just the elements. So I /think/ we should be able to just continue when a node isn't an element and we'll be ok?
Attachment #8710776 - Attachment is obsolete: true
Attachment #8712374 - Flags: review?(roc)
Comment on attachment 8712374 [details] [diff] [review] Patch 1 (v4) - Implement ElementsFromPoint Review of attachment 8712374 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +3299,5 @@ > { > + nsAutoTArray<RefPtr<Element>, 1> elementArray; > + ElementsFromPointHelper(aX, aY, aIgnoreRootScrollFrame, aFlushLayout, > + true, elementArray); > + if (elementArray.Length() == 0) { elementArray.IsEmpty() @@ +3355,5 @@ > + > + for (uint32_t i = 0; i < outFrames.Length(); i++) { > + nsIContent* node = GetContentInThisDocument(outFrames[i]); > + > + if (node && !node->IsElement()) { if (!node || !node->IsElement()) @@ +3356,5 @@ > + for (uint32_t i = 0; i < outFrames.Length(); i++) { > + nsIContent* node = GetContentInThisDocument(outFrames[i]); > + > + if (node && !node->IsElement()) { > + continue; This seems to match what Chrome does. This changes our behavior for elementFromPoint(), I guess, in the testcase I suggested. Please check that our new behavior matches Chrome's elementFromPoint on that testcase and add a test for it. I'm worried that there are other cases where an element has multiple overlapping display items, all acting as a hit-target, with other element display items interleaved. But right now I can't think of a case where that could happen, so let's set that aside.
Attachment #8712374 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26) @ +3356,5 @@ > > + for (uint32_t i = 0; i < outFrames.Length(); i++) { > > + nsIContent* node = GetContentInThisDocument(outFrames[i]); > > + > > + if (node && !node->IsElement()) { > > + continue; > > This seems to match what Chrome does. This changes our behavior for > elementFromPoint(), I guess, in the testcase I suggested. Please check that > our new behavior matches Chrome's elementFromPoint on that testcase and add > a test for it. Ok, actually, elementFromPoint and elementsFromPoint return different top elements in Chrome, so my change here is incorrect. Looks like that behavior is laid out in the spec, in the note portion of elementFromPoint (https://drafts.csswg.org/cssom-view/#dom-document-elementfrompoint). So, I need to keep what we were doing there originally. Will fix and make a new patch.
Changed logic since we deal with elementFromPoint and elementsFromPoint differently. Not much of a code change, but want to make sure my explanation comments make sense to someone other than me.
Attachment #8712374 - Attachment is obsolete: true
Attachment #8713324 - Flags: review?(roc)
Filed followup for elementsFromRect
Summary: Support elementsFromPoint (and elementsFromRect?) → Support elementsFromPoint
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #27) > Ok, actually, elementFromPoint and elementsFromPoint return different top > elements in Chrome, so my change here is incorrect. Looks like that behavior > is laid out in the spec, in the note portion of elementFromPoint > (https://drafts.csswg.org/cssom-view/#dom-document-elementfrompoint). I don't see how that applies to you.
Comment on attachment 8713324 [details] [diff] [review] Patch 1 (v5) - Implement ElementsFromPoint Review of attachment 8713324 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.h @@ +1036,5 @@ > + > + virtual void ElementsFromPointHelper(float aX, float aY, > + bool aIgnoreRootScrollFrame, > + bool aFlushLayout, > + bool aIsElementFromPoint, Multiple boolean parameters are evil. Please, let's use a flags word with named flags for this.
Attachment #8713324 - Flags: review?(roc)
Comment on attachment 8713325 [details] [diff] [review] Patch 2 (v1) - Web Platform Test for elementFromPoint/elementsFromPoint dealing with negative margins Review of attachment 8713325 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/cssom-view/negativeMargins.html @@ +17,5 @@ > + test(function () { > + assert_array_equals(document.elementFromPoint(outerRect.left + 10, > + outerRect.top + 10), > + outer, > + "elementFromPoint should get outer element"); Make these + 1 instead of + 10. Something could go wrong if the Hello font is very small.
Attachment #8713325 - Flags: review?(roc) → review+
Flag-ized booleans
Attachment #8713324 - Attachment is obsolete: true
Attachment #8713900 - Flags: review?(roc)
Attachment #8713900 - Flags: review?(khuey)
Comment on attachment 8713900 [details] [diff] [review] Patch 1 (v6) - Implement ElementsFromPoint Approval Request Comment [Feature/regressing bug #]: None [User impact if declined]: currently elementsFromPoint has to happen via polyfill, which is very slow. For instance, impacts IRCCloud performance. Putting in for uplift all the way to beta since we're early in the cycle. [Describe test coverage new/current, TreeHerder]: Passed try, passes all web-platform-test tests [Risks and why]: None [String/UUID change made/needed]: None
Attachment #8713900 - Flags: approval-mozilla-beta?
Attachment #8713900 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/512497f6a7ed https://hg.mozilla.org/mozilla-central/rev/64ee9c353c06
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8713900 [details] [diff] [review] Patch 1 (v6) - Implement ElementsFromPoint Adds new tests, improves performance, adds support for elementsFromPoint. Please uplift to aurora.
Attachment #8713900 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8713901 [details] [diff] [review] Patch 2 (v2) - Web Platform Test for elementFromPoint/elementsFromPoint dealing with negative margins; r=roc See Comment 38 (forgot to mark the tests for uplift too)
Attachment #8713901 - Flags: approval-mozilla-beta?
Attachment #8713901 - Flags: approval-mozilla-aurora?
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #41) > See Comment 38 (forgot to mark the tests for uplift too) Tests don't need approval, see https://wiki.mozilla.org/Tree_Rules > Exception: If patches only make changes to tests, test harnesses or anything else > that does not affect the shipped builds, they may land with self approval > (use a=testonly, a=npotb etc).
Comment on attachment 8713901 [details] [diff] [review] Patch 2 (v2) - Web Platform Test for elementFromPoint/elementsFromPoint dealing with negative margins; r=roc Not taking this to beta. This is a new feature, we don't take this kind of changes in beta.
Attachment #8713901 - Flags: approval-mozilla-beta?
Attachment #8713901 - Flags: approval-mozilla-beta-
Attachment #8713901 - Flags: approval-mozilla-aurora?
Attachment #8713901 - Flags: approval-mozilla-aurora+
Attachment #8713900 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
the test has problems uplifting to aurora, could you take a look, thanks !
Flags: needinfo?(kyle)
hey kyle i got errors like : applying 0001-Bug-1164427-Add-w-p-t-for-elementFromPoint-elementsF.patch file testing/web-platform/tests/cssom-view/negativeMargins.html already exists 1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/cssom-view/negativeMargins.html.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 0001-Bug-1164427-Add-w-p-t-for-elementFromPoint-elementsF.patch Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ while trying to uplift the 2nd patch to aurora
Flags: needinfo?(kyle)
Ok, looks like we're hitting uplift timing issues, since that manifest changes frequently. I'll fix it again and just uplift it myself.
Flags: needinfo?(kyle)
Noted for beta 46, "Added support for documents.elementsFromPoint"
relnote-firefox: ? → 46+
I opened issue 1259226 which demonstrates lag with this api on scroll.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: