Bug 196263

Summary:

SVGMatrix.IDL methods do not conform to the specs

Product:

WebKit

Reporter:

Said Abou-Hallawa <sabouhallawa>

Component:

SVG

Assignee:

Said Abou-Hallawa <sabouhallawa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dino, rniwa, simon.fraser, thorton, webkit-bug-importer, zimmermann

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

191237, 196086

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (40.95 KB, patch) 2019-03-26 16:22 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (41.58 KB, patch) 2019-03-26 16:46 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (42.25 KB, patch) 2019-04-01 11:20 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
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.