Bug 138439

Summary:

Revert a change in SVGRenderSupport::mapLocalToContainer committed for fixing <https://bugs.webkit.org/show_bug.cgi?id=119626>

Product:

WebKit

Reporter:

Said Abou-Hallawa <sabouhallawa>

Component:

SVG

Assignee:

Said Abou-Hallawa <sabouhallawa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, rhodovan.u-szeged, schenney, sergio, simon.fraser, webkit-bug-importer, zalan, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Example	none
Example	none
Patch	none

Said Abou-Hallawa

Reported 2014-11-05 14:40:26 PST

The original fix <https://bugs.webkit.org/show_bug.cgi?id=119626> was just porting the Blink fix <https://codereview.chromium.org/143363004>. This change causes the WebKit search in SVG to be broken. And it was also reverted from Blink because it broke their SVG rendering.

Attachments
Patch (4.33 KB, patch) 2014-11-06 18:17 PST, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (4.37 KB, patch) 2014-11-13 19:26 PST, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (6.18 KB, patch) 2015-02-13 21:13 PST, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Example (347 bytes, text/html) 2015-02-15 14:55 PST, Said Abou-Hallawa	no flags	Details
Example (410 bytes, text/html) 2015-02-15 14:58 PST, Said Abou-Hallawa	no flags	Details
Patch (6.20 KB, patch) 2015-02-16 14:33 PST, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Said Abou-Hallawa

Comment 1 2014-11-06 18:17:05 PST

Created attachment 241153 [details] Patch

Said Abou-Hallawa

Comment 2 2014-11-06 18:23:03 PST

*** Bug 119626 has been marked as a duplicate of this bug. ***

Simon Fraser (smfr)

Comment 3 2014-11-06 18:27:20 PST

Comment on attachment 241153 [details] Patch This patch needs a layout test (or two).

Said Abou-Hallawa

Comment 4 2014-11-13 19:26:00 PST

Created attachment 241530 [details] Patch

Said Abou-Hallawa

Comment 5 2014-11-13 19:31:15 PST

After talking to Simon, he agreed on not submitting more layout tests. The original fix submitted the test svg/transforms/svg-geometry-crash.html which was firing an assertion. To revert the fix and keep the test, I had to come with the right fix which fixes the assertion and does not break the layout.

Said Abou-Hallawa

Comment 6 2015-02-13 21:13:45 PST

Created attachment 246575 [details] Patch

Simon Fraser (smfr)

Comment 7 2015-02-13 22:17:52 PST

Comment on attachment 246575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246575&action=review > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94 > + transform = downcast<RenderSVGRoot>(parent).localToBorderBoxTransform() * renderer.localToParentTransform(); What is renderer.localToParentTransform() for the SVG root? I'm confused that you need both this and localToBorderBoxTransform.

Said Abou-Hallawa

Comment 8 2015-02-15 14:55:53 PST

Created attachment 246623 [details] Example

Said Abou-Hallawa

Comment 9 2015-02-15 14:56:52 PST

(In reply to comment #7) > Comment on attachment 246575 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246575&action=review > > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94 > > + transform = downcast<RenderSVGRoot>(parent).localToBorderBoxTransform() * renderer.localToParentTransform(); > > What is renderer.localToParentTransform() for the SVG root? I'm confused > that you need both this and localToBorderBoxTransform. Consider the following example (attached): <html> <head> <style> svg { width: 200px; height: 200px; background-color: lime; } </style> </head> <body> <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200" viewbox="-100 -100 200 200"> <rect x="0" y="0" width="100" height="100" fill="orange"/> <rect x="0" y="0" width="100" height="100" transform="translate(50,50) scale(0.5, 0.5)" fill="red"/> </svg> </body> </html> To get the localToParentTransform for a point in the red rectangle you need to do the following: -- Get localToParentTransform in SVG coordinate which is: = translate(50,50) * scale(0.5, 0.5) -- Since the parent is the SVG root, we need to map from SVG coordinates to the css coordinates. And since the viewbox="-100 -100 200 200", the localToBorderBoxTransform of the SVG root is: = translate(100, 100). -- So the localToParentTransform of a point in the red rectangle is css coordinate is: = translate(100, 100) * translate(50, 50) * scale(0.5, 0.5) = translate(150, 150) * scale(0.5, 0.5) As a concrete example, let's map the center of the red rectangle from local coordinate (which is in SVG coordinates) to parent css coordinates. -- The center of the red rectangle in local SVG coordinates is: p1 = (50,50) -- Map p1 from local SVG coordinates to parent SVG coordinates: p2 = translate(50, 50) * scale(0.5, 0.5) * p1 = (75,75) -- Map p2 from parent SVG coordinates to CSS coordinates: p3 = translate(100, 100) * p2 = (175,175) which is the correct mapping. Also notice that SVGRenderSupport::mapLocalToContainer() and SVGRenderSupport::pushMappingToContainer() handle the children SVG elements only. RenderSVGRoot does not call these methods and implements its own ones which expect local css box coordinates.

Said Abou-Hallawa

Comment 10 2015-02-15 14:58:00 PST

Created attachment 246624 [details] Example

WebKit Commit Bot

Comment 11 2015-02-16 14:28:26 PST

Comment on attachment 246575 [details] Patch Rejecting attachment 246575 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 246575, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/6731636418805760

Said Abou-Hallawa

Comment 12 2015-02-16 14:33:39 PST

Created attachment 246684 [details] Patch

WebKit Commit Bot

Comment 13 2015-02-16 15:19:33 PST

Comment on attachment 246684 [details] Patch Clearing flags on attachment: 246684 Committed r180179: <http://trac.webkit.org/changeset/180179>

WebKit Commit Bot

Comment 14 2015-02-16 15:19:40 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.