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

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

({regression})

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 unaffected, firefox26 unaffected, firefox27- fixed, firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(5 attachments, 2 obsolete attachments)

Posted image 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).
SVGContentUtils:ParseNumber should support scientific notation you could put a breakpoint there when its called by ParseArguments.
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
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Attachment #8342903 - Flags: review?(jwatt)
Summary: gradientTransform no longer supports exponential/scientific notation → Parsing of numbers in scientific notation in SVG is broken when there is no sign after the 'e'
Attachment #8342903 - Flags: review?(jwatt) → review+
You'll need a separate beta/aurora patch as the code is slightly different there.
(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!)
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.
(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.
There's a silly bug in the test case from the previous patch.
Attachment #8343566 - Flags: review?(longsonr)
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.
And after refreshing...
Attachment #8343583 - Attachment is obsolete: true
Attachment #8343566 - Flags: review?(longsonr) → review+
https://hg.mozilla.org/mozilla-central/rev/f5eecc6d6dd9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Patch to fix parsing on Aurora branch (v27)
Attachment #8343588 - Attachment is obsolete: true
Attachment #8344423 - Flags: review?(longsonr)
Depends on: 919319
Attachment #8344423 - Flags: review?(longsonr) → review+
Test case for length parsing for trunk
Attachment #8344433 - Flags: review?(longsonr)
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
Attachment #8344423 - Flags: approval-mozilla-aurora?
Attachment #8344433 - Flags: review?(longsonr) → review+
Attachment #8344433 - Attachment description: fixLengthParsing → Regression test for length parsing
Pushed length parsing regression test to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/031d2c4386da
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.
Attachment #8344423 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8344423 - Flags: approval-mozilla-beta?
Attachment #8344423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm approving this low risk uplift. Thanks!
(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
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.