Bug 13981

Summary:

<br> prevents click handler from firing

Product:

WebKit

Reporter:

Antoine Quint <ml>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt

Priority:

Version:

523.x (Safari 3)

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

12207

Bug Blocks:

Attachments:

Description	Flags
Test case for <br> before an <svg> element	none
Render tree comparisons (left=<p/><br/><svg>, right=<p/><br/><div/><svg>)	none
render tree for broken case	none
First attempt	eric: review-
Addressing Eric's issue	oliver: review+

Description Antoine Quint 2007-06-03 05:42:43 PDT

A click event handler on an SVG element positioned after an XHTML <br /> element does not get fired. Removing the <br /> makes the event handler work. See test case.

Comment 1 Antoine Quint 2007-06-03 05:43:28 PDT

Created attachment 14847 [details]
Test case for <br> before an <svg> element

Comment 2 Eric Seidel (no email) 2007-06-04 11:23:56 PDT

Well, clearly the <br> is overlapping with the <svg> and capturing the click.

Comment 3 Eric Seidel (no email) 2007-06-04 11:27:25 PDT

If you start the doc <body><br /><svg> the problem does not occur.  So it seems to be an interaction between the <p> and the <br> which produces this <br /> overlap with the <svg>

Comment 4 Eric Seidel (no email) 2007-06-04 11:29:10 PDT

More info:  Using a <div> instead of an SVG does not show the bug.  I expect that RenderSVGContainer is just not implementing some render tree method correctly.

Comment 5 Eric Seidel (no email) 2007-06-04 11:33:40 PDT

Created attachment 14854 [details]
Render tree comparisons (left=<p/><br/><svg>, right=<p/><br/><div/><svg>)

Comment 6 Eric Seidel (no email) 2007-06-04 11:34:42 PDT

Hyatt might just know off the top of his head what's up here.

Comment 7 Eric Seidel (no email) 2007-06-04 21:45:55 PDT

Created attachment 14857 [details]
render tree for broken case

Comment 8 Eric Seidel (no email) 2007-06-04 22:24:12 PDT

Well, I think we're applying the _tx, _ty twice.  Possibly because we're doing it once in nodeAtPoint and a second time using absoluteTransform() (from inside mapAbsolutePointToLocal()).

IMO the best way to go about fixing this is to cleanly divide the SVG render tree from the HTML render tree by finally separating out the "in HTML" code from RenderSVGContainer into a RenderSVGRoot class or similar.  http://bugs.webkit.org/show_bug.cgi?id=12207 covers doing such.

There might also be an easy way to fix this, I'm not sure.  absoluteTransform() application is necessary to handle rotation etc.  applying _tx, _ty is necessary due to handle scrolling.  Again, I think this type of bug mostly just occurs due to the strange intermixing of HTML and SVG way of doing things in RenderSVGContainer (since RSC can be inside either html or svg content).

Comment 9 Rob Buis 2007-06-17 05:54:57 PDT

Created attachment 15084 [details]
First attempt

This should fix hit testing. Also the rects in the window inspector should be more accurate for svg renderers now.
Cheers,

Rob.

Comment 10 Eric Seidel (no email) 2007-06-18 10:08:50 PDT

Comment on attachment 15084 [details]
First attempt

This line needs to be made more understandable:
-        if (child->nodeAtPoint(request, result, _x, _y, _tx, _ty, hitTestAction)) {
+        if (child->nodeAtPoint(request, result, _x - (_tx - ctm.e()), _y - (_ty - ctm.f()), 0, 0, hitTestAction)) {

by breaking it out into more lines with better names.

The code otherwise looks fine.

Comment 11 Rob Buis 2007-06-19 00:34:59 PDT

Created attachment 15114 [details]
Addressing Eric's issue

This one has a clear name for the calculated scroll offets.
Cheers,

Rob.

Comment 12 Oliver Hunt 2007-06-19 00:46:25 PDT

Comment on attachment 15114 [details]
Addressing Eric's issue

looks fine

Comment 13 Rob Buis 2007-06-19 02:16:20 PDT

Landed in r23583.