Bug 59030

Summary: Make WebKit expose extra touch information
Product: WebKit Reporter: miletus
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: anicolao, ap, commit-queue, eric, fishd, miletus, mjs, rjkroege
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description miletus 2011-04-20 15:57:25 PDT
Some touch devices provide, other than (x,y) location, extra touch information, like the major/minor axis length, and  
the orientation of the touch ellipse. Attached is a proposed patch that routes these extra touch information through
chromium port to WebKit and exposes them to DOM.

WebCore::Touch is augmented with three fields :
m_radius ,  the major axis length of the touch ellipse
m_ratio  ,  the ratio between major and minor axis length of the touch ellipse
m_angle  ,  the orientation of the touch ellipse


The extra touch information is passed in from chromium's representation of touch event
WebKit::WebTouchPoint and then routed to platform touch event WebKit::PlatformTouchEventBuilder

Touch.idl and Document.idl are modified to expose these extra fields to DOM.
Comment 1 miletus 2011-04-20 16:07:36 PDT
Created attachment 90431 [details]
Patch
Comment 2 miletus 2011-04-20 16:16:25 PDT
update cc list
Comment 3 Darin Fisher (:fishd, Google) 2011-04-20 16:33:57 PDT
Comment on attachment 90431 [details]
Patch

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

> Source/WebCore/dom/Touch.idl:39
> +        readonly attribute float            radius;

what specification covers these additions?  if none, then shouldn't
we vendor prefix these to avoid possible collisions in the future
with a formal specification that may define these fields differently?

i confess that i don't know much about the history of this file.
is there already a spec covering the existing fields?
Comment 4 Alexey Proskuryakov 2011-04-20 22:50:22 PDT
If there is no specification, then adding this code to WebKit even under an #if and even with a vendor prefix shouldn't be taken lightly.

Also, this patch has style errors that the bot didn't find.
Comment 5 miletus 2011-04-21 11:53:35 PDT
Created attachment 90568 [details]
Patch
Comment 6 miletus 2011-04-21 12:12:20 PDT
(In reply to comment #3)
> (From update of attachment 90431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90431&action=review
> 
> > Source/WebCore/dom/Touch.idl:39
> > +        readonly attribute float            radius;
> 
> what specification covers these additions?  if none, then shouldn't
> we vendor prefix these to avoid possible collisions in the future
> with a formal specification that may define these fields differently?
> 
> i confess that i don't know much about the history of this file.
> is there already a spec covering the existing fields?

W3C has a Draft of the "Touch Events Specification" at

http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html

It is largely compatible with the touch event interface used in the Safari
DOM model (and the API exposed by WebKit). 

Also the draft is going to add some extra properties for touch point,
including radiusX, radiusY (the X and Y axis of the touch ellipse) and
roatationAngle, which is covered by this patch.

Even Safari/WebKit is not going to follow the draft spec (which is true
currently since the Safari/WebKit DOM model is already there while the 
spec is still being drafted, and also note that Mozilla exposes different
set of API for the touch events), it is still desirable to have the features
to expose these touch information.
Comment 7 miletus 2011-04-21 12:13:26 PDT
(In reply to comment #4)
> If there is no specification, then adding this code to WebKit even under an #if and even with a vendor prefix shouldn't be taken lightly.
>

Please see the comment #6.

 
> Also, this patch has style errors that the bot didn't find.

Would you mind pointing me where it is ?
Comment 8 miletus 2011-04-21 12:15:50 PDT
(In reply to comment #5)
> Created an attachment (id=90568) [details]
> Patch


The extra touch information are changed to comply with the draft spec  

http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html

Now the WebCore::Touch is augmented with 3 fields :

m_radiusX
m_radiusY
m_rotationAngle
Comment 9 Alexey Proskuryakov 2011-04-21 12:33:32 PDT
Comment on attachment 90568 [details]
Patch

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

I was also going to complain about text alignment (we normally use one space), but those are all pre-exisintg issues in this code.

I don't object to landing this, given the kind of support there already is for touch events.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

The patch can't be landed with this line. You should add tests.

> Source/WebKit/chromium/public/WebTouchPoint.h:52
> +        , rotationAngle(0.0) { }

Just "0", and the braces should go on separate lines.
Comment 10 miletus 2011-04-26 13:02:19 PDT
Created attachment 91147 [details]
Patch
Comment 11 miletus 2011-04-26 13:06:02 PDT
Comment on attachment 90568 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> The patch can't be landed with this line. You should add tests.

done.

>> Source/WebKit/chromium/public/WebTouchPoint.h:52
>> +        , rotationAngle(0.0) { }
> 
> Just "0", and the braces should go on separate lines.

done
Comment 12 miletus 2011-04-26 13:06:26 PDT
tests added.

(In reply to comment #10)
> Created an attachment (id=91147) [details]
> Patch
Comment 13 miletus 2011-05-04 10:39:08 PDT
Created attachment 92277 [details]
Patch
Comment 14 miletus 2011-05-04 10:48:14 PDT
(In reply to comment #13)
> Created an attachment (id=92277) [details]
> Patch

prefix the three fields of WebCore::Touch

radiusX
radiusY
rotationAngle

to be 

webkitRadiusX
webkitRadiusY
webkitRotationAngle

for now and will change to the standard name when the draft 

http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html#idl-def-Touch

is approved.
Comment 15 Darin Fisher (:fishd, Google) 2011-05-04 22:18:14 PDT
Well, hmm...

It looks like that spec also covers the other pre-existing fields of Touch.  It looks like the spec is co-edited by folks from Mozilla and Opera.  Do you know the state of their implementations?  Have they vendor prefixed these fields?  Have they been shipping these fields for some time already?

Comment #6 makes me concerned that there may be considerable divergence between the browsers.
Comment 16 miletus 2011-05-05 11:43:31 PDT
(In reply to comment #15)
> Well, hmm...
> 
> It looks like that spec also covers the other pre-existing fields of Touch.  It looks like the spec is co-edited by folks from Mozilla and Opera.  Do you know the state of their implementations?  Have they vendor prefixed these fields?  Have they been shipping these fields for some time already?
> 
> Comment #6 makes me concerned that there may be considerable divergence between the browsers.


According to this thread

http://lists.w3.org/Archives/Public/public-webevents/2011AprJun/0083.html

Matt Brubeck from Mozilla, also one of the editors of the draft, has an
experimental implementation in the nightly build of mobile Firefox.

Also Mozilla has vendor prefixed MozTouchEvent and related APIs which are
not compatible with WebKit.

Opera's implementation is based on iphone (and so the same as WebKit)

The draft is based on WebKit's implementation but some divergence is 
developing.

As for the radius and angle information, since they are now available
from many touch devices, it is good to expose them to web developers.
And as the draft is still evolving, vendor-prefixing the fields seems the
safe way to go for now.
Comment 17 miletus 2011-05-17 11:00:38 PDT
Hi Darin,

more thoughts on this patch ?
Comment 18 Darin Fisher (:fishd, Google) 2011-05-17 11:42:31 PDT
Comment on attachment 92277 [details]
Patch

Agreed.  Sorry for the high latency here.  I was out on vacation.
Comment 19 WebKit Commit Bot 2011-05-17 13:05:09 PDT
Comment on attachment 92277 [details]
Patch

Clearing flags on attachment: 92277

Committed r86700: <http://trac.webkit.org/changeset/86700>
Comment 20 WebKit Commit Bot 2011-05-17 13:05:15 PDT
All reviewed patches have been landed.  Closing bug.