Drawing an SVG image into a <canvas> that is not in the DOM draws the wrong region

[Antoine Quint]

Created attachment 282375 [details]
Testcase

See the attached testcase.

[Antoine Quint]

<rdar://problem/25525072>

[Antoine Quint]

Created attachment 282376 [details]
Patch

[Dean Jackson]

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?

[Antoine Quint]

(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.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 282383 [details]
Patch

[Dean Jackson]

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

[Antoine Quint]

Created attachment 282420 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Antoine Quint]

Created attachment 282433 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

(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

[Said Abou-Hallawa]

(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).

[Said Abou-Hallawa]

(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.

[Dean Jackson]

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.

[WebKit Commit Bot]

Comment on attachment 282433 [details]
Patch

Clearing flags on attachment: 282433

Committed r202712: <http://trac.webkit.org/changeset/202712>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Quint]

The followup bug is https://bugs.webkit.org/show_bug.cgi?id=159340.