Bug 100542 – [chromium] Transform mouse/gesture event coordinates to account for pinch-zoom in compositor.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[W. James MacLean]

[chromium] Transform mouse/gesture event coordinates to account for pinch-zoom in compositor.

[W. James MacLean]

Created attachment 170962 [details]
Patch

[W. James MacLean]

Patch for discussion. (Does not compile without changes to CC to expose the implTransform.)

This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please:

1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom").

2) I was surprised I couldn't just invert the implTransform without having to modify it first. Since the modification only applies to the (hidden) zoom translation, there wasn't any obvious way around it.

Tests to come.

[Dana Jansens]

Comment on attachment 170962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170962&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:667
> +    m.setM41(m.m41() / m.m11()); // Translation components seem to have had scale applied to them,

Does this mean the implTransform is being computed wrong?

> Source/WebKit/chromium/src/WebViewImpl.cpp:670
> +    return m.inverse().mapPoint(location);

You want projectPoint() when you're using the inverse transform instead of mapPoint()

[Dana Jansens]

(In reply to comment #2)
> Patch for discussion. (Does not compile without changes to CC to expose the implTransform.)
> 
> This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please:
> 
> 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom").

Personally, I'd like this more as we have things like MathUtil there, which no longer exist on this side of the webkit boundary.

> 2) I was surprised I couldn't just invert the implTransform without having to modify it first. Since the modification only applies to the (hidden) zoom translation, there wasn't any obvious way around it.
> 
> Tests to come.

[W. James MacLean]

(In reply to comment #3)
> (From update of attachment 170962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170962&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:667
> > +    m.setM41(m.m41() / m.m11()); // Translation components seem to have had scale applied to them,
> 
> Does this mean the implTransform is being computed wrong?

Maybe, or it may mean I'm missing something about what implTransform is supposed to do.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:670
> > +    return m.inverse().mapPoint(location);
> 
> You want projectPoint() when you're using the inverse transform instead of mapPoint()

I'll try that and see if it fixes the problem above.

[Tien-Ren Chen]

(In reply to comment #4)
> (In reply to comment #2)
> > Patch for discussion. (Does not compile without changes to CC to expose the implTransform.)
> > 
> > This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please:
> > 
> > 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom").
> 
> Personally, I'd like this more as we have things like MathUtil there, which no longer exist on this side of the webkit boundary.

I'd vote for the LayerTreeHost approach. Ideally the WebKit/WebCore should know nothing about the page scale. It feels weird to me that the embedder (either the compositor or RenderViewImpl) applies the scale when rendering, while input events are inverted-scaled in WebViewImpl.

[James Robinson]

(In reply to comment #6)
> I'd vote for the LayerTreeHost approach. Ideally the WebKit/WebCore should know nothing about the page scale. It feels weird to me that the embedder (either the compositor or RenderViewImpl) applies the scale when rendering, while input events are inverted-scaled in WebViewImpl.

+1 to this

[W. James MacLean]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Patch for discussion. (Does not compile without changes to CC to expose the implTransform.)
> > > 
> > > This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please:
> > > 
> > > 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom").
> > 
> > Personally, I'd like this more as we have things like MathUtil there, which no longer exist on this side of the webkit boundary.
> 
> I'd vote for the LayerTreeHost approach. Ideally the WebKit/WebCore should know nothing about the page scale. It feels weird to me that the embedder (either the compositor or RenderViewImpl) applies the scale when rendering, while input events are inverted-scaled in WebViewImpl.

Agreed!

[W. James MacLean]

Created attachment 171272 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dana Jansens]

Comment on attachment 171272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:2006
> +        mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent);

why all the * and & back and forth?

Why not just:
WebInputEvent inputEventTransformed = inputEvent;
mouseEvent = static_cast<WebMouseEvent>(inputEvent);
etc?

[Dana Jansens]

Comment on attachment 171272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review

> Source/Platform/chromium/public/WebLayerTreeView.h:36
> +#include "WebTransformationMatrix.h"

nit: unused header

[W. James MacLean]

(In reply to comment #11)
> (From update of attachment 171272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2006
> > +        mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent);
> 
> why all the * and & back and forth?
> 
> Why not just:
> WebInputEvent inputEventTransformed = inputEvent;
> mouseEvent = static_cast<WebMouseEvent>(inputEvent);
> etc?

Won't this slice off the extra fields (x, y, globalX, etc.) in the derived classes WebMouseEvent, WebGestureEvent ?

Will remove the extra header (oops!).

[Dana Jansens]

Comment on attachment 171272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:2006
>>> +        mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent);
>> 
>> why all the * and & back and forth?
>> 
>> Why not just:
>> WebInputEvent inputEventTransformed = inputEvent;
>> mouseEvent = static_cast<WebMouseEvent>(inputEvent);
>> etc?
> 
> Won't this slice off the extra fields (x, y, globalX, etc.) in the derived classes WebMouseEvent, WebGestureEvent ?
> 
> Will remove the extra header (oops!).

Ah I see, okay! I guess this is just awkward since it's being given, and passing on, a reference instead of a pointer.

[W. James MacLean]

(In reply to comment #14)
> (From update of attachment 171272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review
> 
> >>> Source/WebKit/chromium/src/WebViewImpl.cpp:2006
> >>> +        mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent);
> >> 
> >> why all the * and & back and forth?
> >> 
> >> Why not just:
> >> WebInputEvent inputEventTransformed = inputEvent;
> >> mouseEvent = static_cast<WebMouseEvent>(inputEvent);
> >> etc?
> > 
> > Won't this slice off the extra fields (x, y, globalX, etc.) in the derived classes WebMouseEvent, WebGestureEvent ?
> > 
> > Will remove the extra header (oops!).
> 
> Ah I see, okay! I guess this is just awkward since it's being given, and passing on, a reference instead of a pointer.

Yeah, the const-references can be a bit hard if you actually need to transform a value ...

[W. James MacLean]

Created attachment 171329 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171329 [details]
Patch

Attachment 171329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14641301

New failing tests:
fast/frames/iframe-window-focus.html
fast/frames/frame-programmatic-noresize.html
fast/frames/frame-deep-nested-resize.html
fast/frames/frame-with-noresize-can-be-resized-after-setting-noResize-to-false.html
fast/frames/frame-set-rotation-hit.html
fast/frames/frame-dead-region.html
fast/frames/frame-inherit-noresize-from-frameset.html
fast/frames/hover-timer-crash.html
fast/frames/frame-with-noresize-can-be-resized-after-removal-of-noresize.html
fast/frames/frames-with-frameborder-zero-can-be-resized.html
fast/frames/frame-set-scaling-hit.html

[W. James MacLean]

Created attachment 171438 [details]
Patch

[W. James MacLean]

This patch fixes null pointer dereference on m_layerTreeView.

[James Robinson]

Comment on attachment 171438 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 171438 [details]
Patch

Clearing flags on attachment: 171438

Committed r132946: <http://trac.webkit.org/changeset/132946>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.