Bug 206019

Summary:

Setting currentScale to non-finite values should throw TypeError

Product:

WebKit

Reporter:

Sunny He <sunny_he>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, darin, dino, esprehn+autocc, ews-watchlist, fmalita, gyuyoung.kim, kondapallykalyan, pdr, sabouhallawa, schenney, sergio, webkit-bug-importer, zimmermann

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Sunny He 2020-01-09 11:21:57 PST

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.

Comment 1 Radar WebKit Bug Importer 2020-01-09 11:22:23 PST

<rdar://problem/58450454>

Comment 2 Sunny He 2020-01-09 11:29:10 PST

Created attachment 387247 [details]
Patch

Comment 3 Said Abou-Hallawa 2020-01-09 12:40:54 PST

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.

Comment 4 Sunny He 2020-01-09 13:13:35 PST

Created attachment 387258 [details]
Patch

Comment 5 Dean Jackson 2020-01-11 12:50:51 PST

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");

Comment 6 Sunny He 2020-01-17 11:10:36 PST

Created attachment 388065 [details]
Patch

Comment 7 WebKit Commit Bot 2020-01-21 10:58:42 PST

Comment on attachment 388065 [details]
Patch

Clearing flags on attachment: 388065

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

Comment 8 WebKit Commit Bot 2020-01-21 10:58:44 PST

All reviewed patches have been landed.  Closing bug.