Last Comment Bug 405300 - <canvas> isPointInPath doesn't transform its coordinates
: <canvas> isPointInPath doesn't transform its coordinates
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: x86 Linux
-- normal with 2 votes (vote)
: mozilla7
Assigned To: :Benjamin Peterson
: Milan Sreckovic [:milan]
: 589555 (view as bug list)
Depends on:
Blocks: 622842 ietestcenter
  Show dependency treegraph
Reported: 2007-11-25 04:45 PST by Ilmari Heikkinen
Modified: 2011-08-23 05:44 PDT (History)
17 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Transform coords to device-space before cairo_in_fill-check (943 bytes, patch)
2007-11-29 21:45 PST, Ilmari Heikkinen
no flags Details | Diff | Splinter Review
JS workaround to normalize isPointInPath behaviour across browsers (3.35 KB, text/plain)
2008-01-08 00:07 PST, Ilmari Heikkinen
no flags Details
isPointInPath with transformations testcase (2.86 KB, text/html)
2008-03-31 03:06 PDT, Jordan Osete
no flags Details
updated patch (4.05 KB, patch)
2011-05-19 09:53 PDT, :Benjamin Peterson
joe: review+
roc: superreview+
Details | Diff | Splinter Review

Description User image Ilmari Heikkinen 2007-11-25 04:45:44 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); fi; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); fi; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1

When using ctx.isPointInPath(x,y), the x,y-coords should be multiplied by the current transformation matrix to have isPointInPath work for transformed paths. 

The WhatWG spec[1] wording is a bit confusing on this matter: 
"The isPointInPath(x, y) method must return true if the point given by the x and y coordinates passed to the method, when treated as coordinates in the canvas' coordinate space unaffected by the current transformation..."

The idea being that the x,y-coordinates are in canvas space and hence must be transformed to the current space to do the isPointInPath-test.


Reproducible: Always

Steps to Reproduce:
1. Change canvas transform (translate/rotate/scale.)
2. Use isPointInPath on a point that would have had its return value changed by the transform.

Actual Results:  
The return value didn't change.

Expected Results:  
The return value should've had changed.
Comment 1 User image Ilmari Heikkinen 2007-11-29 21:45:09 PST
Created attachment 290828 [details] [diff] [review]
Transform coords to device-space before cairo_in_fill-check
Comment 2 User image Anne (:annevk) 2007-11-30 10:45:28 PST
This bug is INVALID as far as I can tell.
Comment 3 User image Ilmari Heikkinen 2007-11-30 22:34:53 PST
Opera handles isPointInPath coordinates as untransformed coordinates. See test in URL with Firefox vs. Opera. With setTransform, Firefox's behaviour can be worked around.

Comment 4 User image Ilmari Heikkinen 2007-12-16 08:36:00 PST
My original explanation of "how to fix" is wrong. The patch works, but the math is: multiply mouse coords by the inverse of the current transformation matrix.  Which the patch does implicitly (changing the matrix changes the path's matrix as well, so transforming the path to the point's matrix equals to transforming the point to the path's matrix.)

It can be worked around on a web page by using setTransform or by manually keeping track of the current transformation matrix and doing multiply(invert(matrix), [x,y,1]). Latter way + bbox / circle test can then be used to implement a crude version of isPointInPath for browsers that lack it.
Comment 5 User image Vladimir Vukicevic [:vlad] [:vladv] 2008-01-07 15:58:54 PST
WONTFIX'ing this as per bug 380338 -- paths don't get transformed, points do.  Changing the transformation matrix has no effect on the path that's currently built up.
Comment 6 User image Ilmari Heikkinen 2008-01-08 00:07:51 PST
Created attachment 295907 [details]
JS workaround to normalize isPointInPath behaviour across browsers

Note that you need to keep track of the current transformation matrix on Firefox 2. Implementation is left to the reader. Cheers!
Comment 7 User image Jordan Osete 2008-03-31 03:06:42 PDT
Created attachment 312691 [details]
isPointInPath with transformations testcase

I don't understand why this still is a WONTFIX, but here is a simple testcase of isPointInPath with transformations, and a (very simple) workaround for Mozilla included.
Comment 8 User image Jordan Osete 2008-03-31 03:15:51 PDT
(Sorry, the circles should highlight when you move your mouse over them in the testcase.)
Comment 9 User image Charles Schmidt 2010-09-13 14:25:10 PDT
Comment #5 is incorrect. Paths are transformed "IF" there transform is set before the path is drawn. The transform should not be applied retroactively as was being requested in the referenced bug.  As per the specification the isPointInPath method should test the query point "AS IS" again the "transformed" Path.

Please reopen this issue!
Comment 10 User image Tim 2010-10-01 10:34:26 PDT
Please reopen this, all other modern browsers are implementing correctly and this functionality is critical to making canvas interactive (e.g. is my mouse inside this path after transforms have been applied). 


tested on IE9, Chrome, Opera, Safari (iphone and ipad)
Comment 11 User image Jordan Osete 2010-10-05 02:16:10 PDT
For those struggling with this, a "simple" workaround is to detect the bug, then replace isPointInPath() with a correct one (please excuse the poor formatting, just a copy/paste of old code):

var c2dp = CanvasRenderingContext2D.prototype;

//  special isPointInPath method to workaround Mozilla bug 405300
//      []
function isPointInPath_mozilla( x, y )
    this.setTransform( 1, 0, 0, 1, 0, 0 );
    var ret = this.isPointInPath_old( x, y );
    return ret;

//  test for the presence of the bug, and set the workaround function only if needed
var ctx = document.createElement( "canvas" ).getContext( "2d" );
ctx.translate( 50, 0 );
ctx.moveTo( 125, 50 );
ctx.arc( 100, 50, 25, 0, 360, false );
if( !ctx.isPointInPath( 150, 50 ) )
    c2dp.isPointInPath_old = c2dp.isPointInPath;
    c2dp.isPointInPath = isPointInPath_mozilla;
Comment 12 User image :Ms2ger (⌚ UTC+1/+2) 2011-01-31 11:55:57 PST
We should get either the spec or our implementation changed.
Comment 13 User image :Ms2ger (⌚ UTC+1/+2) 2011-02-01 04:40:18 PST
*** Bug 589555 has been marked as a duplicate of this bug. ***
Comment 15 User image :Benjamin Peterson 2011-05-19 09:53:25 PDT
Created attachment 533682 [details] [diff] [review]
updated patch

This is an updated patch.
Comment 16 User image Joe Drew (not getting mail) 2011-06-02 11:50:42 PDT
Comment on attachment 533682 [details] [diff] [review]
updated patch

Review of attachment 533682 [details] [diff] [review]:

What should we do about the spec issue? The spec needs to be changed if everyone else does something different.
Comment 17 User image :Benjamin Peterson 2011-06-02 12:12:47 PDT
Everyone else is doing something different, but they're all doing the same thing: following the spec.
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 17:00:20 PDT
Comment on attachment 533682 [details] [diff] [review]
updated patch

Review of attachment 533682 [details] [diff] [review]:

Benjamin's right. This change makes us follow the spec.
Comment 19 User image Dão Gottwald [:dao] 2011-06-03 00:06:13 PDT
Comment 20 User image Eric Shepherd [:sheppy] 2011-08-04 13:08:33 PDT
Documentation updated:

And mentioned on Firefox 7 for developers.
Comment 21 User image Mihaela Velimiroviciu (:mihaelav) 2011-08-23 05:44:42 PDT
Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

All tests mentioned in comment 7 , comment 10 and comment 14 passed. Marking bug as VERIFIED-FIXED.

Note You need to log in before you can comment on or make changes to this bug.