Closed Bug 1068603 Opened 10 years ago Closed 10 years ago

SVG PATH getTotalLength() returns 0 for Bezier curves (using C) which are really straight lines

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
32 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox32 --- wontfix
firefox33 + wontfix
firefox34 + wontfix
firefox35 - wontfix
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: xcr, Assigned: dholbert)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 1 obsolete file)

1068603.html
546 bytes, text/html
Details
fix v2 (with >=/<= instead of >/<, and with a test)
3.15 KB, patch
dholbert
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36 Steps to reproduce: Today a customer reported missing elements on a SVG canvas on a productive system and I was shocked when I realized something went wrong with Firefox. I was struggling with the bounding box for some hours to realize that it seems to be a bug with the path lengths instead. I created a minimal example for this report. Run the following page in Firefox 32.0.1: <head> </head> <body> <div> <svg width="300" height="300"> <path d="M 0 0 C 12.5 0 37.5 0 50 0 C 62.5 0 87.5 0 100 0" id="path1"/> <path d="M 0 0 L 12.5 0 37.5 0 50 0 L 62.5 0 87.5 0 100 0" id="path2"/> </svg> </div> <script> var n = 2, i, path; for (i = 1; i <= n; i++) { path = document.getElementById('path' + i); if (path) { console.log('path' + i, path); console.log('path' + i + '.length', path.getTotalLength()); } } </script> </body> </html> Actual results: Result: path1 <path id="path1" d="M 0 0 C 12.5 0 37.5 0 50 0 C 62.5 0 87.5 0 100 0"> path1.length 0 path2 <path id="path2" d="M 0 0 L 12.5 0 37.5 0 50 0 L 62.5 0 87.5 0 100 0"> path2.length 100 The length of the first path should not be 0. Expected results: Result: path1 <path id="path1" d="M 0 0 C 12.5 0 37.5 0 50 0 C 62.5 0 87.5 0 100 0"> path1.length 100 path2 <path id="path2" d="M 0 0 L 12.5 0 37.5 0 50 0 L 62.5 0 87.5 0 100 0"> path2.length 100 Instead of being 0, the length of the first path should also be 100.
Component: Untriaged → SVG
Product: Firefox → Core
Attached file 1068603.html —
Regression range: good=2014-05-31 bad=2014-06-01 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=323156681cef&tochange=d6fccb7c56a8 Suspected: Bas Schouten — Bug 992731: Correctly treat a 'line' as a curve with a single inflection point stretching across the entire line. r=jwatt
Blocks: 992731
Status: UNCONFIRMED → NEW
status-firefox32: --- → affected
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
Ever confirmed: true
Flags: needinfo?(bas)
Keywords: regression, testcase
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
Except if the fix is trivial and low-fix, I don't see that bug fixed for 33. Wontfix but don't hesitate to correct me if I am wrong.
status-firefox33: affected → wontfix
As we're pretty late in the beta cycle for 34, this is probably wontfix for 34 as well unless someone thinks it's an easy low risk fix. lmandel, your call... unless you want me to go ahead and decide on things like this.
I think it is a trivial fix: just check that the points are lined up, and if they are, return the length of the line (i.e. the distance between the first and last point).
The very same problem occurs also in getPointAtLength(). Open this page and see it: <!DOCTYPE html> <body> <div id='example'> <svg width="300" height="300"> <path id="path1" d="M0,0 C100,0 150,0 200,0" stroke="black" fill="none"/> </svg> </div> <span id=res></span> <script> var path = document.getElementById("path1"); document.getElementById("res").innerHTML = path.getTotalLength() + " " + path.getPointAtLength(50).x; </script> </body> </html>
This does not have a significant impact enough to continue tracking and it's not being prioritized for fixing. We can consider an uplift when a fix is ready based on risk, but I'm untracking for 35 after it's been wontfixed for several cycles.
tracking-firefox35: + → -
It has a significant impact, at least for my web pages that do not render properly the graphics because of that bug.
(In reply to Angelo Borsotti from comment #5) > I think it is a trivial fix: just check that the points are lined up, and if > they are, return the > length of the line (i.e. the distance between the first and last point). Seriously? Well, for a straight line that is correct but the problem is about curves. I just took the line as example to show an obvious example. (In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #7) > This does not have a significant impact enough to continue tracking and it's > not being prioritized for fixing. We can consider an uplift when a fix is > ready based on risk, but I'm untracking for 35 after it's been wontfixed for > several cycles. For our website it also has a huge impact as we can currently not properly use curves due to Firefox. Thus, I would vote to fix it ;)
In reply to Marcus: I saw the problem only when the path has all points lined up. If one point is not lined up, then the function returns the correct value. Being so, it should be sufficient to check that the points (e.g. the control points of a bezier courve) are all on the same line, and then compute the result. P.S. being the 1st of January, to you all: Happy New Year!
Blocks: 1116541
The problem here seems to be in mozilla::gfx::FlattenBezier() (which calls FindInflectionPoints, which is the function that was tweaked in the bug that caused this, bug 992731). With the values that are now returned by FindInflectionPoints (aT1=0, aCount=1), FlattenBezier now makes *zero calls to LineTo()*. So we end up with a 0-length flattened path (with no segments at all). This makes getTotalLength() etc. behave as if the path is 0-length. It looks like what's *supposed* to happen is: - FlattenBezier calls FindInflectionApproximationRange (which tells us what range can be linearly approximated). - That function sees that the whole curve can be approximated by a straight line, so it returns a range of t1Min=-1, t1Max=2 (values outside the 0,1 bounds, to indicate "everything", I think). - BUT, FlattenBezier never actually handles this configuration of t1Min/t1Max values. So we end up returning without calling LineTo at all. I suspect FlattenBezier just needs a check for t1Min < 0 and t1Max > 1, with a LineTo() call & an early-return.
Attached patch fix v1 (obsolete) — — Splinter Review
This fixes the testcases here (attachment & comment 6), and also fixes bug 1116541.
Assignee: bas → dholbert
Attachment #8543183 - Flags: review?(bas)
Comment on attachment 8543183 [details] [diff] [review] fix v1 Review of attachment 8543183 [details] [diff] [review]: ----------------------------------------------------------------- Nice work.
Attachment #8543183 - Flags: review?(bas) → review+
Comment on attachment 8543183 [details] [diff] [review] fix v1 Thanks for the quick review turnaround! >+ if (count == 1 && t1min < 0 && t1max > 1.0) { >+ // The whole range can be approximated by a line segment. Actually -- I'm realizing my checks here should probably be for <= 0 and >= 1 (including the endpoints) because the later checks seem to exclude the endpoints, as shown by the following contextual code: > if (t1min > 0) { > // Flatten the Bezier up until the first inflection point's approximation > // point. [...] > } > if (t1max >= 0 && t1max < 1.0 && (count == 1 || t2min > t1max)) { I'll make that tweak (changing my >/< to >=/<=) and carry forward r+, unless you have any objections. I'll also add some testcases before landing.
Flags: needinfo?(dholbert)
Can't you pull out the checks into some inline function and then use it both times?
Which checks?
Here's the updated fix, with comment 14 addressed (including the boundaries 0 and 1 in the new early-return case), and with a test added to the existing test_pathLength.html mochitest. (I also added some comments to clarify what we're already testing in the existing checks within that test.) I verified that the new mochitest "is()"-check fails before the fix is applied, and passes afterwards. Robert: per previous comment, I don't think I understood what you're suggesting. Perhaps it would be suitable for a followup? (If it involves some refactoring/cleanup, it might be better to do that separately, since this patch is pretty nicely targeted & backportable right now, which is good since this bug affects all branches.)
Attachment #8543183 - Attachment is obsolete: true
Flags: needinfo?(dholbert)
Attachment #8543353 - Flags: review+
I went ahead and landed, in the interests of getting this fixed, & backported to Aurora once it's been verified on trunk, in time for the merge in just over a week: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9458d466161 Robert, if you think there's some refactoring that needs doing (per comment 15), could you spin off a followup to cover that?
Flags: needinfo?(longsonr)
Try run, to bolster my innocence in case there's any orange on/around my push: https://tbpl.mozilla.org/?tree=Try&rev=292e96d7697f [setting needinfo=me to request backport approval after this has hit m-c]
Flags: needinfo?(dholbert)
I looked at the file and the logic is not as similar as I first thought. I don't think any further refactoring will make it any clearer.
Flags: needinfo?(longsonr)
Summary: SVG PATH getTotalLength() returns 0 for Bezier curves (using C) → SVG PATH getTotalLength() returns 0 for Bezier curves (using C) which are really straight lines
Flags: needinfo?(dholbert)
OS: Windows 7 → All
Hardware: x86_64 → All
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/b9458d466161
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8543353 [details] [diff] [review] fix v2 (with >=/<= instead of >/<, and with a test) Approval Request Comment [Feature/regressing bug #]: Bug 992731 [User impact if declined]: Broken SVG content, due to getPathLength/getPointAtLength returning incorrect values for curves that are really straight lines. [Describe test coverage new/current, TBPL]: Automated testcase included. Patch has made it to m-c (today's nightly). [Risks and why]: I think this is low-risk. This is a scenario we just flat-out didn't handle in the old code, and now we're handling it. (by creating a line-segment to approximate a curve which is itself really a straight line) [String/UUID change made/needed]: None
Flags: needinfo?(dholbert)
Attachment #8543353 - Flags: approval-mozilla-aurora?
FWIW: I verified that this bug's attached testcase & dependent bug 1116541's attached testcase are both working correctly in today's nightly. (The testcase URLs on the other dependent bugs (bug 1020255, bug 1055968) also work correctly, as well, but I'm not sure that this bug is actually what fixed them, as I noted in my comments on those bugs earlier this morning.)
No longer blocks: 1020255
No longer blocks: 1055968
I don't think we will take in 35.
status-firefox35: affected → wontfix
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8543353 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: