Bug 32484 – [Qt] Add support for scale and transformation properties to TouchEvent

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

[Simon Hausmann]

The iPhone's TouchEvent has a scale and a transformation property. Arguably the properties do not belong into the TouchEvent and should be only in the gesture handling, but on the other hand they are convenient to have there, easy to implement and necessary to be able to access existing web-content using the touch interfaces.

[Igor Trindade Oliveira]

Someone is working on this bug?

[Igor Trindade Oliveira]

Created attachment 92783 [details]
Test case

Test case

[Igor Trindade Oliveira]

Created attachment 92787 [details]
Patch

proposed patch

[Alexis Menard (darktears)]

Comment on attachment 92787 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        [Qt] Add support for scale and rotation properties to TouchEvent. Scale property

Why double Qt here? You don't need to paste again the title of the bug.

> Source/WebCore/ChangeLog:9
> +        gives the multiplier the user has pinched in the gesture and rotation gives the

"Scale property gives the value of the multiplier used when the user pinched." Something like that.

[Alexis Menard (darktears)]

Could you look benjamin? I've heard some really really unsure rumors that you are working on Touch related stuff :D.

[Igor Trindade Oliveira]

Created attachment 92788 [details]
Patch

Proposed patch. update changelog.

[Igor Trindade Oliveira]

Created attachment 92867 [details]
patch

Proposed patch. add test.

[WebKit Review Bot]

Comment on attachment 92867 [details]
patch

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

New failing tests:
fast/events/touch/touch-scale-rotation.html

[WebKit Review Bot]

Created attachment 92899 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Igor Trindade Oliveira]

Created attachment 92906 [details]
patch

Proposed patch. skip test from chromium.

[Antonio Gomes]

Comment on attachment 92906 [details]
patch

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

> LayoutTests/ChangeLog:5
> +        [Qt] Add support for scale and transformation properties to TouchEvent

This change is not Qt specific is it?

[Igor Trindade Oliveira]

(In reply to comment #11)
> (From update of attachment 92906 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92906&action=review
> 
> > LayoutTests/ChangeLog:5
> > +        [Qt] Add support for scale and transformation properties to TouchEvent
> 
> This change is not Qt specific is it?

No, but the initial implementation is done in Qt side. I will update the patch Changelog.

[Igor Trindade Oliveira]

Created attachment 92986 [details]
patch

Proposed patch.

[Benjamin Poulain]

Comment on attachment 92986 [details]
patch

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

> LayoutTests/ChangeLog:15
> +        * fast/events/touch/touch-scale-rotation-expected.txt: Added.
> +        * fast/events/touch/touch-scale-rotation.html: Added.

This only tests the forward/valid case. You should also test the values for the cases:
-more than 2 fingers
-only one finger
-boundaries limits (like two very close fingers)

> Source/WebCore/platform/PlatformTouchEvent.h:67
> +        , m_scale(1)
> +        , m_rotationAngle(0)

You should use explicit floats for the literals, otherwise you might get a warning for the conversion int->float.

> Source/WebCore/platform/PlatformTouchEvent.h:102
> +    float m_scale;
> +    float m_rotationAngle;

Don't you need to also modify the WebKit 2 parts to serialize those?

> Source/WebCore/platform/qt/PlatformTouchEventQt.cpp:33
> +static qreal handleRotationTouchEvent(const QList<QTouchEvent::TouchPoint>& points)

I don't like handleRotationTouchEvent() and handleScaleTouchEvent() because they rely on startPos(). This only works if the framework above only accept one gesture at the time:
-pan
-lift your finger
-pinch
-lift your fingers
-etc.

In reality, most devs want to support the gesture following eachothers:
-pan
-keep fingers on screen
-pinch
-lift just one finger
-continue panning.

So the computation of handleRotationTouchEvent() and handleScaleTouchEvent() need to be done from the last gesture started.
Gesture recognizer generally also have threshold, which needs to be taken into account.

> Source/WebCore/platform/qt/PlatformTouchEventQt.cpp:57
> +            / QLineF(firstPoint.startPos(), lastPoint.startPos()).length();

Splitting this line like this is a sign you should use temporary variables to make this operation clearer :)

[Benjamin Poulain]

Robert, are you interested in supporting this feature in Chromium when it lands? If not, we should #ifdef the IDL, otherwise you would expose something that never returns meaningful values.

[Robert Kroeger]

(In reply to comment #15)
> Robert, are you interested in supporting this feature in Chromium when it lands? If not, we should #ifdef the IDL, otherwise you would expose something that never returns meaningful values.

Benjamin: thanks for the heads-up.

Please (at a minimum) #ifdef the IDL. It will be quite difficult to correctly modify the Chrome gesture recognizer to add the correct scale and transform to the Touch event.

Also: maybe these fields should have some sort of prefix given their exclusion from http://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html?

Aside: I feel that the "right" thing is what Simon said at the start: scale and transformation don't belong in the TouchEvent. Instead, we should have higher-level semantic gesture events yes?

[Benjamin Poulain]

(In reply to comment #16)
> Aside: I feel that the "right" thing is what Simon said at the start: scale and transformation don't belong in the TouchEvent. Instead, we should have higher-level semantic gesture events yes?

I don't really like those properties either. But I see how they make it easier to write JavaScript on mobile.

iOS is really the de facto standard there, with many apps designed just for it. I understand why people want this feature.

[Robert Kroeger]

(In reply to comment #17)
> (In reply to comment #16)
> > Aside: I feel that the "right" thing is what Simon said at the start: scale and transformation don't belong in the TouchEvent. Instead, we should have higher-level semantic gesture events yes?
> 
> I don't really like those properties either. But I see how they make it easier to write JavaScript on mobile.
> 
> iOS is really the de facto standard there, with many apps designed just for it. I understand why people want this feature.

I agree with your reasoning. The patch is worthwhile on this basis. I added https://bugs.webkit.org/show_bug.cgi?id=61550 to track a matching update for chrome.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.