Bug 172114 - Align WebKitCSSMatrix stringifier with spec for DOMMatrix
Summary: Align WebKitCSSMatrix stringifier with spec for DOMMatrix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://drafts.fxtf.org/geometry/#dom...
Keywords: FromImplementor
Depends on:
Blocks:
 
Reported: 2017-05-15 05:30 PDT by Simon Pieters
Modified: 2017-05-16 14:14 PDT (History)
3 users (show)

See Also:


Attachments
WIP Patch (11.39 KB, patch)
2017-05-15 12:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.93 KB, patch)
2017-05-15 12:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.05 KB, patch)
2017-05-15 14:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pieters 2017-05-15 05:30:32 PDT
See
https://github.com/w3c/fxtf-drafts/issues/120
https://github.com/w3c/fxtf-drafts/pull/148
https://github.com/w3c/web-platform-tests/pull/5885

In WebKit the numbers are serialized always with 6 digits, per spec it should use JS's ToString.

Fail	WebKitCSSMatrix stringifier: identity (2d)	assert_equals: expected "matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000)"


NaN and Infinity values should cause an InvalidStateError exception in the stringifier.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5168

<!DOCTYPE html>
<script>
var m = new WebKitCSSMatrix();
m.a = NaN;
m.b = Infinity;
m.c = -Infinity;
w(String(m));
</script>

log: matrix(nan, inf, -inf, 1.000000, 0.000000, 0.000000)


Related bugs:
https://bugs.webkit.org/show_bug.cgi?id=153675
https://bugs.webkit.org/show_bug.cgi?id=110001
https://bugs.webkit.org/show_bug.cgi?id=163505
Comment 1 Chris Dumez 2017-05-15 09:34:29 PDT
(In reply to Simon Pieters from comment #0)
> See
> https://github.com/w3c/fxtf-drafts/issues/120
> https://github.com/w3c/fxtf-drafts/pull/148
> https://github.com/w3c/web-platform-tests/pull/5885
> 
> In WebKit the numbers are serialized always with 6 digits, per spec it
> should use JS's ToString.
> 
> Fail	WebKitCSSMatrix stringifier: identity (2d)	assert_equals: expected
> "matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
> 1.000000, 0.000000, 0.000000)"

Ok, it seems Firefox does this. Seems like a good thing to fix.

> 
> 
> NaN and Infinity values should cause an InvalidStateError exception in the
> stringifier.
> 
> http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5168
> 
> <!DOCTYPE html>
> <script>
> var m = new WebKitCSSMatrix();
> m.a = NaN;
> m.b = Infinity;
> m.c = -Infinity;
> w(String(m));
> </script>
> 
> log: matrix(nan, inf, -inf, 1.000000, 0.000000, 0.000000)

That part, I am less sure of. No other browser seems to throw and I expect compatibility risks if we started throwing.
Comment 2 Chris Dumez 2017-05-15 11:39:42 PDT
(In reply to Chris Dumez from comment #1)
> (In reply to Simon Pieters from comment #0)
> > See
> > https://github.com/w3c/fxtf-drafts/issues/120
> > https://github.com/w3c/fxtf-drafts/pull/148
> > https://github.com/w3c/web-platform-tests/pull/5885
> > 
> > In WebKit the numbers are serialized always with 6 digits, per spec it
> > should use JS's ToString.
> > 
> > Fail	WebKitCSSMatrix stringifier: identity (2d)	assert_equals: expected
> > "matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
> > 1.000000, 0.000000, 0.000000)"
> 
> Ok, it seems Firefox does this. Seems like a good thing to fix.
> 
> > 
> > 
> > NaN and Infinity values should cause an InvalidStateError exception in the
> > stringifier.
> > 
> > http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5168
> > 
> > <!DOCTYPE html>
> > <script>
> > var m = new WebKitCSSMatrix();
> > m.a = NaN;
> > m.b = Infinity;
> > m.c = -Infinity;
> > w(String(m));
> > </script>
> > 
> > log: matrix(nan, inf, -inf, 1.000000, 0.000000, 0.000000)
> 
> That part, I am less sure of. No other browser seems to throw and I expect
> compatibility risks if we started throwing.

Looks like Simon is Ok with this change so I'll look into doing this part too.
Comment 3 Chris Dumez 2017-05-15 12:18:35 PDT
Created attachment 310154 [details]
WIP Patch
Comment 4 Chris Dumez 2017-05-15 12:50:26 PDT
Created attachment 310156 [details]
Patch
Comment 5 Simon Fraser (smfr) 2017-05-15 14:11:58 PDT
Comment on attachment 310156 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1702
> +        && std::isfinite(m_matrix[3][3]);

What happened to m_matrix[3][0], m_matrix[3][1], m_matrix[3][2] ?

> LayoutTests/fast/css/matrix-stringifier-expected.txt:19
> +FAIL DOMMatrix stringifier: identity (2d) undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL DOMMatrix stringifier: integer 2d undefined is not a constructor (evaluating 'new self[constr]("matrix(1, 0, 0, 1, 0, 0)")')
> +FAIL DOMMatrix stringifier: integer 3d undefined is not a constructor (evaluating 'new self[constr]("matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)")')
> +FAIL DOMMatrix stringifier: floating point 2d undefined is not a constructor (evaluating 'new self[constr]("matrix(0.1, 0.2, 0.3, 0.4, 0.5, 0.6)")')
> +FAIL DOMMatrix stringifier: floating point 3d undefined is not a constructor (evaluating 'new self[constr]("matrix3d(0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7)")')
> +FAIL DOMMatrix stringifier: NaN undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL DOMMatrix stringifier: Infinity undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL DOMMatrix stringifier: -Infinity undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL ${constr}.toString should be enumerable undefined is not an object (evaluating 'self[constr].prototype')
> +FAIL DOMMatrixReadOnly stringifier: identity (2d) undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL DOMMatrixReadOnly stringifier: integer 2d undefined is not a constructor (evaluating 'new self[constr]("matrix(1, 0, 0, 1, 0, 0)")')
> +FAIL DOMMatrixReadOnly stringifier: integer 3d undefined is not a constructor (evaluating 'new self[constr]("matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)")')
> +FAIL DOMMatrixReadOnly stringifier: floating point 2d undefined is not a constructor (evaluating 'new self[constr]("matrix(0.1, 0.2, 0.3, 0.4, 0.5, 0.6)")')
> +FAIL DOMMatrixReadOnly stringifier: floating point 3d undefined is not a constructor (evaluating 'new self[constr]("matrix3d(0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7)")')
> +FAIL DOMMatrixReadOnly stringifier: NaN undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL DOMMatrixReadOnly stringifier: Infinity undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL DOMMatrixReadOnly stringifier: -Infinity undefined is not a constructor (evaluating 'new self[constr]()')
> +FAIL ${constr}.toString should be enumerable undefined is not an object (evaluating 'self[constr].prototype')

I assume these fails are expected at this point?
Comment 6 Chris Dumez 2017-05-15 14:15:43 PDT
Comment on attachment 310156 [details]
Patch

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

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1702
>> +        && std::isfinite(m_matrix[3][3]);
> 
> What happened to m_matrix[3][0], m_matrix[3][1], m_matrix[3][2] ?

Well, this is an excellent question :)
Comment 7 Chris Dumez 2017-05-15 14:16:53 PDT
Comment on attachment 310156 [details]
Patch

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

>> LayoutTests/fast/css/matrix-stringifier-expected.txt:19
>> +FAIL ${constr}.toString should be enumerable undefined is not an object (evaluating 'self[constr].prototype')
> 
> I assume these fails are expected at this point?

And yes, those are expected at this point since we do not support DOMMatrix or DOMMatrixReadOnly yet.

That said, the ${constr} in the message is a bug, I'll fix before landing.
Comment 8 Chris Dumez 2017-05-15 14:21:25 PDT
Created attachment 310162 [details]
Patch
Comment 9 Chris Dumez 2017-05-15 15:08:00 PDT
Comment on attachment 310162 [details]
Patch

Clearing flags on attachment: 310162

Committed r216881: <http://trac.webkit.org/changeset/216881>
Comment 10 Chris Dumez 2017-05-15 15:08:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Pieters 2017-05-16 14:14:20 PDT
Let me know if throwing causes problems. The reason I think it should be OK is because the current behavior will throw upon parsing anyway:

var m = new WebKitCSSMatrix();
m.a = NaN;
var m2 = new WebKitCSSMatrix(String(m)); // SyntaxError (DOM Exception 12): The string did not match the expected pattern.