Bug 174278 – Add CanvasRenderingContext2D::getTransform

• Component: Canvas
• Status: RESOLVED FIXED

[Devin Rousso]

Now that DOMMatrix is supported, we can implement the CanvasRenderingContext2D functions that use it:

[NewObject] DOMMatrix getTransform();
[MayThrowException] void setTransform(optional DOMMatrixInit transform);

https://html.spec.whatwg.org/multipage/canvas.html#canvastransform

[Devin Rousso]

Created attachment 314885 [details]
[Patch] Without tests

[Build Bot]

Attachment 314885 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/CanvasRenderingContext2D.h:122:  The parameter name "transform" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 314885 [details]
[Patch] Without tests

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:817
>  

The isfinite() bitwise ORs instead of logic ORs in setTransform() seems weird. I bet it was unintentional.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:120
> +    Ref<DOMMatrix> getTransform();

I think we can make this `const`.

[Build Bot]

Comment on attachment 314885 [details]
[Patch] Without tests

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

New failing tests:
canvas/philip/tests/2d.missingargs.html

[Build Bot]

Created attachment 314895 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314885 [details]
[Patch] Without tests

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

New failing tests:
canvas/philip/tests/2d.missingargs.html

[Build Bot]

Created attachment 314897 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314885 [details]
[Patch] Without tests

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

New failing tests:
canvas/philip/tests/2d.missingargs.html

[Build Bot]

Created attachment 314901 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

The failing assertion was:
LayoutTests/canvas/philip/tests/2d.missingargs.html

> try { var _thrown = false;
>   ctx.setTransform();
> } catch (e) { if (!(e instanceof TypeError)) _fail("Failed assertion: expected exception of type TypeError, got: "+e); _thrown = true; } finally { _assert(_thrown, "should throw exception of type TypeError: ctx.setTransform()"); }

This test was written when only `setTransform(a,b,c,d,e,f)` existed. So we should re-evaluate whether it is correct or not.

---

The IDL for setTransform is now overloaded:
https://html.spec.whatwg.org/multipage/canvas.html#2dcontext

> void setTransform(unrestricted double a, unrestricted double b, unrestricted double c, unrestricted double d, unrestricted double e, unrestricted double f);
> void setTransform(optional DOMMatrixInit transform);

So which function does: `ctx.setTransform()` invoke?

It seems this is a valid invocation of the 2nd signature, expecting an optional matrix.

---

If that is the case it should be valid.

WebIDL says if an an optional dictionary is not provided the optional value may be an empty dictionary:
https://heycam.github.io/webidl/#dfn-optional-argument

> If the type of an argument is a dictionary type or a union type that has a dictionary as
> one of its flattened member types, and that dictionary type and its ancestors have no
> required members, and the argument is either the final argument or is followed only by
> optional arguments, then the argument must be specified as optional. Such arguments are
> always considered to have a default value of an empty dictionary, unless otherwise specified.

DOMMatrixInit is a dictionary type with no `required` members.
https://drafts.fxtf.org/geometry/#dictdef-dommatrixinit

> dictionary DOMMatrixInit { ... }

So that would mean:

    setTransform()

Should be equivalent to:

    setTransform({})

Which should be fine.

---

So I think this particular assertion is no longer valid.

[Sam Weinig]

Comment on attachment 314885 [details]
[Patch] Without tests

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:820
> +    auto matrixOrException = DOMMatrixReadOnly::fromMatrix(WTFMove(matrixInit));

It seems a shame to allocate this matrix just to get the exception seems like a shame.  Seems like you could expose DOMReadOnlyMatrix::validateAndFixup and just pull out the values.  I'm also unclear what is supposed to happen if the matrix is not a 2d matrix.  It seems odd to leave the remaining values on the floor.  Is CanvasRenderingContext2D supposed to have a full matrix now, not just an affine transform?

[Devin Rousso]

Created attachment 314928 [details]
Patch

[Build Bot]

Comment on attachment 314928 [details]
Patch

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

New failing tests:
fast/canvas/2d.setTransform.matrix.html

[Build Bot]

Created attachment 314929 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314928 [details]
Patch

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

New failing tests:
fast/canvas/2d.setTransform.matrix.html

[Build Bot]

Created attachment 314930 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314928 [details]
Patch

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

New failing tests:
fast/canvas/2d.setTransform.matrix.html

[Build Bot]

Created attachment 314931 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 314965 [details]
Patch

[Joseph Pecoraro]

> Is CanvasRenderingContext2D supposed to have a full matrix now, not just an affine transform?

The spec isn't clear, so we should file a bug on the spec. However, given the spec doesn't mention 3D attributes at all, and doesn't upgrade the older setTransform() to deal with 3D, I think its safe to say this should only affect 2D attributes.

[Joseph Pecoraro]

Comment on attachment 314965 [details]
Patch

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

> LayoutTests/fast/canvas/2d.setTransform.matrix.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

There should be a test for when setTransform can throw an exception. Calling `setTransform` with an init dictionary that doesn't pass the validation check should throw a TypeError with a responsible string. This could probably be its own small test file.

[Joseph Pecoraro]

Comment on attachment 314965 [details]
Patch

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

>> LayoutTests/fast/canvas/2d.setTransform.matrix.html:1
>> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> There should be a test for when setTransform can throw an exception. Calling `setTransform` with an init dictionary that doesn't pass the validation check should throw a TypeError with a responsible string. This could probably be its own small test file.

Also whats with this ancient DOCTYPE. Just use the basic one =).

[Devin Rousso]

Created attachment 315035 [details]
Patch

[Sam Weinig]

(In reply to Joseph Pecoraro from comment #20)
> > Is CanvasRenderingContext2D supposed to have a full matrix now, not just an affine transform?
> 
> The spec isn't clear, so we should file a bug on the spec. However, given
> the spec doesn't mention 3D attributes at all, and doesn't upgrade the older
> setTransform() to deal with 3D, I think its safe to say this should only
> affect 2D attributes.

What do other browsers do?

[Devin Rousso]

(In reply to Sam Weinig from comment #24)
> (In reply to Joseph Pecoraro from comment #20)
> > > Is CanvasRenderingContext2D supposed to have a full matrix now, not just an affine transform?
> > 
> > The spec isn't clear, so we should file a bug on the spec. However, given
> > the spec doesn't mention 3D attributes at all, and doesn't upgrade the older
> > setTransform() to deal with 3D, I think its safe to say this should only
> > affect 2D attributes.
> 
> What do other browsers do?

As far as I can tell, it looks like no other browser (on Mac) has implemented this.

[Sam Weinig]

(In reply to Devin Rousso from comment #25)
> (In reply to Sam Weinig from comment #24)
> > (In reply to Joseph Pecoraro from comment #20)
> > > > Is CanvasRenderingContext2D supposed to have a full matrix now, not just an affine transform?
> > > 
> > > The spec isn't clear, so we should file a bug on the spec. However, given
> > > the spec doesn't mention 3D attributes at all, and doesn't upgrade the older
> > > setTransform() to deal with 3D, I think its safe to say this should only
> > > affect 2D attributes.
> > 
> > What do other browsers do?
> 
> As far as I can tell, it looks like no other browser (on Mac) has
> implemented this.

In that case, I think it would make sense to get clarity on the behavior with non-affine matrices before committing to an implementation.

[Joseph Pecoraro]

> In that case, I think it would make sense to get clarity on the behavior
> with non-affine matrices before committing to an implementation.

Dean, it appears you added this to the spec, maybe you have an opinion?
https://github.com/whatwg/html/pull/280#issuecomment-152398609

I don't see any discussion about 3d or not. Devin will file a bug and point to it.

[Dean Jackson]

(In reply to Joseph Pecoraro from comment #27)
> > In that case, I think it would make sense to get clarity on the behavior
> > with non-affine matrices before committing to an implementation.
> 
> Dean, it appears you added this to the spec, maybe you have an opinion?
> https://github.com/whatwg/html/pull/280#issuecomment-152398609
> 
> I don't see any discussion about 3d or not. Devin will file a bug and point
> to it.

A 3d transformation can still be affine, so I don't think DOMAffineFoo works.

I agree that we should raise a bug on what to do with setTransform if you give something with 3d values.

[Joseph Pecoraro]

> I agree that we should raise a bug on what to do with setTransform if you
> give something with 3d values.

https://github.com/whatwg/html/issues/2826

[Dean Jackson]

Comment on attachment 315035 [details]
Patch

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

> LayoutTests/fast/canvas/2d.getTransform.modification.html:22
> +ctx.setTransform(1, 2, 3, 4, 5, 6);
> +shouldBe("copiedTransform.a", "1");
> +shouldBe("copiedTransform.b", "0");
> +shouldBe("copiedTransform.c", "0");
> +shouldBe("copiedTransform.d", "1");
> +shouldBe("copiedTransform.e", "0");
> +shouldBe("copiedTransform.f", "0");
> +
> +copiedTransform.translate(-1, -2);
> +var newTransform = ctx.getTransform();

How about another test that transforms the context directly, then checks to make sure the original getTransform hasn't changed?

> LayoutTests/fast/canvas/2d.getTransform.modification.html:29
> +

Also a test that gets a transform, manipulates it, then does setTransform, then get to make sure it worked.

Bonus, you could do it with two contexts. One you change via getTransform/setTransform, the other directly, then compare getTransform on both at the end.

> LayoutTests/fast/canvas/2d.setTransform.matrix.html:18
> +shouldEvaluateTo("originalTransform.a", 1);
> +shouldEvaluateTo("originalTransform.b", 0);
> +shouldEvaluateTo("originalTransform.c", 0);
> +shouldEvaluateTo("originalTransform.d", 1);
> +shouldEvaluateTo("originalTransform.e", 0);
> +shouldEvaluateTo("originalTransform.f", 0);
> +

Make a helper function?

shouldHaveMatrixEquality(originalTransform, [1, 0, 0, 1, 0, 0])

> LayoutTests/fast/canvas/2d.setTransform.matrix.html:45
> +ctx.setTransform(new DOMMatrix([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]));
> +var newTransformFrom3dDOMMatrix = ctx.getTransform();
> +shouldEvaluateTo("newTransformFrom3dDOMMatrix.a", 1);
> +shouldEvaluateTo("newTransformFrom3dDOMMatrix.b", 2);
> +shouldEvaluateTo("newTransformFrom3dDOMMatrix.c", 5);
> +shouldEvaluateTo("newTransformFrom3dDOMMatrix.d", 6);
> +shouldEvaluateTo("newTransformFrom3dDOMMatrix.e", 13);
> +shouldEvaluateTo("newTransformFrom3dDOMMatrix.f", 14);
> +

Is this enough to check that the 3d bits were dropped?

> LayoutTests/fast/canvas/2d.setTransform.matrix.html:66
> +shouldThrowErrorName(function() {
> +    ctx.setTransform({a: 1, m11: 11, b: 2, m12: 12, c: 3, m21: 21, d: 4, m22: 22, e: 5, m41: 41, f: 6, m42: 42});
> +}, "TypeError");

Add

ctx.setTransform(1)
ctx.setTransform({a: {}})
ctx.setTransform({a: "hi"})
etc

[Devin Rousso]

Created attachment 315744 [details]
Patch

[Build Bot]

Comment on attachment 315744 [details]
Patch

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

New failing tests:
fast/canvas/2d.setTransform.matrix.html

[Build Bot]

Created attachment 315749 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315744 [details]
Patch

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

New failing tests:
fast/canvas/2d.setTransform.matrix.html

[Build Bot]

Created attachment 315750 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 315751 [details]
Patch

[Devin Rousso]

Created attachment 315753 [details]
Patch

[Devin Rousso]

(In reply to Devin Rousso from comment #37)
> Created attachment 315753 [details]
> Patch

So I ran into a situation while writing the additional tests proposed by @dino:

For the test that runs `ctx.setTransform({a: {}});`, it wouldn't have any affect if I made [1] call through to [2], which checks for `std::isfinite()`.  In this case, the value `{}` is NaN, so it would fail that check and therefore not call `resetTransform()`.  I think that this is not the desired behavior, so I changed the logic of [1] to call `resetTransform()` and then `transform()`, similar to how the existing logic of [2] works.

I will file a bug on the spec about this.  For now, any thoughts?

[1] CanvasRenderingContext2D::setTransform(DOMMatrixInit&& matrixInit)
[2] CanvasRenderingContext2D::setTransform(float m11, float m12, float m21, float m22, float dx, float dy)

[Devin Rousso]

(In reply to Devin Rousso from comment #38)
> (In reply to Devin Rousso from comment #37)
> > Created attachment 315753 [details]
> > Patch
> 
> So I ran into a situation while writing the additional tests proposed by
> @dino:
> 
> For the test that runs `ctx.setTransform({a: {}});`, it wouldn't have any
> affect if I made [1] call through to [2], which checks for
> `std::isfinite()`.  In this case, the value `{}` is NaN, so it would fail
> that check and therefore not call `resetTransform()`.  I think that this is
> not the desired behavior, so I changed the logic of [1] to call
> `resetTransform()` and then `transform()`, similar to how the existing logic
> of [2] works.
> 
> I will file a bug on the spec about this.  For now, any thoughts?
> 
> [1] CanvasRenderingContext2D::setTransform(DOMMatrixInit&& matrixInit)
> [2] CanvasRenderingContext2D::setTransform(float m11, float m12, float m21,
> float m22, float dx, float dy)

So I thought about this a bit more, and I've did a bit of a 180.  I think it makes more sense for `setTransform()` to just bail in the case that NaN/Infinity is present, as this follows the existing behavior of `setTransform()`.  I've filed a bug on the spec:

https://github.com/whatwg/html/issues/2845

[Devin Rousso]

Created attachment 315806 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315806 [details]
Patch

Clearing flags on attachment: 315806

Committed r219619: <http://trac.webkit.org/changeset/219619>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.