Bug 13981 - <br> prevents click handler from firing
Summary: <br> prevents click handler from firing
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
Depends on: 12207
  Show dependency treegraph
Reported: 2007-06-03 05:42 PDT by Antoine Quint
Modified: 2007-06-19 02:16 PDT (History)
1 user (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
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.

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.

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.