Last Comment Bug 501421 - Implement getIntersectionList(), getEnclosureList()
: Implement getIntersectionList(), getEnclosureList()
Status: NEW
: dev-doc-needed
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- normal with 23 votes (vote)
: ---
Assigned To: Felipe Corrêa da Silva Sanches
:
Mentors:
https://svgwg.org/svg2-draft/struct.h...
: 589646 (view as bug list)
Depends on:
Blocks: ietestcenter 1262352
  Show dependency treegraph
 
Reported: 2009-06-30 11:17 PDT by Jeff Schiller
Modified: 2016-05-03 19:26 PDT (History)
28 users (show)
dmu: needinfo? (juca)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case revealing the problem. (1.12 KB, image/svg+xml)
2009-06-30 11:17 PDT, Jeff Schiller
no flags Details
interactive test case (1.45 KB, image/svg+xml)
2010-09-15 16:09 PDT, Felipe Corrêa da Silva Sanches
no flags Details
wip. Need some advice please (7.05 KB, patch)
2010-09-15 16:24 PDT, Felipe Corrêa da Silva Sanches
no flags Details | Diff | Review
wip. Works but only with simple boundingbox checks (8.42 KB, patch)
2010-09-16 10:03 PDT, Felipe Corrêa da Silva Sanches
jwatt: feedback-
Details | Diff | Review
more complete testcase for getIntersectionList (340 bytes, image/svg+xml)
2013-02-25 10:51 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details
more complete testcase for getIntersectionList (2.78 KB, image/svg+xml)
2013-02-25 10:54 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details

Description Jeff Schiller 2009-06-30 11:17:11 PDT
Created attachment 386062 [details]
Test case revealing the problem.

As mentioned here: http://www.mozilla.org/projects/svg/status.html please support the following methods on the <svg> DOM element:

getIntersectionList()
getEnclosureList()
checkIntersection()
checkClosure()

Spec details here: http://www.w3.org/TR/SVG11/struct.html#InterfaceSVGSVGElement

Opera supports these methods.

I have attached a very basic test case that covers getIntersectionList and getEnclosureList.
Comment 1 Helder "Lthere" Magalhães 2009-07-24 09:20:41 PDT
Reading that that Firefox 3.5- didn't support these methods [1], I got the impression that this was working post 3.5, and quickly launched my nightly build (3.6a [2])... But apparently not yet. :-|

[1] http://code.google.com/p/doctype/wiki/SVGSVGElement
[2] Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090724 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Comment 2 Lars Gunther 2010-04-03 02:32:54 PDT
Shouldn't this bug be for all platforms?
Comment 3 Helder "Lthere" Magalhães 2010-04-03 06:16:15 PDT
(In reply to comment #2)
> Shouldn't this bug be for all platforms?

Yup. :-)
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-08-22 23:52:55 PDT
*** Bug 589646 has been marked as a duplicate of this bug. ***
Comment 5 Felipe Corrêa da Silva Sanches 2010-09-15 16:09:09 PDT
Created attachment 475682 [details]
interactive test case

This is a testcase in which you move a rectangle by moving the mouse cursor. Fill color indicates whether the rect is enclosed by the box (another svg:rect). Stroke indicates whether the rect intersects the box.
Comment 6 Felipe Corrêa da Silva Sanches 2010-09-15 16:24:11 PDT
Created attachment 475692 [details] [diff] [review]
wip. Need some advice please

This is work in progress. This patch works partially. With this patch the test case (475682) passes. But this patch is still incomplete. It is my first attempt at solving a non-trivial bug, so I'd like to have feedback on it. I am not sure I am doing things the best way.

some questions:

1) referenceElement is the root of a subtree for which every node should have its bbox checked against the provided rect for enclosure or intersection, right?

2) what is the precise definition of intersetion? I guess that an element that is intersecting rect must not be enclosed by rect at the same time, but that is not clear in the SVG spec.

3) when referenceElement is nsnull we should use the svg:svg element as root of the subtree to be searched for enclosure/intersections?

Please, give me some feedback on overall code quality / coding style.
Comment 7 Jeff Schiller 2010-09-15 16:32:34 PDT
It's great that you're working on this - these methods are invaluable, in my opinion (equivalent of getElementsByClassName() etc for SVG).

For 1), please follow up with this issue I raised last year: http://www.w3.org/Graphics/SVG/WG/track/actions/2695 it looks like the spec was resolved and tests added.  Unfortunately I don't remember how it was resolved :/
Comment 8 Jeff Schiller 2010-09-15 16:40:55 PDT
Here's the thread I started on www-svg: http://lists.w3.org/Archives/Public/www-svg/2009Nov/0015.html
Comment 9 Daniel Holbert [:dholbert] 2010-09-15 16:44:57 PDT
Comment on attachment 475692 [details] [diff] [review]
wip. Need some advice please

I'm not familiar with the APIs being implemented in this bug, so I'd defer to jwatt or longsonr on giving feedback on this.  Also, I'm changing "review?" to "feedback?", since (as you said) this isn't done yet & you're just looking for feedback.

Since you asked, here are some coding-style nits:

>+void
>+nsSVGSVGElement::evaluateList(PRBool enclosure_test, nsIContent* node,
>+                           nsBaseContentList* contentList,
>+                           gfxRect* ref){

Mozilla coding style is to have function names be capitalized in  C++, so "evaluateList" should be capitalized.

Also (though we don't follow this everywhere), function-arguments should generally be prefixed with an "a" and be camelCase, not underscore_separated. (e.g. "aEnclosureTest", rather than "enclosure_test")

And the 2nd & 3rd lines quoted above need two more spaces of indentation to line up.

>+      if (ref->Contains(nodeBBox)){
>+        contentList->AppendElement(node);
>+      } else {
>+        return;
>+      }

>+  nsCOMPtr<nsINode> _node = do_QueryInterface(node);

Rename "node" to |aContent| or |aElement| (since it's a nsIContent*), and rename "_node" to "node".

>+  for (i=0; i < length; i++){

Add a space on either side of the "=".

>+  nsBaseContentList* contentList = new nsBaseContentList();
>+  NS_ENSURE_TRUE(contentList, NS_ERROR_OUT_OF_MEMORY);

"new" never fails in Mozilla code now (or rather, it aborts instead of failing), so there's no need to check for out-of-memory.

>+  void evaluateList(PRBool enclosure_test, nsIContent* node, nsBaseContentList* contentList,
>+                           gfxRect* ref);
>+

This line is too long -- please wrap to <80 characters. (also, the indentation is off on the second line)
Comment 10 Jeff Schiller 2010-09-15 16:46:04 PDT
1) And checking SVG 1.1 second edition: http://www.w3.org/TR/2010/WD-SVG11-20100622/struct.html#__svg__SVGSVGElement__getIntersectionListIt says that referenceElement: "If not null, then any intersected element that doesn't have the referenceElement as ancestor must not be included in the returned NodeList."2) My interpretation of "intersects" is in the mathematical sense: If an element's bbox overlaps in any way with the argument rectangle.3) I would agree with your interpretation, another way to interpret this is when referenceElement is null, all elements are checked (so start at <svg>).
Comment 11 Felipe Corrêa da Silva Sanches 2010-09-15 17:38:16 PDT
hmmm... It seems that the implementation of these functions is much more complicated than I had thought! I was considering only intersections of bounding boxes. We are really talking about intersection of the verctors themselves? That is certainly more complicated...
Comment 12 Felipe Corrêa da Silva Sanches 2010-09-16 07:58:24 PDT
(In reply to comment #9)
> Also (though we don't follow this everywhere), function-arguments should
> generally be prefixed with an "a" and be camelCase, not underscore_separated.
> (e.g. "aEnclosureTest", rather than "enclosure_test")

Does this rule apply also to _retval ? should it be aRetVal instead or is it a special case where we prefer _lowercase ?
Comment 13 Daniel Holbert [:dholbert] 2010-09-16 08:55:42 PDT
Mm, good question - "_retval" should be all-lowercase, according to an MXR search I just did.  I'm not sure why, but that's the convention. :)
Comment 14 Felipe Corrêa da Silva Sanches 2010-09-16 10:03:44 PDT
Created attachment 475884 [details] [diff] [review]
wip. Works but only with simple boundingbox checks

This is an updated version of the previous patch.
It has some refactoring work and it is closer to the mozilla coding style.

I have added 2 auxiliary methods to check for enclosure and for intersection of an element (nsIContent*) with a given gfxRect. These methods are currently only checking for enclosure and intersection of the bounding box of the element. These must be improved to also consider the pointer-events criteria.

I need feedback on:

1) proper iteration of the subtree (I am not sure about the correctness of my implementation)
2) how to get a reference for <svg> root element (for the case when referenceElement is nsnull)
3) how to proceed to implement the proper pointer-events criteria
Comment 15 Jeff Schiller 2010-09-16 12:09:58 PDT
I wrote a simple test for Opera this morning and they don't use bounding boxes for getIntersectionList()
Comment 16 karthik.eleven 2011-12-22 23:28:53 PST
Any chance this bug could get some love? Been open for a couple of years now.

I agree with Jeff's testing as far as Opera is concerned. Opera's implementation could thus be used as a reference.
Comment 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2013-02-25 10:51:30 PST
Created attachment 717967 [details]
more complete testcase for getIntersectionList

Here is a more complete testcase. This checks complications such as non-rectilinear shapes, transforms, filters, stroke, pointer-events, marker, clipPath and mask.

The fill of any path elements that are included in the list returned by getIntersectionList will turn green.

I've not tested IE, but Webkit and Opera behave very differently.
Comment 18 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2013-02-25 10:54:00 PST
Created attachment 717969 [details]
more complete testcase for getIntersectionList
Comment 19 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2013-02-25 10:58:53 PST
Comment on attachment 475884 [details] [diff] [review]
wip. Works but only with simple boundingbox checks

Things that are unclear in the spec:

 * what "rendered content" means

 * what the effect of non-rectilinear shapes, transforms, filters, stroke,
   pointer-events, marker, clipPath and mask should be

 * what "Each candidate graphics element is to be considered a match only
   if the same graphics element can be a target of pointer events as
   defined in ‘pointer-events’ processing" means. (Opera seems to take this
   to mean something like "points that render that would also react to
   pointer events" contribute, whereas Webkit only seems to pay attention
   to whether or not pointer-events="none".

I think at least these points need to be clarified before there's any point reviewing a patch, since the answers will greatly influence the patch.
Comment 20 est.risto 2014-05-22 07:33:24 PDT
There seems to be problems with getIntersectionList in FireFox 29, seems like it's currently not included ?
Comment 21 l.brandon63 2015-02-25 15:10:34 PST Comment hidden (advocacy)
Comment 22 philippe.carret 2015-07-17 06:56:16 PDT
This does not seems to be implemented in Nightly build 42.0a1

test FF (fails)  Chrome IE (succeeded)
http://www.xn--dahlstrm-t4a.net/svg/interactivity/intersection/sandbox_hover.svg


Any confirmation ?
Comment 23 Paul LeBeau 2015-07-22 19:17:25 PDT
Test case for these and the other related methods (elementFromPoint, elementsFromPoint, etc)

http://jsfiddle.net/y0mthrd5/2/

FF is the last major browser not to have implemented these.
Comment 24 Daniel Trebbien 2015-07-23 05:33:31 PDT
Although Chrome and Safari support getIntersectionList(), their implementation has a bug which severely limits the usefulness of the API:
- http://crbug.com/370012
- https://bugs.webkit.org/show_bug.cgi?id=81137
Comment 25 Angelo Borsotti 2015-11-22 23:32:35 PST Comment hidden (advocacy)
Comment 26 Daosheng Mu[:daoshengmu] 2016-04-28 19:58:11 PDT
Hi, Felipe Corrêa da Silva Sanches. If you don't mind, I would like to take over this bug.
Comment 27 Robert Longson 2016-04-28 23:04:43 PDT
It seems we're waiting for answers to comment 19. Do you have them?
Comment 28 Daosheng Mu[:daoshengmu] 2016-05-03 19:26:33 PDT
Not yet really. I will think about it

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