Bug 34714 - onclick is not reliable for transformed SVG elements
Summary: onclick is not reliable for transformed SVG elements
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Hans Muller
Depends on:
Reported: 2010-02-08 11:33 PST by Bill Lipa
Modified: 2012-04-17 12:23 PDT (History)
8 users (show)

See Also:

test case (716 bytes, application/xhtml+xml)
2010-02-08 11:33 PST, Bill Lipa
no flags Details
Patch (6.04 KB, patch)
2012-02-03 09:23 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2012-02-03 09:35 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (33.29 KB, patch)
2012-02-16 14:35 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (33.26 KB, patch)
2012-02-17 16:19 PST, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Lipa 2010-02-08 11:33:17 PST
Created attachment 48347 [details]
test case

The onclick javascript handler does not fire under some common conditions with SVG elements.  I have created a minimal self-contained test case which is attached.

The test case displays two SVG rectangles.  On the left rectangle, using the currently released Safari and today's nightly build, no click is detected.  On the right rectangle, the click is detected.  The only difference is the transform on the left rectangle element.  The transform correctly adjusts the display, but interferes incorrectly with click detection.

Both clicks are detected correctly in Firefox.
Chrome has the same behavior as Safari.
Comment 1 Bill Lipa 2010-03-12 09:18:36 PST
This is still broken in Safari 4.0.5.
Comment 2 Dirk Schulze 2010-05-31 13:26:50 PDT
I tested it with another coordinate base than 'cm' and it worked. So maybe this is not a problem of hittesting itself. Also do the results differ between Safari Chrome and WebKitGtk, but shouldn't. Maybe the coordinates get to small during the mapping to the local transform, so that this is caused by rounding problems. Needs further testing.
Comment 3 Hans Muller 2012-02-03 09:23:01 PST
Created attachment 125349 [details]
Comment 4 WebKit Review Bot 2012-02-03 09:27:25 PST
Attachment 125349 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1

LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 6 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Dirk Schulze 2012-02-03 09:28:28 PST
Comment on attachment 125349 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=125349&action=review

r- because of style.

> Source/WebCore/ChangeLog:8
> +	Use FloatPoints in RenderSVGRoot::nodeAtPoint() when converting the incoming point to local coordinates.

No tabs at all please.

> LayoutTests/ChangeLog:8
> +	Checks that elementFromPoint() works correctly with SVG elements defined within a 1x1 viewbox.


> LayoutTests/svg/hittest/script-tests/svg-floating-point-transform.js:6
> +        {x:1, y:1},
> +        {x:1, y:399},
> +        {x:399, y:1},

Indentation should be 4 spaces. For tests as well. Also make a space between ':' and the following number.
Comment 6 Hans Muller 2012-02-03 09:35:48 PST
Created attachment 125353 [details]
Comment 7 WebKit Review Bot 2012-02-03 10:09:08 PST
Comment on attachment 125353 [details]

Attachment 125353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11416407

New failing tests:
Comment 8 Dirk Schulze 2012-02-03 14:15:12 PST
Comment on attachment 125353 [details]

r=me. I don't see a relation to the bugs on the bug report and the detailed list of the build bot. I assume that the build list is the one we should look at. And I don't sees a failing SVG test.

To the test: you shouldn't generate the html file on yourself. This gets generated by a script called Tools/Scripts/make-script-test-wrappers (you have to call it manually. It is located in Tools/Scripts).

Instead you should create the elements dynamically in your script. Would be great if you can change the test. Please see also at LayoutTests/svg/dynamic-updates/ for examples.
Comment 9 Dirk Schulze 2012-02-03 14:16:08 PST
(In reply to comment #8)
> (From update of attachment 125353 [details])
> r=me.

The r=me was wrong. Sorry, not sure why it is in the text.
Comment 10 Hans Muller 2012-02-16 14:35:40 PST
Created attachment 127444 [details]
Comment 11 Hans Muller 2012-02-16 14:39:12 PST
I've uploaded changes to the expected txt and png files for the select-textLength-spacing-squeeze-2 test for the mac platform only.   Not sure how to cause those files to be generated for the other platforms.
Comment 12 WebKit Review Bot 2012-02-16 18:24:34 PST
Comment on attachment 127444 [details]

Attachment 127444 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542186

New failing tests:
Comment 13 Nikolas Zimmermann 2012-02-17 07:26:46 PST
Comment on attachment 127444 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=127444&action=review

The failing tests need to be marked in platform/chromium/test_expectations.txt.
BUGWK<YOURBUG>: svg/your/file.svg = IMAGE+TEXT
or = IMAGE (depending on if you expect image+text to fail, or only the image).

The usual process for generating baselines is:
* Amend chromium expectations, to paint the cr-linux EWS green
* Include test results generated on Lion, inclduing pixel test baselines, if needed
* Land your patch, wait for Gtk/Qt/Win to cycle, then use "webkit-patch rebaseline" to grab the new results from these platforms, then land them as follow-up patch.

r- because the test won't work in trunk anymore, due recent changes:

> LayoutTests/svg/hittest/script-tests/svg-small-viewbox.js:54
> +startTest(rootSVGElement, 100, 100);

The testcase is outdated, coded against a version before we switched to the repaint.js harness. I fear you have to upate it again.
Look at how svg/<xxx>/script-tests/foobar.js tests look now in trunk (no more startTest, executeTest renamed to repaintTest).
You also have to include repaint.js in your .html file now.
Comment 14 Hans Muller 2012-02-17 16:19:51 PST
Created attachment 127671 [details]
Comment 15 Hans Muller 2012-02-17 16:23:11 PST
I've revised the new test, svg/hittest/svg-small-viewbox.xhtml, to look as much the other hittests as possible.   It no longer depends on any shared scripts, and it's just one file.
Comment 16 WebKit Review Bot 2012-02-24 15:49:30 PST
Comment on attachment 127671 [details]

Clearing flags on attachment: 127671

Committed r108857: <http://trac.webkit.org/changeset/108857>
Comment 17 WebKit Review Bot 2012-02-24 15:49:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Stephen Chenney 2012-04-17 12:23:50 PDT
Committed r114413: <http://trac.webkit.org/changeset/114413>