Bug 199850 - CanvasRenderingContext2D.setTransfrom() reads only the aliases attributes of DOMMatrix2DInit
Summary: CanvasRenderingContext2D.setTransfrom() reads only the aliases attributes of ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Linux
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-16 20:55 PDT by Kagami Sascha Rosylight
Modified: 2019-07-23 13:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.78 KB, patch)
2019-07-23 11:43 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.76 KB, patch)
2019-07-23 12:15 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kagami Sascha Rosylight 2019-07-16 20:55:46 PDT
Relevant WPT test: https://w3c-test.org/css/geometry/DOMMatrix2DInit-validate-fixup.html
Smaller visual test: https://codepen.io/SaschaNaz/pen/voBBee (Both rectangle must look same with their width being twice of their height.)

Relevant source: https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp#L910

It seems the method internally reads a/b/c/d/c/f instead of m11/m12/m21/m22/m41/m42, which is a problem because validateAndFixup only ensures correct values for m** fields.
Comment 1 Radar WebKit Bug Importer 2019-07-19 23:50:45 PDT
<rdar://problem/53339548>
Comment 2 Said Abou-Hallawa 2019-07-23 11:43:52 PDT
Created attachment 374689 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-07-23 11:51:20 PDT
Comment on attachment 374689 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:916
> +    setTransform(matrixInit.m11.valueOr(1), matrixInit.m12.valueOr(0), matrixInit.m21.valueOr(0), matrixInit.m22.valueOr(1), matrixInit.m41.valueOr(0), matrixInit.m42.valueOr(0));

Should this be matrixInit.m11.valueOr(matrixInit.a.valueOr(1)) etc?
Comment 4 Said Abou-Hallawa 2019-07-23 12:06:51 PDT
Comment on attachment 374689 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:916
>> +    setTransform(matrixInit.m11.valueOr(1), matrixInit.m12.valueOr(0), matrixInit.m21.valueOr(0), matrixInit.m22.valueOr(1), matrixInit.m41.valueOr(0), matrixInit.m42.valueOr(0));
> 
> Should this be matrixInit.m11.valueOr(matrixInit.a.valueOr(1)) etc?

Just before this line we call DOMMatrixReadOnly::validateAndFixup(matrixInit) which ensures the m** fields are set if they are not set and the aliases fields are set for example.

    if (!init.m11)
        init.m11 = init.a.valueOr(1);

So I think I do not need to repeat this code again here.
Comment 5 Said Abou-Hallawa 2019-07-23 12:11:04 PDT
Comment on attachment 374689 [details]
Patch

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

>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:916
>>> +    setTransform(matrixInit.m11.valueOr(1), matrixInit.m12.valueOr(0), matrixInit.m21.valueOr(0), matrixInit.m22.valueOr(1), matrixInit.m41.valueOr(0), matrixInit.m42.valueOr(0));
>> 
>> Should this be matrixInit.m11.valueOr(matrixInit.a.valueOr(1)) etc?
> 
> Just before this line we call DOMMatrixReadOnly::validateAndFixup(matrixInit) which ensures the m** fields are set if they are not set and the aliases fields are set for example.
> 
>     if (!init.m11)
>         init.m11 = init.a.valueOr(1);
> 
> So I think I do not need to repeat this code again here.

I think I should even use matrixInit.m11.value() since it has been set to a.valueOr(1).
Comment 6 Said Abou-Hallawa 2019-07-23 12:15:15 PDT
Created attachment 374691 [details]
Patch
Comment 7 WebKit Commit Bot 2019-07-23 13:39:31 PDT
Comment on attachment 374691 [details]
Patch

Clearing flags on attachment: 374691

Committed r247735: <https://trac.webkit.org/changeset/247735>
Comment 8 WebKit Commit Bot 2019-07-23 13:39:33 PDT
All reviewed patches have been landed.  Closing bug.