Bug 44083 - SVGLocatable.getScreenCTM ignores scrolling
Summary: SVGLocatable.getScreenCTM ignores scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL: http://graphics.stanford.edu/~mbostoc...
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2010-08-16 18:07 PDT by Mike Bostock
Modified: 2014-06-27 09:08 PDT (History)
5 users (show)

See Also:


Attachments
Test case. (1.85 KB, text/html)
2010-08-16 18:07 PDT, Mike Bostock
no flags Details
Expected output of test case. (11.36 KB, image/png)
2010-08-16 18:08 PDT, Mike Bostock
no flags Details
Actual output of test case. (12.22 KB, image/png)
2010-08-16 18:09 PDT, Mike Bostock
no flags Details
Patch (119.38 KB, patch)
2010-08-27 00:55 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff
Test case for getScreenCTM and scrolling on iOS. (1.90 KB, text/html)
2014-06-27 09:06 PDT, Jason Davies
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Bostock 2010-08-16 18:07:33 PDT
Created attachment 64541 [details]
Test case.

Per SVG Tiny 1.2, Section A.8.11 SVGLocatable:

"""
getScreenCTM

Returns the transformation matrix from current user units to the initial viewport coordinate system. The clientX and clientY coordinates of a MouseEvent are in the initial viewport coordinate system. Note that null is returned if this element is not hooked into the document tree. This method would have been more aptly named as getClientCTM, but the name getScreenCTM is kept for historical reasons.
"""

In WebKit (and Safari, Chrome), the matrix returned by getScreenCTM considers the scrollTop and scrollLeft. Therefore, it is as if pageX and pageY were used instead of clientX and clientY, as described by the spec. Firefox and Opera exhibit the correct behavior, consistent with clientX and clientY.
Comment 1 Mike Bostock 2010-08-16 18:08:52 PDT
Created attachment 64542 [details]
Expected output of test case.
Comment 2 Mike Bostock 2010-08-16 18:09:15 PDT
Created attachment 64543 [details]
Actual output of test case.
Comment 3 Mike Bostock 2010-08-16 18:21:32 PDT
Oops, clarification to my initial description: getScreenCTM ignores scrollTop and scrollLeft in WebKit, which is why it is inconsistent with clientX and clientY.

For example, in Firefox and Opera, if you scroll down by 200 pixels, svg.getScreenCTM().f will go down by 200 pixels (e.g., 92 -> -108). On the other hand, in WebKit, Safari and Chrome the value of svg.getScreenCTM().f will stay constant independent of scrolling.
Comment 4 Eric Seidel (no email) 2010-08-24 21:39:15 PDT
http://trac.webkit.org/browser/trunk/WebCore/svg/SVGLocatable.cpp#L87

is the code.  Notice how it stops at the first non-SVG element.

I suspect that scrolling may be expressed in tx/ty anyway, I'm not sure.
Comment 5 Mike Bostock 2010-08-24 23:42:55 PDT
(In reply to comment #4)
> Notice how it stops at the first non-SVG element.

That's true for mode == NearestViewportScope (getCTM), but not mode == ScreenScope (getScreenCTM), where stopAtElement is 0 and it goes all the way to the root element.

I think what's needed is something like this at the end of computeCTM:

    if (mode == ScreenScope) {
        DOMWindow* window = element->ownerDocument()->defaultView();
        ctm = ctm.translate(window->scrollX(), window->scrollY());
    }

    return ctm;

Although I haven't thought through whether it should be translate or translateRight, yet. Or maybe -scrollX and -scrollY.
Comment 6 Mike Bostock 2010-08-24 23:44:07 PDT
(In reply to comment #4)
> Notice how it stops at the first non-SVG element.

Oops, right. I see the current->isSVGElement() now. Either way, that part looks fine. It's just not taking into account scrolling.
Comment 7 Eric Seidel (no email) 2010-08-24 23:55:54 PDT
Maybe the solution is as simple as asking frame()->view() for the scrollOffset().
Comment 8 Nikolas Zimmermann 2010-08-25 00:21:20 PDT
(In reply to comment #0)
> Created an attachment (id=64541) [details]
> Test case.
> 
> Per SVG Tiny 1.2, Section A.8.11 SVGLocatable:
> 
> """
> getScreenCTM
> 
> Returns the transformation matrix from current user units to the initial viewport coordinate system. The clientX and clientY coordinates of a MouseEvent are in the initial viewport coordinate system. Note that null is returned if this element is not hooked into the document tree. This method would have been more aptly named as getClientCTM, but the name getScreenCTM is kept for historical reasons.
> """

First of all, we focus on SVG 1.1 2nd Edition, not SVG Tiny 1.2.
I explicitely ignored the scroll offset when implementing these functions, because what SVG 1.1 says, is:
http://www.w3.org/TR/SVG/types.html#InterfaceSVGLocatable


Returns the transformation matrix from current user units (i.e., after application of the ‘transform’ attribute, if any) to the parent user agent's notice of a "pixel". For display devices, ideally this represents a physical screen pixel. For other devices or environments where physical pixel sizes are not known, then an algorithm similar to the CSS2 definition of a "pixel" can be used instead. Note that null is returned if this element is not hooked into the document tree. This method would have been more aptly named as getClientCTM, but the name getScreenCTM is kept for historical reasons.

I was not sure about scrolling, but it seems to me that the SVG Tiny 1.2 behaviour is more clear.
So, I'll have a look to fix it.
Comment 9 Simon Fraser (smfr) 2010-08-25 09:32:33 PDT
(In reply to comment #7)
> Maybe the solution is as simple as asking frame()->view() for the scrollOffset().

What about SVG nested inside multiple overflow:scroll divs?
Comment 10 Mike Bostock 2010-08-25 11:53:43 PDT
(In reply to comment #9)
> What about SVG nested inside multiple overflow:scroll divs?

That part's fine, as you can see from this test case:

http://graphics.stanford.edu/~mbostock/getScreenCTM-nested.html

The goal is for getScreenCTM to be consistent with the mouse event's clientX and clientY. The only discrepancy with the current implementation is scrolling on the parent frame or window; scrolled container elements appear to be handled correctly (and thus are consistent with pageX and pageY).
Comment 11 Nikolas Zimmermann 2010-08-27 00:55:57 PDT
Created attachment 65686 [details]
Patch

Indeed, all we need to do is respect the FrameViews scrollOffset.
Nested divs with overflow="scroll" already work properly, as the localToAbsolute() call already takes care of handling those translations.

Added three new tests covering scrollable SVG areas, and a large SVG area contained in (nested) divs with overflow="scroll", to rule out Simons concerncs.
Comment 12 Dirk Schulze 2010-08-27 01:05:29 PDT
Comment on attachment 65686 [details]
Patch

Great examples! r=me
Comment 13 Nikolas Zimmermann 2010-08-27 01:10:45 PDT
Landed in r66187. Mike, please retry, with a nightly newer or equa to r66187 :-)
Comment 14 Mike Bostock 2010-08-31 23:45:20 PDT
(In reply to comment #13)
> Landed in r66187. Mike, please retry, with a nightly newer or equa to r66187 :-)

Works great! Thanks for fixing this.
Comment 15 Jason Davies 2014-06-27 09:06:41 PDT
Created attachment 233985 [details]
Test case for getScreenCTM and scrolling on iOS.
Comment 16 Jason Davies 2014-06-27 09:08:08 PDT
Unfortunately this issue seems to be unresolved on Safari in iOS 7.1.1.  I’ve attached a modified version Mike’s test case that uses touchmove instead of mousemove.