Closed Bug 1055961 Opened 10 years ago Closed 10 years ago

Touch.radiusX always returns zero

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S5 (26sep)

People

(Reporter: graziani, Assigned: jeffhwang)

Details

Attachments

(1 file, 1 obsolete file)

According to spec definition: Touch.radiusX returns the X radius of the ellipse that most closely circumscribes the area of contact with the screen. The value is in pixels of the same scale as screenX. However a simple test in a mobile device running Firefox OS shows that radiusX, radiusY and rotationAngle are always zero. Tested in Nexus4 running FFOS. Test page: http://graziani.cafe24.com/test/radius That works fine in Firefox for Android. Ref.: https://developer.mozilla.org/en-US/docs/Web/API/Touch.radiusX
At nsAppShell.cpp, addDOMTouch(), we are creating Touch object and passing the AMOTION_EVENT_AXIS_SIZE as radiusX and radiusY, and fixing orientationAngle as 0. new dom::Touch(touch.id, nsIntPoint(floor(touch.coords.getX() + 0.5), floor(touch.coords.getY() + 0.5)), nsIntPoint(touch.coords.getAxisValue(AMOTION_EVENT_AXIS_SIZE), touch.coords.getAxisValue(AMOTION_EVENT_AXIS_SIZE)), 0, touch.coords.getAxisValue(AMOTION_EVENT_AXIS_PRESSURE)) However according to https://source.android.com/devices/tech/input/touch-devices.html the "size" property is a normalized value, varying from 0 to 1.0, and once we round it to integer we are getting 0 for the radius.
This patch uses the touchMajor and touchMinor as radiusX or radiusY (according to the orientation). It also converts the orientation (rad) into rotationAngle (deg). I tested it in FFOS in Nexus4, and the result became the same as Android Firefox Browser in Nexus 4.
Attachment #8475735 - Flags: review?(mwu)
Thanks for the patch! FYI, this code is getting upended and rewritten to some degree in bug 970751. I asked the assignee on the bug to not implement radiusX/radiusY/rotationAngle since you have an implementation here. Once that lands, please create a new patch and I'll review it.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8475735 [details] [diff] [review] Send touchMajor and touchMinor as radius Review of attachment 8475735 [details] [diff] [review]: ----------------------------------------------------------------- Please rebase this on top of the new touch event code that landed the weekend. ::: widget/gonk/nsAppShell.cpp @@ +221,5 @@ > const ::Touch& touch = data.motion.touches[i]; > + float radiusX; > + float radiusY; > + float rotationAngle; > + float orientation; I think you should just declare variables when they're used, rather than putting them at the top. @@ +225,5 @@ > + float orientation; > + > + orientation = touch.coords.getAxisValue(AMOTION_EVENT_AXIS_ORIENTATION); > + > + if(orientation >= 0 && orientation <= M_PI_2){ According to mobile/android/base/GeckoEvent.java, orientation == M_PI_2 needs to be converted to 0. Also, add a space after if and before the '{'. @@ +226,5 @@ > + > + orientation = touch.coords.getAxisValue(AMOTION_EVENT_AXIS_ORIENTATION); > + > + if(orientation >= 0 && orientation <= M_PI_2){ > + radiusX = touch.coords.getAxisValue(AMOTION_EVENT_AXIS_TOUCH_MAJOR)/2; Add a space before and after '/' here and elsewhere. @@ +232,5 @@ > + rotationAngle = orientation * 180 / M_PI; > + } else { > + radiusX = touch.coords.getAxisValue(AMOTION_EVENT_AXIS_TOUCH_MINOR)/2; > + radiusY = touch.coords.getAxisValue(AMOTION_EVENT_AXIS_TOUCH_MAJOR)/2; > + rotationAngle = -1 * orientation * 180 / M_PI; On Android Firefox, we add 90 degrees if the angle is negative. That's a different thing than flipping the sign here.
Attachment #8475735 - Flags: review?(mwu)
Sorry, I am out of the office for the next months. I've asked Jeff to update the patch.
Flags: needinfo?(faraojh)
Sorry for the late response here. I have attached a new patch resolved the nit and comment. Asking Mike to review again.
Attachment #8475735 - Attachment is obsolete: true
Attachment #8494917 - Flags: review?(mwu)
Flags: needinfo?(faraojh)
Comment on attachment 8494917 [details] [diff] [review] touch_radius.patch Perfect, thanks!
Attachment #8494917 - Flags: review?(mwu) → review+
Keywords: checkin-needed
Assignee: nobody → faraojh
Hi, can you provide a try run for this change, thanks!
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35fc2502cb8 Friendly reminder that your commit message should be summarizing what the patch is doing, not restating the problem it's fixing :) https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: