Bug 206019 – Setting currentScale to non-finite values should throw TypeError

• Component: SVG
• Status: RESOLVED FIXED

[Sunny He]

SVG2 spec 5.14.2 Interface SVGSVGElement (https://www.w3.org/TR/SVG2/struct.html#__svg__SVGSVGElement__currentScale) defines currentScale to be float, not unrestricted float. Chrome and Firefox throw TypeError if currentScale is set to NaN or Inf, WebKit silently sets currentScale to NaN.

[Radar WebKit Bug Importer]

<rdar://problem/58450454>

[Sunny He]

Created attachment 387247 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 387247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387247&action=review

> Source/WebCore/ChangeLog:6
> +        Align SVGElement currentScale definition to SVG2 section 5.14.2.

Please add a link to the specs: https://www.w3.org/TR/SVG2/struct.html#InterfaceSVGSVGElement.

> LayoutTests/svg/dom/set-currentScale-nonfinite.html:15
> +    svgvar.currentScale = 1.0;
> +    shouldBe('svgvar.currentScale', '1.0')

Maybe you need to add semicolon at the end each statement just for consistency.

> LayoutTests/svg/dom/set-currentScale-nonfinite.html:46
> +        testFailed('set currentScale to Infinity without error')

svgvar.currentScale is set to character 'a' not Infinity.

> LayoutTests/svg/dom/set-currentScale-nonfinite.html:49
> +            testPassed('set currentScale to Infinity throws TypeError')

Ditto.

[Sunny He]

Created attachment 387258 [details]
Patch

[Dean Jackson]

Comment on attachment 387258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387258&action=review

> LayoutTests/svg/dom/set-currentScale-nonfinite.html:24
> +    try {
> +        svgvar.currentScale = undefined;
> +        testFailed('set currentScale to undefined without error');
> +    } catch(e) {
> +        if (e instanceof TypeError)
> +            testPassed('set currentScale to undefined throws TypeError');
> +    }
> +    shouldBe('svgvar.currentScale', '1.0');

This is fine, but maybe you could have avoided the duplication:

const testIncorrectScaleValue = (value, description) => {
  try {
     svgvar.currentScale = value;
     testFailed(`Setting currentScale to ${description} should have thrown an exception.`);
  } catch (e) {
     if (e instanceof TypeError)
         testPassed(`Setting currentScale to ${description} threw an exception.`);
  }
  shouldBe('svgvar.currentScale', '1.0');
};

testIncorrectScaleValue(undefined, "undefined");
testIncorrectScaleValue(NaN, "NaN");
testIncorrectScaleValue(Infinity, "Infinity");
testIncorrectScaleValue("a", "a string");

[Sunny He]

Created attachment 388065 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388065 [details]
Patch

Clearing flags on attachment: 388065

Committed r254863: <https://trac.webkit.org/changeset/254863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.