Bug 172114

Summary: Align WebKitCSSMatrix stringifier with spec for DOMMatrix
Product: WebKit Reporter: Simon Pieters (:zcorpan) <zcorpan>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, simon.fraser
Priority: P2 Keywords: FromImplementor
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-stringifier
See Also: https://bugs.webkit.org/show_bug.cgi?id=172130
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch none

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
Patch (17.93 KB, patch)
2017-05-15 12:50 PDT, Chris Dumez
no flags
Patch (18.05 KB, patch)
2017-05-15 14:21 PDT, Chris Dumez
no flags
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
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
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.