946529 - Parsing of numbers in scientific notation in SVG is broken when there is no sign after the 'e'

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: test graphic

A gradientTransform value of the form "matrix(-.7235 .6903 .6903 .7235 -2050 1.14e4)" no longer parses but reports "Unexpected value matrix(-.7235 .6903 .6903 .7235 -2050 1.14e4) parsing gradientTransform attribute."

Using the attached test graphic I get a regression range of:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f0d363d72753&tochange=abe6790a5dd8

which points to bug 929011.

(The tail of foxkeh should have a gradient if the transform is parsed correctly.)

According to SVG 1.1F2 scientific notation should be supported for <number> when used in an attribute (http://www.w3.org/TR/SVG11/types.html#DataTypeNumber).

Comment 1

[Robert Longson [:longsonr]]

SVGContentUtils:ParseNumber should support scientific notation you could put a breakpoint there when its called by ParseArguments.

Comment 2

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1a

Proposed patch to fix parsing of numbers using scientific notation of the form 1e1 (i.e. no sign between the 'e' and following digit).

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=ff1f2dbaef2b

Comment 3

[Robert Longson [:longsonr]]

You'll need a separate beta/aurora patch as the code is slightly different there.

Comment 4

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #3)
> You'll need a separate beta/aurora patch as the code is slightly different
> there.

Thanks for the review Robert! I thought this code was only in nightly?
(By the way I would have asked you for review but I thought you weren't around--so thanks!)

Comment 5

[Brian Birtles (:birtles)]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5eecc6d6dd9

Comment 6

[Robert Longson [:longsonr]]

Bug 919319 was the start of my parsing updates. Has an earlier version of this code. So that's aurora but not beta I think.

Comment 7

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #6)
> Bug 919319 was the start of my parsing updates. Has an earlier version of
> this code. So that's aurora but not beta I think.

Oh, you're right. I was only testing transform lists (which are fine on Aurora) but numbers in other places will be buggy on Aurora.

Comment 8

[Brian Birtles (:birtles)]

Attachment: Fix test case from previous patch

There's a silly bug in the test case from the previous patch.

Comment 9

[Brian Birtles (:birtles)]

Attachment: (WIP) Patch to test length parsing

I started working on fixing the number parsing on Aurora but I'm having trouble with building and I'm not going to be able to fix it before the weekend.

I've attached a WIP patch of the regression test for this. The idea is to:
- Land the test patch on trunk
- Make another patch with this test for Aurora and basically fix the parsing same way as in attachment 8342903 [details] [diff] [review] and land that on Aurora

I'll pick it up on Monday and see if I make it before the merge.

Comment 10

[Brian Birtles (:birtles)]

Attachment: (WIP) Patch to test length parsing

And after refreshing...

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5eecc6d6dd9

Comment 12

[Brian Birtles (:birtles)]

Pushed test fix to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba0b5224e04

Comment 13

[Brian Birtles (:birtles)]

Attachment: Fix parsing of numbers on aurora

Patch to fix parsing on Aurora branch (v27)

Comment 14

[Brian Birtles (:birtles)]

Attachment: Regression test for length parsing

Test case for length parsing for trunk

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8344423 [details] [diff] [review]
Fix parsing of numbers on aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 919319
User impact if declined: SVG content that uses scientific notation for numbers or lengths will not render correctly
Testing completed (on m-c, etc.): Similar patch landed on m-c 2~3 days ago. Includes test case.
Risk to taking this patch (and alternatives if risky): No obvious risk.
String or IDL/UUID changes made by this patch: None

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ba0b5224e04

Comment 17

[Brian Birtles (:birtles)]

Pushed length parsing regression test to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/031d2c4386da

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/031d2c4386da

Comment 19

[Benjamin Kerensa [:bkerensa]]

We are probably not going to see too many users impacted by this but I'm going to go ahead and approve the low risk uplift.

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8344423 [details] [diff] [review]
Fix parsing of numbers on aurora

Transferring the previous approval request to beta since it didn't get approved in time for the merge.

Note that I think this will affect much content since the popular SVG scour tool compacts SVG coordinates using this notation.[1]

[1] http://www.codedread.com/scour/ops.php

Comment 21

[Benjamin Kerensa [:bkerensa]]

I'm approving this low risk uplift. Thanks!

Comment 22

[Brian Birtles (:birtles)]

(In reply to Benjamin Kerensa [:bkerensa] from comment #21)
> I'm approving this low risk uplift. Thanks!

Thank you!

https://hg.mozilla.org/releases/mozilla-beta/rev/6bbfc4a81d06