Bug 172114

Summary:

Align WebKitCSSMatrix stringifier with spec for DOMMatrix

Product:

WebKit

Reporter:

Simon Pieters (:zcorpan) <zcorpan>

Component:

DOM

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, simon.fraser

Priority:

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