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

RESOLVED FIXED 138439

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

https://bugs.webkit.org/show_bug.cgi?id=138439

Summary Revert a change in SVGRenderSupport::mapLocalToContainer committed for fixing...

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component SVG

Assignee

Said Abou-Hallawa

Reported

2014-11-05 14:40 PST

Modified

2015-02-16 15:19 PST History

CC List

16 users Show

URL

Keywords

Duplicates (1)

119626 View as bug list

Depends on

Blocks