Bug 32484

Summary: [Qt] Add support for scale and transformation properties to TouchEvent
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: New BugsAssignee: Igor Trindade Oliveira <igor.oliveira>
Status: RESOLVED INVALID    
Severity: Normal CC: benjamin, benm, dglazkov, diegohcg, igor.oliveira, jonni.rainisto, kent.hansen, laszlo.gombos, menard, rjkroege, webkit.review.bot
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 32482, 60487    
Bug Blocks: 32485, 61550    
Attachments:
Description Flags
Test case
none
Patch
none
Patch
none
patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
patch
none
patch benjamin: review-

Description Simon Hausmann 2009-12-13 02:40:18 PST
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.
Comment 1 Igor Trindade Oliveira 2011-05-06 07:39:11 PDT
Someone is working on this bug?
Comment 2 Igor Trindade Oliveira 2011-05-09 05:47:22 PDT
Created attachment 92783 [details]
Test case

Test case
Comment 3 Igor Trindade Oliveira 2011-05-09 06:32:30 PDT
Created attachment 92787 [details]
Patch

proposed patch
Comment 4 Alexis Menard (darktears) 2011-05-09 06:36:21 PDT
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.
Comment 5 Alexis Menard (darktears) 2011-05-09 06:37:14 PDT
Could you look benjamin? I've heard some really really unsure rumors that you are working on Touch related stuff :D.
Comment 6 Igor Trindade Oliveira 2011-05-09 06:43:00 PDT
Created attachment 92788 [details]
Patch

Proposed patch. update changelog.
Comment 7 Igor Trindade Oliveira 2011-05-09 15:41:42 PDT
Created attachment 92867 [details]
patch

Proposed patch. add test.
Comment 8 WebKit Review Bot 2011-05-09 18:14:12 PDT
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
Comment 9 WebKit Review Bot 2011-05-09 18:14:17 PDT
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
Comment 10 Igor Trindade Oliveira 2011-05-09 19:04:38 PDT
Created attachment 92906 [details]
patch

Proposed patch. skip test from chromium.
Comment 11 Antonio Gomes 2011-05-10 10:35:29 PDT
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?
Comment 12 Igor Trindade Oliveira 2011-05-10 11:06:56 PDT
(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.
Comment 13 Igor Trindade Oliveira 2011-05-10 11:23:04 PDT
Created attachment 92986 [details]
patch

Proposed patch.
Comment 14 Benjamin Poulain 2011-05-22 06:26:16 PDT
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 :)
Comment 15 Benjamin Poulain 2011-05-22 06:43:44 PDT
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.
Comment 16 Robert Kroeger 2011-05-25 14:02:46 PDT
(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?
Comment 17 Benjamin Poulain 2011-05-26 05:03:58 PDT
(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.
Comment 18 Robert Kroeger 2011-05-26 12:31:12 PDT
(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.
Comment 19 Jocelyn Turcotte 2014-02-03 03:16:01 PST
=== 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.