Bug 10587 – pointer-events is not implemented for RenderSVGImage or RenderSVGText

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

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.

[Rob Buis]

Created attachment 10260 [details]
First attempt

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

Rob.

[Eric Seidel (no email)]

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.

[Dave Hyatt]

Comment on attachment 10260 [details]
First attempt

I do not want PointerEventsHitRules in RenderObject.h.

[Rob Buis]

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.

[Rob Buis]

Created attachment 10294 [details]
Different approach II

I forgot the new interface.
Cheers,

Rob.

[Eric Seidel (no email)]

did you mean to mark this for review rob?

[Andreas Neumann]

Here is another testcase:

http://www.carto.net/neumann/webkitsvgbugs/pointer-events_test.svg

[Rob Buis]

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.

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Rob Buis]

Created attachment 12276 [details]
Improved testcase

This patch should address Eric's issues.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 12276 [details]
Improved testcase

looks good.

the one xlink:href change isn't needed though.

[Rob Buis]

Landed in r18659.

[David Kilzer (:ddkilzer)]

(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/

[David Kilzer (:ddkilzer)]

(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.