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+

Antoine Quint

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

Attachments
Test case for <br> before an <svg> element (624 bytes, application/xml) 2007-06-03 05:43 PDT, Antoine Quint	no flags	Details
Render tree comparisons (left=<p/><br/><svg>, right=<p/><br/><div/><svg>) (103.59 KB, image/png) 2007-06-04 11:33 PDT, Eric Seidel (no email)	no flags	Details
render tree for broken case (45.67 KB, image/png) 2007-06-04 21:45 PDT, Eric Seidel (no email)	no flags	Details
First attempt (27.26 KB, patch) 2007-06-17 05:54 PDT, Rob Buis	eric: review-	Details Formatted Diff Diff
Addressing Eric's issue (27.53 KB, patch) 2007-06-19 00:34 PDT, Rob Buis	oliver: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Antoine Quint

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

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

Eric Seidel (no email)

Comment 2 2007-06-04 11:23:56 PDT

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

Eric Seidel (no email)

Comment 3 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>

Eric Seidel (no email)

Comment 4 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.

Eric Seidel (no email)

Comment 5 2007-06-04 11:33:40 PDT

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

Eric Seidel (no email)

Comment 6 2007-06-04 11:34:42 PDT

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

Eric Seidel (no email)

Comment 7 2007-06-04 21:45:55 PDT

Created attachment 14857 [details] render tree for broken case

Eric Seidel (no email)

Comment 8 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).

Rob Buis

Comment 9 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.

Eric Seidel (no email)

Comment 10 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.

Rob Buis

Comment 11 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.

Oliver Hunt

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

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

Rob Buis

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

Landed in r23583.

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