Closed Bug 471281 Opened 16 years ago Closed 15 years ago

Canvas arcTo Rendering Incorrectly in Firefox - Works in Safari

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: tag+bugzilla, Assigned: krit)

References

Details

Attachments

(3 files, 6 obsolete files)

a drawing to the implementation
15.18 KB, image/svg+xml
Details
Testcase
822 bytes, text/html
Details
arcTo-code fix for all platforms
4.04 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5

In trying to build a simple rectangle with rounded corners I think I have found a bug with the arcTo method of the canvas API in Firefox. The sample code I have provided works fine in Safari and as far as I can tell, my code is correct (hard to say for certain - documentation of the canvas API is rarer than hens' teeth). In Firefox, the corners are inexplicably shot off in a direction I didn't ask for.

Reproducible: Always

Steps to Reproduce:
1.Create the following HTML page:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
	<head>
		<title>Bug Test Case</title>
	</head>
	<body>
		<canvas id="foo" width="300" height="300"></canvas>
		<script>
			var ctx = document.getElementById('foo').getContext('2d');
			ctx.beginPath();
			ctx.moveTo(120, 100);
			ctx.lineTo(180, 100);
			ctx.arcTo(200, 100, 200, 120, 20);
			ctx.lineTo(200, 180);
			ctx.arcTo(200, 200, 180, 200, 20);
			ctx.lineTo(120, 200);
			ctx.arcTo(100, 200, 100, 180, 20);
			ctx.lineTo(100, 120);
			ctx.arcTo(100, 100, 120, 100, 20);
			ctx.fill();
		</script>
	</body>
</html>

2. View the page in Firefox, then in Safari
Actual Results:  
A square with rounded corners

Expected Results:  
A square with two rounded corners and two corners that are pushed way out from the edges of the square.
Component: General → Layout: Canvas
Product: Firefox → Core
QA Contact: general → layout.canvas
yes, FireFox doesn't follow the specification here. The author seems to missunderstand the specification. eg x2, y2 should never be added to the path:
>  mThebes->LineTo(gfxPoint(x2, y2));
There are two tangents. One from currentPoint to x1,y1 and one from (x1,y1) to (x2,y2). The points where the circle touches the tangents, are the points the arc is drawn to. The point on the tangent (x1,y1) to (x2,y2), where the circle touches the tangent, should be the last point on the path. The circle is only discribed by the radius, that means the points, where the circle touches the tangents depends on the radius of the circle and the angle between the two tangents.
Attached patch arcTo-code (obsolete) — — Splinter Review 
I haven't compiled the code yet. But the code base itself works and should give an idea how to implement arcTo.
This is a drawing to the implementation. I hope it helps to understand the code.
Attached patch arcTo-code (obsolete) — — Splinter Review 
This implementation matches the specification of HTML5 and arcTo (http://www.whatwg.org/specs/web-apps/current-work/#dom-context-2d-arcto).
I tested it with http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.arcTo.html (the tests included in the test-folder), http://canvex.lazyilluminati.com/misc/arcto.html and my own tests.
Attachment #354627 - Attachment is obsolete: true
Attachment #354691 - Flags: review?
Attached patch arcTo-code (obsolete) — — Splinter Review 
sorry, forgot a check if angle1 should be greater or lower pi. The test above works now too.
Attachment #354691 - Attachment is obsolete: true
Attachment #354694 - Flags: review?
Attachment #354691 - Flags: review?
Schulze,

Delighted to see how quickly you picked this up and sorted it out - such is the wonderfulness of open source projects (can you imagine how long Microsoft would have taken, if ever, to sort out such a bug in IE?)

Unfortunately my maths skills are nowhere near up to the level needed to resolve an issue like this otherwise I would have had a go myself. Is this fix likely to make it into Firefox 3.0.6?
(In reply to comment #6)
> Unfortunately my maths skills are nowhere near up to the level needed to
> resolve an issue like this otherwise I would have had a go myself. Is this fix
> likely to make it into Firefox 3.0.6?

Do you ask me if it is ready for Firefox 3.0.6? It's my first patch for Firefox, but isn 3.0.x only for security issues?
Otherwise I'll explain the code to everyone, who want's to review it. There are only basic skills in vector analysis needed.
> isn't 3.0.x only for security issues?

I have no idea - I'm a web developer, not a web browser developer ;) I reported this because it was causing me a problem as a user of the browser. I'll take your word on the level of skills in vector analysis needed because I only just about know what a vector is!

Anyway, I hope it does make it into the 3.0.x branch of Firefox - the sooner it's fixed, the sooner I can start using it. I see the status of this bug remains "UNCONFIRMED" - how does it get confirmed?
Any updates on this? Is this really still not fixed 3 months later?
(In reply to comment #2)
> Created an attachment (id=354627) [details]
> arcTo-code
> 
> I haven't compiled the code yet. But the code base itself works and should give
> an idea how to implement arcTo.

This bug is still broken, so it seems. Is there anything you can do.. or anything I can do to get this fixed? Looking forward to hearing from you.

Al
(In reply to comment #10)
> This bug is still broken, so it seems. Is there anything you can do.. or
> anything I can do to get this fixed? Looking forward to hearing from you.
> 
> Al

Well, I uploaded a patch and set the review tag. There is nothing else I can do. We just need a reviewer. I wrote it above, this is my first patch for Firefox. I don't know any reviewer who can look at this patch.
I can review this -- can you reupload as a unified diff though? (diff -u)  If not I can probably convert the context diff, but unified diffs are usually the norm for patches.  Thanks!
Thanks Vlad! Getting this fixed would make FireFox's ArcTo work with all the Canvas tutorials out there. At the moment people are getting confused about what ArcTo is "meant" to do, thinking that FireFox is doing it correctly.
Attached patch arcTo-code unified (obsolete) — — Splinter Review 
Ok, this is the unified version of the same patch.
Attachment #354694 - Attachment is obsolete: true
Attachment #367258 - Flags: review?
Attachment #354694 - Flags: review?
ah sorry. The other way around :-P ... uploading a new version in a bit.
The right direction this time. sorry again.
Attachment #367258 - Attachment is obsolete: true
Attachment #367259 - Flags: review?
Attachment #367258 - Flags: review?
Any news on this bug? Will it be coming out in the next FF update?
Attachment #367259 - Flags: review? → review?(jmuizelaar)
Attached file Testcase — 
This is the test case that the reporter attached. Gotta figure out some way to test this in an automated way.
Status: UNCONFIRMED → NEW
Ever confirmed: true
It looks like we already have a bunch of arcto tests that are currently todo'd. that I'm hoping this will fix. The seem to test a bunch of things so this testcase probably isn't needed.
(In reply to comment #19)
> It looks like we already have a bunch of arcto tests that are currently todo'd.
> that I'm hoping this will fix. The seem to test a bunch of things so this
> testcase probably isn't needed.

Where are this testcases? Can i test them and are there result images, how it should look like? I found some examples in Web as well as some stress tests. The patch worked for all of them. But I didn't find examples of Mozilla.
The tests in the mozilla tree are basically a copy of these:
http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.arcTo.html
(In reply to comment #21)
> The tests in the mozilla tree are basically a copy of these:
> http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.arcTo.html

ah ok, I tested them :-)
Does http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.html
pass for you?

When running the mochitests on try server I get:
15960 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.transformation.html | pixel 0,0 is 63,192,0,255; expected 0,255,0,255 +/- 0
15961 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.transformation.html | pixel 50,0 is 255,0,0,255; expected 0,255,0,255 +/- 0
15962 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.transformation.html | pixel 99,0 is 255,0,0,255; expected 0,255,0,255 +/- 0
15964 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.transformation.html | pixel 50,25 is 63,192,0,255; expected 0,255,0,255 +/- 0
15965 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.transformation.html | pixel 99,25 is 255,0,0,255; expected 0,255,0,255 +/- 0
15968 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.transformation.html | pixel 99,49 is 191,64,0,255; expected 0,255,0,255 +/- 0
I tested it with trunk, and this test did not fail.

Failing tests on Philip's test suite are:

2d.​path.​arcTo.​collinear.​2
2d.​path.​arcTo.​collinear.​3
fail, but the spec will change here. It wants to draw infinite lines.

2d.​path.​arcTo.​nonfinite
fails with "Aborted with exception: An invalid or illegal string was specified". The spec wants us to return silently here.

2d.​path.​arcTo.​emptysubpath
is not related to arcTo
2d.path.arcTo.transformation fails with this patch for me on os x. I've not tried it on other platforms.

I get the following:
http://people.mozilla.com/~jmuizelaar/arcto.png
Maybe other transformation tests fail for you on Mac too? It's a bit strange that it works on linux, but not on Mac. I haven no possibilities to test it on windows.
Can it be a bug in cairo? Or different versions of cairo? I followed the instruction for downloading and building firefox. Does the build process take the cairo dev's of my linux system, or the cairo source you get with firefox?

I get the following on linux:
http://www1.inf.tu-dresden.de/~s7468461/screenshot-on-linux.png
(In reply to comment #25)
> 2d.path.arcTo.transformation fails with this patch for me on os x. I've not
> tried it on other platforms.
> 
> I get the following:
> http://people.mozilla.com/~jmuizelaar/arcto.png

Can you please post a screenshot of http://www1.inf.tu-dresden.de/~s7468461/arcto.html ? This makes it easier to track down the problem.
(In reply to comment #26)
> Maybe other transformation tests fail for you on Mac too? It's a bit strange
> that it works on linux, but not on Mac. I haven no possibilities to test it on
> windows.
> Can it be a bug in cairo? Or different versions of cairo? I followed the
> instruction for downloading and building firefox. Does the build process take
> the cairo dev's of my linux system, or the cairo source you get with firefox?

It should use the cairo source you get with firefox unless you spefically enable system-cairo. But it could certainly be a bug in cairo but it looks like it's not specific to your system. The try server's results match what we've seen. It passes on linux and fails on os x and win32. The cairo code paths on OS X and linux are pretty different so I'm less surprised by the mismatch there, but the fact that linux and win32 don't match does surprise me.

> I get the following on linux:
> http://www1.inf.tu-dresden.de/~s7468461/screenshot-on-linux.png

(In reply to comment #27)
> Can you please post a screenshot of
> http://www1.inf.tu-dresden.de/~s7468461/arcto.html ? This makes it easier to
> track down the problem.

Here you go:
http://people.mozilla.com/~jmuizelaar/arcto2.png

and here's how it looks without your patch (which passes the transformation tests):
http://people.mozilla.com/~jmuizelaar/arcto2-old.png
arcto2.png is the way it should look like. And that the old code passes the test is luck.
Glad the bug is finally out of luck! If you guy need me to test here I can. I have PC: Vista, XP, Ubuntu & MAC: Leopard, Tiger & whatever is running on the old G4 (probably tiger again).
Attached patch arcTo-code fix for all platforms (obsolete) — — Splinter Review 
This should hopefully work for all platforms now.
Attachment #367259 - Attachment is obsolete: true
Attachment #368533 - Flags: review?
Attachment #367259 - Flags: review?(jmuizelaar)
This code is based on Philip Taylers arcTo emulation, written in Canvas/JavaScript. Both patches, this one as well as the last one should work on all platforms.
But this code is cleaner and makes use of arctan instead of arccos and therefor is saver.
Attachment #368533 - Attachment is obsolete: true
Attachment #368591 - Flags: review?
Attachment #368533 - Flags: review?
Attachment #368591 - Flags: review? → review?(jmuizelaar)
(In reply to comment #32)
> Created an attachment (id=368591) [details]
> arcTo-code fix for all platforms
> 
> This code is based on Philip Taylers arcTo emulation, written in
> Canvas/JavaScript. Both patches, this one as well as the last one should work
> on all platforms.
> But this code is cleaner and makes use of arctan instead of arccos and therefor
> is saver.

It passes the tests that it should for me on OS X. Nice work all.
I've been testing the .arcto() in the nightly and it still looks broken. How long would it be until these fixes make it through to the Nightly?
It'll be in the trunk nightlies shortly, I am reliably informed, and then very very likely in Firefox 3.5.1.  Apologies for the delay!
Flags: wanted1.9.1?
(In reply to comment #35)
> It'll be in the trunk nightlies shortly, I am reliably informed, and then very
> very likely in Firefox 3.5.1.  Apologies for the delay!

No problem. I'm realy happy when this get fixed :-)
Awesome! In one of the Firefox 35 days projects I mention that this is not fixed yet. Would look cool to have it fixed and display a message to the user that Moz squashed the bug as part of the release.

It will also make parsing SVGs in the DOM much quicker when rendering them to the Canvas, as you wont have to recalculate the path to "fake" an arcTo.
Attachment #368591 - Flags: review?(jmuizelaar) → review+
Comment on attachment 368591 [details] [diff] [review]
arcTo-code fix for all platforms

Sorry for dropping the ball here re: review. I remember giving this a second review, but must have never hit submit.

Looks good. It would be nice if there were some more explanatory comments about how it worked, but that's not critical.
I can put together a page with some test cases on it and we can make sure it's hammered down. I can make it so it loads arcTo's from InkScape so we can test it in some different situations really quickly. I think I will have some time this weekend, so I will link to something here soon.
http://hg.mozilla.org/mozilla-central/rev/b116b49459f8
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Was getting failures on mac. No idea why as it passed on try.

Error 0 (Unknown error: 0).16263 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.coincide.1.html | pixel 50,1 is 255,0,0,255; expected 0,255,0,255 +/- 0
16264 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.coincide.1.html | pixel 50,25 is 255,0,0,255; expected 0,255,0,255 +/- 0
16265 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.coincide.1.html | pixel 50,48 is 255,0,0,255; expected 0,255,0,255 +/- 0
16271 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.collinear.1.html | pixel 50,25 is 255,0,0,255; expected 0,255,0,255 +/- 0
16311 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.curve1.html | pixel 80,45 is 255,0,0,255; expected 0,255,0,255 +/- 0
16312 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.curve1.html | pixel 80,46 is 255,0,0,255; expected 0,255,0,255 +/- 0
16315 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.curve2.html | pixel 50,25 is 255,0,0,255; expected 0,255,0,255 +/- 0
16318 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.curve2.html | pixel 55,21 is 121,134,0,255; expected 0,255,0,255 +/- 0
16321 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.curve2.html | pixel 72,28 is 4,251,0,255; expected 0,255,0,255 +/- 0
16331 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.end.html | pixel 1,48 is 255,0,0,255; expected 0,255,0,255 +/- 0
16332 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.end.html | pixel 50,25 is 255,0,0,255; expected 0,255,0,255 +/- 0
16334 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.end.html | pixel 98,48 is 255,0,0,255; expected 0,255,0,255 +/- 0
16338 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.start.html | pixel 1,48 is 255,0,0,255; expected 0,255,0,255 +/- 0
16341 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.shape.start.html | pixel 98,48 is 255,0,0,255; expected 0,255,0,255 +/- 0
16355 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.zero.1.html | should not throw exception
16358 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.path.arcTo.zero.2.html | should not throw exception
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The test didn't fail on the normal unittest box, only on the "Mochitest" box from the Firefox-Unittest tree. And even on that box it only failed in 4 of 6 runs, as far as I can tell.
What is the difference between this two test boxes?
The Mochitest-only boxes are new, and might be not ready yet.
Three hours ago, the Mac Mochitest box went orange again; this time with TEST-UNEXPECTED-PASS in the arcTo tests:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1245300543.1245302381.22395.gz

How can this be possible? Does it mean that the box tested the wrong build?
(In reply to comment #45)
> The Mochitest-only boxes are new, and might be not ready yet.
> Three hours ago, the Mac Mochitest box went orange again; this time with
> TEST-UNEXPECTED-PASS in the arcTo tests:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1245300543.1245302381.22395.gz
> 
> How can this be possible? Does it mean that the box tested the wrong build?

As far as I understand it, the only difference between the mochitest-only box and the main unittest box is that the mochitest-only box tests the same bits from the main "build" box.  The unittest box builds its own version of the code using a different mozconfig than the standard "build" box uses.  I believe the only difference in those two mozconfigs would be --enable-tests and the flags to enable leak logging.

Both are testing optimized builds, so it isn't a opt vs. debug issue.  It might be possible that the mochitest only box pulled the wrong build (from the build box) to test.

CC'ing catlee.
This should be fixed by the landing of bug 499161
Relanded as:
http://hg.mozilla.org/mozilla-central/rev/e97819582eed
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
This still does not seem to work for this example: http://canvex.lazyilluminati.com/misc/arcto.html

It appears that Opera is the only vendor for which the arcTo() working as expected across the board. Perhaps someone from SVG could verify whether the example above is getting the expected results?
Alistair, why is SVG relevant here?

Note that SVG specifies arcs via endpoints, while canvas specifies them via control points.  It looks like Opera is doing endpoint-specified arcs in canvas, which is wrong.
Assignee: nobody → vbs85
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: