Bug 159276 - Drawing an SVG image into a <canvas> that is not in the DOM draws the wrong region
Summary: Drawing an SVG image into a <canvas> that is not in the DOM draws the wrong r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-29 14:41 PDT by Antoine Quint
Modified: 2016-07-01 02:51 PDT (History)
10 users (show)

See Also:


Attachments
Testcase (961 bytes, text/html)
2016-06-29 14:41 PDT, Antoine Quint
no flags Details
Patch (5.25 KB, patch)
2016-06-29 14:48 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2016-06-29 15:30 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2016-06-30 01:53 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (922.08 KB, application/zip)
2016-06-30 02:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.21 MB, application/zip)
2016-06-30 02:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.13 MB, application/zip)
2016-06-30 02:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.52 MB, application/zip)
2016-06-30 04:56 PDT, Build Bot
no flags Details
Patch (13.17 KB, patch)
2016-06-30 06:43 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Test case -- The image source size is different from the SVG viewBox. (906 bytes, text/html)
2016-06-30 11:46 PDT, Said Abou-Hallawa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-06-29 14:41:50 PDT
Created attachment 282375 [details]
Testcase

See the attached testcase.
Comment 1 Antoine Quint 2016-06-29 14:42:08 PDT
<rdar://problem/25525072>
Comment 2 Antoine Quint 2016-06-29 14:48:41 PDT
Created attachment 282376 [details]
Patch
Comment 3 Dean Jackson 2016-06-29 14:50:55 PDT
Comment on attachment 282376 [details]
Patch

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

Didn't you want to remove a timeout from an existing test too?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1406
> +        // If the <img> element is not attached to the DOM, the SVG image will not have had its
> +        // container sized yet and it needs to be sized to match the <img>.
> +        if (!imageElement.inDocument())
> +            image->setContainerSize(FloatSize(imageElement.naturalWidth(), imageElement.naturalHeight()));
> +        else
> +            image->setContainerSize(normalizedSrcRect.size());

What happens if the image element doesn't have a size?
Comment 4 Antoine Quint 2016-06-29 14:55:40 PDT
(In reply to comment #3)
> Comment on attachment 282376 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282376&action=review
> 
> Didn't you want to remove a timeout from an existing test too?

Yes, for the test written for https://bugs.webkit.org/show_bug.cgi?id=148845, but it's fairly involved and didn't want to mess it up.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1406
> > +        // If the <img> element is not attached to the DOM, the SVG image will not have had its
> > +        // container sized yet and it needs to be sized to match the <img>.
> > +        if (!imageElement.inDocument())
> > +            image->setContainerSize(FloatSize(imageElement.naturalWidth(), imageElement.naturalHeight()));
> > +        else
> > +            image->setContainerSize(normalizedSrcRect.size());
> 
> What happens if the image element doesn't have a size?

It gets the SVG image's intrinsic size.
Comment 5 Antoine Quint 2016-06-29 15:08:32 PDT
Actually, this patch does not cover the case where the <img> is in the DOM but has not been rendered yet. Will fix and add a new test.
Comment 6 Antoine Quint 2016-06-29 15:30:30 PDT
Created attachment 282383 [details]
Patch
Comment 7 Dean Jackson 2016-06-29 22:35:33 PDT
Comment on attachment 282383 [details]
Patch

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

Maybe you should add a test with an image that doesn't have a width and height. new Image();

> LayoutTests/svg/as-image/img-with-svg-resource-not-in-dom-and-drawImage.html:10
> +var svgData = 'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 200 200"><rect width="200" height="200" fill="red" /><rect x="20" y="20" width="40" height="40" fill="green" /></svg>';

don't need version attribute
Comment 8 Antoine Quint 2016-06-30 01:53:10 PDT
Created attachment 282420 [details]
Patch
Comment 9 Build Bot 2016-06-30 02:21:14 PDT
Comment on attachment 282420 [details]
Patch

Attachment 282420 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1599391

New failing tests:
fast/hidpi/image-srcset-svg-canvas-2x.html
Comment 10 Build Bot 2016-06-30 02:21:19 PDT
Created attachment 282422 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Build Bot 2016-06-30 02:33:00 PDT
Comment on attachment 282420 [details]
Patch

Attachment 282420 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1599408

New failing tests:
fast/hidpi/image-srcset-svg-canvas-2x.html
Comment 12 Build Bot 2016-06-30 02:33:04 PDT
Created attachment 282423 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 13 Build Bot 2016-06-30 02:36:04 PDT
Comment on attachment 282420 [details]
Patch

Attachment 282420 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1599426

New failing tests:
fast/hidpi/image-srcset-svg-canvas-2x.html
Comment 14 Build Bot 2016-06-30 02:36:08 PDT
Created attachment 282424 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 Build Bot 2016-06-30 04:56:01 PDT
Comment on attachment 282420 [details]
Patch

Attachment 282420 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1599934

New failing tests:
fast/hidpi/image-srcset-svg-canvas-2x.html
Comment 16 Build Bot 2016-06-30 04:56:06 PDT
Created attachment 282431 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Antoine Quint 2016-06-30 06:43:41 PDT
Created attachment 282433 [details]
Patch
Comment 18 Said Abou-Hallawa 2016-06-30 11:46:04 PDT
Created attachment 282453 [details]
Test case -- The image source size is different from the SVG viewBox.

This test case is broken in Chrome and FireFox also.
Comment 19 Said Abou-Hallawa 2016-06-30 11:47:24 PDT
Comment on attachment 282433 [details]
Patch

Looks good to me. Unofficial r=me. But please add the test case which I attached to the bug.
Comment 20 Said Abou-Hallawa 2016-06-30 11:49:59 PDT
(In reply to comment #18)
> Created attachment 282453 [details]
> Test case -- The image source size is different from the SVG viewBox.
> 
> This test case is broken in Chrome and FireFox also.

Sorry I meant it is broken in FireFox and Safari without your patch
Comment 21 Said Abou-Hallawa 2016-06-30 11:52:28 PDT
(In reply to comment #20)
> (In reply to comment #18)
> > Created attachment 282453 [details]
> > Test case -- The image source size is different from the SVG viewBox.
> > 
> > This test case is broken in Chrome and FireFox also.
> 
> Sorry I meant it is broken in FireFox and Safari without your patch

Actually it is broken in Chrome also. The expatiation is to have the canvas filled with black except a green rectangle (0,0)-(20,20) and a red rectangle (180,180)-(200,200).
Comment 22 Said Abou-Hallawa 2016-06-30 11:57:35 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > Created attachment 282453 [details]
> > > Test case -- The image source size is different from the SVG viewBox.
> > > 
> > > This test case is broken in Chrome and FireFox also.
> > 
> > Sorry I meant it is broken in FireFox and Safari without your patch
> 
> Actually it is broken in Chrome also. The expatiation is to have the canvas
> filled with black except a green rectangle (0,0)-(20,20) and a red rectangle
> (180,180)-(200,200).

It is also broken with your patch in WK2. I was trying it in WK1. This is a timing issue bug.
Comment 23 Dean Jackson 2016-06-30 15:07:01 PDT
Comment on attachment 282433 [details]
Patch

Could we add a test that exposes the flakey/timing case and mark it as PASS FAIL? I want to make sure we've captured that bug.
Comment 24 WebKit Commit Bot 2016-06-30 15:18:37 PDT
Comment on attachment 282433 [details]
Patch

Clearing flags on attachment: 282433

Committed r202712: <http://trac.webkit.org/changeset/202712>
Comment 25 WebKit Commit Bot 2016-06-30 15:18:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Antoine Quint 2016-07-01 02:51:53 PDT
The followup bug is https://bugs.webkit.org/show_bug.cgi?id=159340.