172114 – Align WebKitCSSMatrix stringifier with spec for DOMMatrix

RESOLVED FIXED 172114

Align WebKitCSSMatrix stringifier with spec for DOMMatrix

https://bugs.webkit.org/show_bug.cgi?id=172114

Summary Align WebKitCSSMatrix stringifier with spec for DOMMatrix

Simon Pieters (:zcorpan)

Reported 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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 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.

Chris Dumez

Comment 2 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.

Chris Dumez

Comment 3 2017-05-15 12:18:35 PDT

Created attachment 310154 [details] WIP Patch

Chris Dumez

Comment 4 2017-05-15 12:50:26 PDT

Created attachment 310156 [details] Patch

Simon Fraser (smfr)

Comment 5 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?

Chris Dumez

Comment 6 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 :)

Chris Dumez

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

Chris Dumez

Comment 8 2017-05-15 14:21:25 PDT

Created attachment 310162 [details] Patch

Chris Dumez

Comment 9 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>

Chris Dumez

Comment 10 2017-05-15 15:08:02 PDT

All reviewed patches have been landed. Closing bug.

Simon Pieters (:zcorpan)

Comment 11 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.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version Safari Technology Preview

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Chris Dumez

Reported

2017-05-15 05:30 PDT

Modified

2017-05-16 14:14 PDT History

CC List

3 users Show

URL

https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-stringifier

Keywords FromImplementor

Depends on

Blocks