Bug 77179

Summary: getIntersectionList causes transforms to be recalculated in SVG
Product: WebKit Reporter: Philip Rogers <pdr>
Component: SVGAssignee: Peter Beverloo <peter>
Status: RESOLVED FIXED    
Severity: Normal CC: peter, rwlbuis, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 11274    
Attachments:
Description Flags
Test case - box spins when the mouse is moved
none
Patch
none
Patch (+ test)
none
Patch (+ test)
zimmermann: review+
Patch for landing none

Description Philip Rogers 2012-01-26 20:16:30 PST
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.
Comment 1 Peter Beverloo 2012-01-31 06:57:32 PST
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.
Comment 2 Peter Beverloo 2012-01-31 07:00:00 PST
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!
Comment 3 WebKit Review Bot 2012-01-31 07:04:35 PST
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.
Comment 4 Peter Beverloo 2012-01-31 08:54:02 PST
Created attachment 124746 [details]
Patch (+ test)

Added a test following a discussion with pdr in #ksvg.
Comment 5 WebKit Review Bot 2012-01-31 08:56:38 PST
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.
Comment 6 Philip Rogers 2012-01-31 11:06:34 PST
(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.
Comment 7 Peter Beverloo 2012-02-01 04:02:25 PST
(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.
Comment 8 Peter Beverloo 2012-02-01 04:03:09 PST
Created attachment 124925 [details]
Patch (+ test)

Updated patch.
Comment 9 Nikolas Zimmermann 2012-02-01 04:13:25 PST
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.
Comment 10 Peter Beverloo 2012-02-01 04:22:04 PST
Created attachment 124927 [details]
Patch for landing

Thank you for the review! Changed the test and results to create a green square.
Comment 11 WebKit Review Bot 2012-02-01 05:31:27 PST
Comment on attachment 124927 [details]
Patch for landing

Clearing flags on attachment: 124927

Committed r106464: <http://trac.webkit.org/changeset/106464>
Comment 12 WebKit Review Bot 2012-02-01 05:31:32 PST
All reviewed patches have been landed.  Closing bug.