RESOLVED FIXED 196263
SVGMatrix.IDL methods do not conform to the specs 
https://bugs.webkit.org/show_bug.cgi?id=196263
Summary SVGMatrix.IDL methods do not conform to the specs
Said Abou-Hallawa
Reported 2019-03-26 11:38:55 PDT
The SVG 1.1 specs link is: https://www.w3.org/TR/SVG11/coords.html#InterfaceSVGMatrix. The specs of the methods is a little bit vague. For example the multiply method does not clearly state whether the operation should change the SVGMatrix object i.e. "thisMatrix *= secondMatrix; return thisMatrix;". Or it is read only operation i.e., "return thisMatrix * secondMatrix". But you can notice in the specs that SVGMatrix methods do not raise the exception NO_MODIFICATION_ALLOWED_ERR if the object is read only. Notice also setting the attribute 'a' for example may raise this exception. Therefore, I think the specs wanted to make these operations read-only. None of the methods should raise the exception NO_MODIFICATION_ALLOWED_ERR.
Attachments
Patch (26.51 KB, patch)
2019-03-26 11:41 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (40.95 KB, patch)
2019-03-26 16:22 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (41.58 KB, patch)
2019-03-26 16:46 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (42.25 KB, patch)
2019-04-01 11:20 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2019-03-26 11:41:23 PDT
Created attachment 365978 [details] Patch
Radar WebKit Bug Importer
Comment 2 2019-03-26 11:49:12 PDT
<rdar://problem/49283958>
Simon Fraser (smfr)
Comment 3 2019-03-26 13:04:54 PDT
Comment on attachment 365978 [details] Patch This needs a test (preferably a WPT so that we can share with other browsers).
Said Abou-Hallawa
Comment 4 2019-03-26 16:22:21 PDT
Created attachment 366020 [details] Patch
Said Abou-Hallawa
Comment 5 2019-03-26 16:46:10 PDT
Created attachment 366022 [details] Patch
Said Abou-Hallawa
Comment 6 2019-03-26 16:52:01 PDT
(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 365978 [details] > Patch > > This needs a test (preferably a WPT so that we can share with other > browsers). LayoutTests/svg/dom/SVGMatrix.html tests only the cases where invalid arguments are passed to the SVGMatrix methods. I cleaned the test and added few cases where valid arguments are passed. Notice that the new test passes in WebKit with and without this patch. SVGMatrix had two problems: -- The SVGMatrix.IDL was written incorrectly. -- The SVGMatrix methods were calling commitChange() even though the underlying SVGMatrixValue was not changing.
Simon Fraser (smfr)
Comment 7 2019-03-26 17:17:05 PDT
(In reply to Said Abou-Hallawa from comment #6) > (In reply to Simon Fraser (smfr) from comment #3) > > Comment on attachment 365978 [details] > > Patch > > > > This needs a test (preferably a WPT so that we can share with other > > browsers). > > LayoutTests/svg/dom/SVGMatrix.html tests only the cases where invalid > arguments are passed to the SVGMatrix methods. I cleaned the test and added > few cases where valid arguments are passed. > > Notice that the new test passes in WebKit with and without this patch. > SVGMatrix had two problems: > > -- The SVGMatrix.IDL was written incorrectly. > -- The SVGMatrix methods were calling commitChange() even though the > underlying SVGMatrixValue was not changing. Do we mate Chrome/FF behavior on this new test?
Simon Fraser (smfr)
Comment 8 2019-03-26 17:17:14 PDT
^match
Said Abou-Hallawa
Comment 9 2019-03-26 17:45:55 PDT
Chrome and FF almost match the WebKit behavior. You can see these differences if you open LayoutTests/svg/dom/SVGMatrix.html in Chrome or FireFox. -- FireFox throws exceptions with all invalid attribute values or invalid arguments. -- The exceptions' messages in both FF and Chrome are different from the WebKit messages. -- The calculation for tan(90) in FF is different from WebKit and Chrome.
Simon Fraser (smfr)
Comment 10 2019-03-26 18:04:44 PDT
Comment on attachment 366022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366022&action=review > LayoutTests/svg/dom/SVGMatrix.html:110 > +shouldBeEqualToString("matrixToString(matrix.translate(10, 20))", "{ a: 2, b: 0, c: 0, d: 1, e: 20, f: 220}"); > +shouldBeEqualToString("matrixToString(matrix.scale(5))", "{ a: 10, b: 0, c: 0, d: 5, e: 0, f: 200}"); > +shouldBeEqualToString("matrixToString(matrix.scaleNonUniform(2, 3))", "{ a: 4, b: 0, c: 0, d: 3, e: 0, f: 200}"); > +shouldBeEqualToString("matrixToString(matrix.skewX(90))", "{ a: 2, b: 0, c: 32662478706390740, d: 1, e: 0, f: 200}"); > +shouldBeEqualToString("matrixToString(matrix.skewY(90))", "{ a: 2, b: 16331239353195370, c: 0, d: 1, e: 0, f: 200}"); Don't we need a test that's like: let matrix = { a: 2, b: 0, c: 0, d: 1, e: 0, f: 200} // or whatever matrix.scale(2) shouldBeEqualToString("matrixToString(matrix, "{ a: 10, b: 0, c: 0, d: 5, e: 0, f: 200}");
Said Abou-Hallawa
Comment 11 2019-03-26 18:48:15 PDT
Comment on attachment 366022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366022&action=review >> LayoutTests/svg/dom/SVGMatrix.html:110 >> +shouldBeEqualToString("matrixToString(matrix.skewY(90))", "{ a: 2, b: 16331239353195370, c: 0, d: 1, e: 0, f: 200}"); > > Don't we need a test that's like: > > let matrix = { a: 2, b: 0, c: 0, d: 1, e: 0, f: 200} // or whatever > matrix.scale(2) > shouldBeEqualToString("matrixToString(matrix, "{ a: 10, b: 0, c: 0, d: 5, e: 0, f: 200}"); Sorry but I do not understand your suggestion. Did you mean we need to confirm that matrix was not changed by the previous calls? If this is the case, the following statements already do that: debug(""); debug("Check that the matrix is still containing the correct values"); shouldBeEqualToString("matrixToString(matrix)", "{ a: 2, b: 0, c: 0, d: 1, e: 0, f: 200}");
Said Abou-Hallawa
Comment 12 2019-04-01 11:20:50 PDT
Created attachment 366410 [details] Patch
WebKit Commit Bot
Comment 13 2019-04-01 11:46:50 PDT
Comment on attachment 366410 [details] Patch Clearing flags on attachment: 366410 Committed r243703: <https://trac.webkit.org/changeset/243703>
WebKit Commit Bot
Comment 14 2019-04-01 11:46:52 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.