Calling context.setTransform( 0, 0, 0, 0, 0, 0 ) breaks canvas even after calling setTransform with non-zero values.

NEW
Unassigned

Status

()

Core
Canvas: 2D
--
minor
8 years ago
6 years ago

People

(Reporter: Scott, Unassigned)

Tracking

({testcase})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

1003 bytes, text/html
Details
14.64 KB, patch
Joe Drew (not getting mail)
: review-
Joe Drew (not getting mail)
: feedback?
jrmuizel
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.41 Safari/534.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 ( .NET CLR 3.5.30729; .NET4.0C)

After calling context.setTransform( 0, 0, 0, 0, 0, 0 ) on a canvas element's 2d context, subsequent calls to setTransform do not set the context's transformation matrix correctly. Drawing functions do not show expected results, even after resetting the transformation matrix to the identity via context.setTransform( 1, 0, 0, 1, 0, 0 ).

Reproducible: Always

Steps to Reproduce:
1. Call context.setTransform( 0, 0, 0, 0, 0, 0 );
2. Call context.setTransform( 1, 0, 0, 1, 0, 0 ); resetting to identity matrix should cause future draws to show expected image results
3. Call any context drawing function [ for the example case, I used ctx.fillRect( 10, 10, 80, 80 ); ]
4. Compare to another canvas who has not had setTransform( 0, 0, 0, 0, 0, 0 ) called
Actual Results:  
Canvas without the zero-transformation matrix shows a black square at (10,10) with width and height = 80; canvas with zero-transformation and subsequent reset to identity shows nothing.

Expected Results:  
Both canvasses should have shown the same black square.

Test script:

<html>
<head>
	<title>Firefox Zero Transformation Matrix Bug</title>
	<script type="text/javascript">
		window.onload = function()
		{
			// get the contexts
			var cnv1 = document.getElementById( 'test1' ),
				ctx1 = cnv1.getContext('2d'),
				cnv2 = document.getElementById( 'test2' ),
				ctx2 = cnv2.getContext('2d');
				
			// draw a rect to the first context
			// set the transform to identity (to show that ctx2's call to the same function does not have the same result)
			ctx1.setTransform( 1, 0, 0, 1, 0, 0 );
			ctx1.fillRect( 10, 10, 80, 80 );
			
			// here's where the problem is
			ctx2.setTransform( 0, 0, 0, 0, 0, 0 );
			// set to identity
			ctx2.setTransform( 1, 0, 0, 1, 0, 0 );
			ctx2.fillRect( 10, 10, 80, 80 );
		}
	
	</script>
	<style type="text/css">
		canvas{ border: 1px solid black; }
	</style>
</head>
<body>

<canvas id="test1" width="100" height="100"></canvas>
<canvas id="test2" width="100" height="100"></canvas>

</body>
</html>
(Reporter)

Comment 1

8 years ago
Created attachment 490433 [details]
working test case

This attachment shows the test case where one context has had a zero-transformation matrix set via setTransform against another canvas. The first canvas has not had the setTransform called that breaks the canvas.
Keywords: testcase

Comment 2

7 years ago
Created attachment 533664 [details] [diff] [review]
raise an error on singular matrices

The problem is that trying to set a singular matrix as the transform causes cairo to error out and refuse to do anymore paint operations. This patch makes transform() and setTransform() error out on singular matrices. (I choose this behavior since the spec is silent on it.) It also makes setTransform() consistent with transform() by silently returning on args of Infinity or Nan.
Assignee: nobody → benjamin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #533664 - Flags: review?(joe)

Updated

7 years ago
Duplicate of this bug: 531846
Comment on attachment 533664 [details] [diff] [review]
raise an error on singular matrices

Review of attachment 533664 [details] [diff] [review]:
-----------------------------------------------------------------

I like this. Because it's technically an interface change, though, I'd like roc to sr.

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +1427,5 @@
>  NS_IMETHODIMP
>  nsCanvasRenderingContext2D::SetTransform(float m11, float m12, float m21, float m22, float dx, float dy)
>  {
>      if (!FloatValidate(m11,m12,m21,m22,dx,dy))
> +        return NS_OK;

Can you make SetTransform behave the same as Transform in a separate patch? It can be committed separately.

::: content/canvas/test/test_canvas.html
@@ +21342,5 @@
> +  try {
> +      ctx.setTransform(0, 0, 0, 0, 0, 0);
> +      ok(false, "setTransform didn't throw");
> +  } catch (e if e instanceof DOMException) {
> +      ok(e.code == DOMException.SYNTAX_ERR);

No "wrong exception"?
Attachment #533664 - Flags: superreview?(roc)
Attachment #533664 - Flags: review?(joe)
Attachment #533664 - Flags: review+
Shouldn't we just handle singular transforms correctly instead of throwing? That means while the transform is singular, drawing operations shouldn't paint anything (except for effects due to non-over operators). That should fix bug 659201 too.

Comment 6

7 years ago
(In reply to comment #5)
> Shouldn't we just handle singular transforms correctly instead of throwing?
> That means while the transform is singular, drawing operations shouldn't
> paint anything (except for effects due to non-over operators). That should
> fix bug 659201 too.

Is that a useful behavior to have? I see this is what WebKit does, but I'm not sure where one would use it.
(In reply to comment #5)
> Shouldn't we just handle singular transforms correctly instead of throwing?
> That means while the transform is singular, drawing operations shouldn't
> paint anything (except for effects due to non-over operators). That should
> fix bug 659201 too.

I would agree with that, it is quite standard to not generate exceptions or otherwise halt execution on floating point error-cases (like divide by zero). For example this is why by default FPU exceptions don't raise signals.
(In reply to comment #6)
> Is that a useful behavior to have? I see this is what WebKit does, but I'm
> not sure where one would use it.

Imagine you're animating the transform and it happens to pass through a singular value. You don't want to suddenly have your script get an exception. You'd end up having to test for a singular matrix yourself, which would suck.

Also, even when there isn't a sane behavior we tend to not throw exceptions because it's the "bottom of the slippery slope" state: if some browsers throw exceptions and others don't, the ones that don't seem to work better for users. But in this case there is a perfectly sane behavior, so we should just use it.

Comment 9

7 years ago
Created attachment 545314 [details] [diff] [review]
ignore all drawing operations when a singular transform is in effect

Here is a simple patch with a hopefully comprehensive test.
Attachment #533664 - Attachment is obsolete: true
Attachment #545314 - Flags: review?(joe)
Attachment #533664 - Flags: superreview?(roc)
Comment on attachment 545314 [details] [diff] [review]
ignore all drawing operations when a singular transform is in effect

I really don't like having to sprinkle if (singular) all over the place. Can we not fix Cairo to allow us to get a cairo_t out of permanent error state after setting a singular matrix?
Attachment #545314 - Flags: review?(joe) → review-

Comment 11

7 years ago
I'm not sure(In reply to comment #10)
> Comment on attachment 545314 [details] [diff] [review] [review]
> ignore all drawing operations when a singular transform is in effect
> 
> I really don't like having to sprinkle if (singular) all over the place. Can
> we not fix Cairo to allow us to get a cairo_t out of permanent error state
> after setting a singular matrix?

I'm not sure. Can't cairo being in an error state cause the whole context to be put in an invalid state such that simply changing the error status won't work?

FWIW, this is nearly exactly what WebKit does, too.

Comment 12

7 years ago
Moreover, suppose we could reset cairo's error state. Where would we do it? We can't do it after someone sets a singular matrix because then drawing operations would use a different transform. We can't reset the error state when the singular matrix is removed because some cairo operations like save/restore and setting styles must still be supported, and cairo refusing to do anything when it's in error.
Comment on attachment 545314 [details] [diff] [review]
ignore all drawing operations when a singular transform is in effect

Jeff, what do you think?
Attachment #545314 - Flags: feedback?(jmuizelaar)

Comment 14

7 years ago
Jeff?

Comment 15

7 years ago
FWIW, the HTML5 spec now explicitly states how to handle singular transformations.

Updated

7 years ago
QA Contact: canvas.2d → jmuizelaar

Updated

6 years ago
Assignee: benjamin → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.