Bug 77179 – getIntersectionList causes transforms to be recalculated in SVG

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

Created attachment 124249 [details]
Test case - box spins when the mouse is moved

In an SVG file the combination of adding a child element and calling getIntersectionList causes transforms to be recalculated. In the testcase (see: spinbox.svg), moving the mouse causes the green box to spin.

This may be a regression as it occurs on WebKit r106063 and Chrome 16.0.912.77 but not Safari 5.1.2.

[Peter Beverloo]

Bisecting Chromium builds ends up in the following WebKit commit range (r91856-r91828, http://trac.webkit.org/log/trunk/?rev=91856&stop_rev=91828&verbose=on), most interesting clearly is this commit by rbuis:

http://trac.webkit.org/changeset/91850

The issue seems to be on line 127 of RenderSVGModelObject.cpp (http://trac.webkit.org/changeset/91850/trunk/Source/WebCore/rendering/svg/RenderSVGModelObject.cpp), which retrieves the renderer's localToParentTransform AffineTransform, removes the const and multiplies it with the currently known transform.

Patch upcoming.

[Peter Beverloo]

Created attachment 124723 [details]
Patch

No new tests as the rect's get(Screen)CTM() methods don't seem to be influenced by this, though verified manually. Pointers in how to write one would be welcome, of course!

[WebKit Review Bot]

Attachment 124723 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo]

Created attachment 124746 [details]
Patch (+ test)

Added a test following a discussion with pdr in #ksvg.

[WebKit Review Bot]

Attachment 124746 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philip Rogers]

(In reply to comment #4)
> Created an attachment (id=124746) [details]
> Patch (+ test)
> 
> Added a test following a discussion with pdr in #ksvg.

I think this change looks good overall.

A couple very minor nits: I think git is messing up the png in your ChangeLog, which is causing the style guide to complain.
A common pattern in pixel tests is to have an untranslated green square as "pass". Could you make the initial rotation be 0 or add a comment in the test describing that a rotated green square should be considered passing? This will make it easier for sheriffs/rebaseliners. One way would be to rotate the entire svg element by -50deg.

[Peter Beverloo]

(In reply to comment #6)
> (In reply to comment #4)
> > Created an attachment (id=124746) [details] [details]
> > Patch (+ test)
> > 
> > Added a test following a discussion with pdr in #ksvg.
> 
> I think this change looks good overall.
> 
> A couple very minor nits: I think git is messing up the png in your ChangeLog, which is causing the style guide to complain.
> A common pattern in pixel tests is to have an untranslated green square as "pass". Could you make the initial rotation be 0 or add a comment in the test describing that a rotated green square should be considered passing? This will make it easier for sheriffs/rebaseliners. One way would be to rotate the entire svg element by -50deg.

Thank you. The style-bot was having troubles yesterday so I'm fairly sure it's unrelated to the patch. The new patch should be fine.

I've made the test clearer by making the square blue instead of green, and added a comment that a non-rotated square in the top-left corner is the expected result, whereas other positions and/or rotations are failures.

[Peter Beverloo]

Created attachment 124925 [details]
Patch (+ test)

Updated patch.

[Nikolas Zimmermann]

Comment on attachment 124925 [details]
Patch (+ test)

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

Good catch, r=me with comments:

> LayoutTests/svg/custom/intersection-list-transforms.svg:13
> +  <!-- This test passes if there is a blue non-rotated square visible on the top-left corner of the page. -->

It should be green.

[Peter Beverloo]

Created attachment 124927 [details]
Patch for landing

Thank you for the review! Changed the test and results to create a green square.

[WebKit Review Bot]

Comment on attachment 124927 [details]
Patch for landing

Clearing flags on attachment: 124927

Committed r106464: <http://trac.webkit.org/changeset/106464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.