Bug 34714 – onclick is not reliable for transformed SVG elements

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

[Bill Lipa]

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.

[Bill Lipa]

This is still broken in Safari 4.0.5.

[Dirk Schulze]

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.

[Hans Muller]

Created attachment 125349 [details]
Patch

[WebKit Review Bot]

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.

[Dirk Schulze]

Comment on attachment 125349 [details]
Patch

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.

Ditto.

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

[Hans Muller]

Created attachment 125353 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125353 [details]
Patch

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

New failing tests:
svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg
svg/text/select-textLength-spacing-stretch-3.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg
svg/text/select-x-list-2.svg
svg/text/select-x-list-with-tspans-2.svg

[Dirk Schulze]

Comment on attachment 125353 [details]
Patch

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.

[Dirk Schulze]

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

[Hans Muller]

Created attachment 127444 [details]
Patch

[Hans Muller]

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.

[WebKit Review Bot]

Comment on attachment 127444 [details]
Patch

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

New failing tests:
svg/text/select-textLength-spacing-squeeze-2.svg
svg/hittest/svg-small-viewbox.xhtml

[Nikolas Zimmermann]

Comment on attachment 127444 [details]
Patch

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.

[Hans Muller]

Created attachment 127671 [details]
Patch

[Hans Muller]

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.

[WebKit Review Bot]

Comment on attachment 127671 [details]
Patch

Clearing flags on attachment: 127671

Committed r108857: <http://trac.webkit.org/changeset/108857>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen Chenney]

Committed r114413: <http://trac.webkit.org/changeset/114413>