Bug 10587 - pointer-events is not implemented for RenderSVGImage or RenderSVGText
Summary: pointer-events is not implemented for RenderSVGImage or RenderSVGText
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/SVG/interact.htm...
Keywords:
Depends on: 10415
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-27 00:08 PDT by Eric Seidel (no email)
Modified: 2007-01-08 08:06 PST (History)
2 users (show)

See Also:


Attachments
First attempt (191.71 KB, patch)
2006-08-27 12:32 PDT, Rob Buis
hyatt: review-
Details | Formatted Diff | Diff
Different approach (196.78 KB, patch)
2006-08-29 02:02 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Different approach II (201.98 KB, patch)
2006-08-29 02:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Revived patch (293.79 KB, patch)
2007-01-05 15:34 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Improved testcase + patch (231.60 KB, patch)
2007-01-06 08:30 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Improved testcase (236.61 KB, patch)
2007-01-07 07:29 PST, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-08-27 00:08:31 PDT
pointer-events is not implemented for RenderSVGImage or RenderSVGText

I implemented the pointer-events property as part of:
http://bugzilla.opendarwin.org/show_bug.cgi?id=10415

but I only did so for RenderPath.  

I designed my implementation such that someone now would just need to pull the PointerEventsHitRules struct down into RenderObject.h (or its own header) and implement pointerEventsHitRules() methods for RenderSVGImage and RenderSVGText and then use similar logic to what is found in RenderPath::nodeAtPoint to wire it all together.
Comment 1 Rob Buis 2006-08-27 12:32:58 PDT
Created attachment 10260 [details]
First attempt

I am not sure whether we need to do the hit testing per pixel right now?
Cheers,

Rob.
Comment 2 Eric Seidel (no email) 2006-08-27 13:35:58 PDT
Comment on attachment 10260 [details]
First attempt

So this is different than I had intended, but not necessarily a "wrong" way to do this.

I had not intended for pointerEventsHitRules() to be pulled down into RenderObject, but rather for RenderSVGText and RenderSVGImage to write their own variant of the function.

I had however intended PointerEventsHitRules to be pulled down (or at least pulled out of RenderPath).

PointerEventsHitRules should be wrapped inside of SVG_SUPPORT blocks.

I'd be happy to discuss it with you, but I think that having a RenderSVGImage::pointerEventsHitRules() which made it clear how each of the pointer-events hit rules for RenderImage worked, would be helpful.  I'm not sure that PointerEventsHitRules will be sufficient for capturing the exact data of what is/is-not required for a successful "hit" for RenderImage though. I'd have to look at the spec more to be sure.
Comment 3 Dave Hyatt 2006-08-27 14:03:14 PDT
Comment on attachment 10260 [details]
First attempt

I do not want PointerEventsHitRules in RenderObject.h.
Comment 4 Rob Buis 2006-08-29 02:02:33 PDT
Created attachment 10293 [details]
Different approach

Here is the approach using MI. Note that the code that is commented out will be fixed soon, I think
it can be removed, but we may need it if we test against pixels in future. Also maybe a test for text
is needed?
Cheers,

Rob.
Comment 5 Rob Buis 2006-08-29 02:07:19 PDT
Created attachment 10294 [details]
Different approach II

I forgot the new interface.
Cheers,

Rob.
Comment 6 Eric Seidel (no email) 2006-09-25 03:34:39 PDT
did you mean to mark this for review rob?
Comment 7 Andreas Neumann 2007-01-04 13:14:14 PST
Here is another testcase:

http://www.carto.net/neumann/webkitsvgbugs/pointer-events_test.svg
Comment 8 Rob Buis 2007-01-05 15:34:46 PST
Created attachment 12249 [details]
Revived patch

I revivied this patch, because it would make some of the carto.net examples work much nicer.
Probably the methods fillContains/strokeContains need to be implemented somehow for RenderSVGText.
The text hit testing testcase is probably a bit lame but it does test the functionality. It will probably need finetuning once above methods work.
Cheers,

Rob.
Comment 9 Rob Buis 2007-01-06 08:30:00 PST
Created attachment 12258 [details]
Improved testcase + patch

This patch should be much better. The results mostly match FF and Opera. The testcases now make
more sense IMHO, for text and image we just have hit/miss, since stroke and fill do not need to be
seperately shown like for paths.
Cheers,

Rob.
Comment 10 Eric Seidel (no email) 2007-01-06 12:35:32 PST
Comment on attachment 12258 [details]
Improved testcase + patch

-    : RenderObject(node)
+    : RenderObject(node), PointerEventsInterface()

separate line for each constructor

pointerEventsHItRules is yet another reason why it would be nice to have some sort of SVG base class for all the SVG renderers.

Maybe it makes sense to just have the pointer events logic all in PointerEventsHitRules (making it a class).  then you'd just do something like:

PointerEventHitRules(kSVGImageRules, style()->svgStyle()->pointerEvents())

It seems sorta odd for RenderPath to inherit from PointerEventsInterface, since no one ever uses that interface except for internal RenderPath methods.  It's also not as though the hit rules are bound to any specific type, so much as all the various element types subscripe to some set of hit rules.  Just a thought.

Overall the patch looks fine.  Lets chat about this briefly over IRC though.
Comment 11 Rob Buis 2007-01-07 07:29:08 PST
Created attachment 12276 [details]
Improved testcase

This patch should address Eric's issues.
Cheers,

Rob.
Comment 12 Eric Seidel (no email) 2007-01-07 22:41:47 PST
Comment on attachment 12276 [details]
Improved testcase

looks good.

the one xlink:href change isn't needed though.
Comment 13 Rob Buis 2007-01-08 00:32:19 PST
Landed in r18659.
Comment 14 David Kilzer (:ddkilzer) 2007-01-08 03:16:32 PST
(In reply to comment #13)
> Landed in r18659.

This caused two layout test failures/changes:

svg/custom/pointer-events-path
svg/custom/pointer-events-text

http://build.webkit.org/results/post-commit-powerpc-mac-os-x/5002/

Comment 15 David Kilzer (:ddkilzer) 2007-01-08 08:06:31 PST
(In reply to comment #14)
> (In reply to comment #13)
> > Landed in r18659.
> 
> This caused two layout test failures/changes:
> 
> svg/custom/pointer-events-path
> svg/custom/pointer-events-text
> 
> http://build.webkit.org/results/post-commit-powerpc-mac-os-x/5002/

Fixed in r18665.