Bug 196263

Summary: SVGMatrix.IDL methods do not conform to the specs
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, rniwa, simon.fraser, thorton, webkit-bug-importer, zimmermann
Priority: P2 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

Description Said Abou-Hallawa 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.
Comment 1 Said Abou-Hallawa 2019-03-26 11:41:23 PDT
Created attachment 365978 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-03-26 11:49:12 PDT
<rdar://problem/49283958>
Comment 3 Simon Fraser (smfr) 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).
Comment 4 Said Abou-Hallawa 2019-03-26 16:22:21 PDT
Created attachment 366020 [details]
Patch
Comment 5 Said Abou-Hallawa 2019-03-26 16:46:10 PDT
Created attachment 366022 [details]
Patch
Comment 6 Said Abou-Hallawa 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.
Comment 7 Simon Fraser (smfr) 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?
Comment 8 Simon Fraser (smfr) 2019-03-26 17:17:14 PDT
^match
Comment 9 Said Abou-Hallawa 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.
Comment 10 Simon Fraser (smfr) 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}");
Comment 11 Said Abou-Hallawa 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}");
Comment 12 Said Abou-Hallawa 2019-04-01 11:20:50 PDT
Created attachment 366410 [details]
Patch
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2019-04-01 11:46:52 PDT
All reviewed patches have been landed.  Closing bug.