Closed
Bug 1055961
Opened 10 years ago
Closed 10 years ago
Touch.radiusX always returns zero
Categories
(Firefox OS Graveyard :: GonkIntegration, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S5 (26sep)
People
(Reporter: graziani, Assigned: jeffhwang)
Details
Attachments
(1 file, 1 obsolete file)
1.78 KB,
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•10 years ago
|
||
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.
Reporter | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
Sorry, I am out of the office for the next months. I've asked Jeff to update the patch.
Flags: needinfo?(faraojh)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
Comment on attachment 8494917 [details] [diff] [review]
touch_radius.patch
Perfect, thanks!
Attachment #8494917 -
Flags: review?(mwu) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Assignee: nobody → faraojh
Comment 8•10 years ago
|
||
Hi, can you provide a try run for this change, thanks!
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
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
Comment 11•10 years ago
|
||
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.
Description
•