293224 - [FIX]Matrix from getScreenCTM should be to initial viewport space

Status: VERIFIED FIXED

(Core :: SVG, defect, P1)

Description

[Andreas Neumann]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050504 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050504 Firefox/1.0+

The .getScreenCTM() method of the root element (documentElement) delivers wrong
values when a viewBox is involved.

Please have a look at
http://www.carto.net/neumann/temp/getscreenctm.svg

Clicking on the rectangle should reveal the scale factor and offset values that
should change after each window resize. Unfortunately that does not work on
Mozilla svg. No matter how a resize the window, mozilla svg always displays the
same values. The example works well in ASV6 and Batik.

That method is very important to me since almost all of my more complex examples
build on this method and won't work without it.

Reproducible: Always

Actual Results:  
in Mozilla native SVG the translation values seem to reflect the position of the
browser window within the main screen on the computer, but in fact they should
represent the offset of the viewBox system within the browser window. The scale
values are always at one, but they should reflect the resizing of the viewBox
coordinate system. Please compare with ASV6 or the newest Batik about the behaviour.

Comment 1

[Holger Will]

Attachment: testcase+javascript implementation of getScreenCTM()

Comment 2

[Holger Will]

any chnace we might see this in 1.5 ?
this is much more important than bug#272630 which this bug depends on.

Comment 3

[Holger Will]

Attachment: testcase

move your mouse around. the circle should be under the mouse, but it isnt.

Comment 4

[tor]

Testcase appears to be a different bug.

Comment 5

[Holger Will]

Attachment: testcase

ohh ! sorry, uploaded the wrong file ! here is the right testcase !

Comment 6

[tor]

Attachment: getScreenCTM changes

Needs testing for non-<svg:svg> elements.

Comment 7

[Holger Will]

Attachment: test nested g elements

a more complex testcase
drag around the white rect, it should allways stay under your mouse pointer.

Comment 8

[tor]

Attachment: a bit more cleanup

Comment 9

[Jonathan Watt [:jwatt]]

Holger: Okay, let's fix the bugs in your testcases first.

1) You're mixing up *screen* coordinates with *client* coordinates. You need to
use *screenX* and *screenY* when playing with getScreenCTM, not clientX and clientY.

2) Why are you calling getScreenCTM on the 'circle' element's parentNode? If
you're using the mouse coordinates to set attributes of the 'circle' element
then just call getScreenCTM on it.

I'll attach corrected testcases that should work in firefox shortly.

Andreas:

The SVG file you've linked to doesn't show how you're using getScreenCTM. Are
you also mixing up screen and client coordinates? Under what circumstances are
you finding it necessary to call getScreenCTM on an 'svg' element. I'd be *very*
interested to know of any scenarios where that's really what you want to do.
Holger, if you have any I'd like to hear from you too. :-)

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: Holger's first testcase

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: Holger's second testcase

Comment 12

[Holger Will]

i didnt know about screenX/Y, and im not sure i understand the difference to
clientX/Y, but it sounds reasonable to use screen coordinates with screenCTM !
unfortunatly using screenX/Y breaks the examples in Batik, but that migth be a
batik bug...

Comment 13

[Jonathan Watt [:jwatt]]

From a quick look at batik it seems it's getScreenCTM is more like what you
might call a getClientCTM method. :-/ Hence why things worked for you when using
the client coordinates.

Comment 14

[Holger Will]

yeah , but it seems to me that in ASV6 clientX/Y == screenX/Y.
so getScreenCTM() == "getClientCTM" in ASV6 and Batik.
if this is implemented correctly ( i do believe what you say is correct ), i see
no way to script anything like the testcase in a cross viewer manner.

Comment 15

[Jonathan Watt [:jwatt]]

Actually the fact that ASV6 uses client space for both screenX/Y *and*
getScreenCTM means that this isn't a problem in ASV. They might argue that their
"screen" is the client area, but who cares. As long as they remain consistant
you can use *screen*X/Y and things will work fine.

Batik on the other hand is a problem since screenX/Y isn't in the same space as
getScreenCTM. I've filed a bug for that.

http://issues.apache.org/bugzilla/show_bug.cgi?id=36165

Comment 16

[Jonathan Watt [:jwatt]]

The SVG WG are going to discuss this at their face-to-face next week, and Chris
is going to get back to me with their conclusion. I'd recommend you don't go to
a lot of effort to change any of your existing scripts until then.

Comment 17

[Holger Will]

(In reply to comment #9)

> use *screenX* and *screenY* when playing with getScreenCTM, not clientX and
clientY.

on further testing, i just realized, that this proposal still does not work
correctly, it seem getScreenCTM does not take currentTranslate/Scale into account. 

Comment 18

[Holger Will]

Attachment: another testcase

the first testcase with currentScale set to 0.5

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: Holger's first testcase

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: Holger's second testcase

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: Holger's third testcase

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: patch

Please ignore comment 9, apparently the use of "screen" in getScreenCTM is just
unfortunate naming. getScreenCTM should return the transformation matrix from
current user units to the initial viewport coordinate system. In other words,
you should use clientX and clientY in conjunction with getScreenCTM to
transform cursor coordinates to user space, and not screenX and screenY as I
asserted. (This should be publicly clarified by the W3C at a later date.) I've
updated the bug summary and testcases.

This patch makes our implementation of getScreenCTM return a transform to the
same coordinate system as other implementations, and as the spec. was intended
to say it should.

Comment 23

[Jonathan Watt [:jwatt]]

Requesting blocking1.8b4. Sorry this is so late in the game, but we were waiting
on WG clarification. Fixing this is very important for compatibility between SVG
implementations. The change is SVG only and should be safe since GetScreenCTM is
only used recursively by itself in our code. In other words content is it's only
consumer. I'd like to get the patch in for b4 though to get it properly tested.

Comment 24

[Jonathan Watt [:jwatt]]

(In reply to comment #17)
> it seem getScreenCTM does not take currentTranslate/Scale into account. 

BTW, good catch Holger, and yes, this patch fixes that too.

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: patch - include currentScale and Translate for <svg><svg></svg></svg>

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 194961 [details] [diff] [review]
patch - include currentScale and Translate for <svg><svg></svg></svg>

requesting approval1.8b4. see comment 23 for risk.

Comment 27

[Jonathan Watt [:jwatt]]

checked in on trunk

Comment 28

[tor]

We should take this if possible for 1.8b4 - it fixes a specification compliance
problem that we've been discussing with the SVG WG.

Comment 29

[Holger Will]

verifying fix. works great, thanks Jonathan, this was just in time ;-)

Comment 30

[Jonathan Watt [:jwatt]]

checked in on branch