Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152463004:
Fix motion event orientation handling for styluses. (Closed)
|
Created:
5 years, 7 months ago by USE eero AT chromium.org Modified:
5 years, 6 months ago CC:
chromium-reviews, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix motion event orientation handling for styluses.
For a stylus, a motion event orientation range is full 360 degrees
(whereas for a touch screen or pad the range is only 180 degrees).
This change fixes handling of orientation angles which do not fall in
[-90, 90] range.
BUG=492239
Committed: https://crrev.com/100b5ba06a37d2e88c759679e0110b108efde2f9
Cr-Commit-Position: refs/heads/master@{#332010}
Patch Set 1 #
Total comments: 5
Patch Set 2 : GetOrientation comment #
Total comments: 5
Patch Set 3 : Be more restrictive #
Total comments: 12
Patch Set 4 : Comments #
Messages
Total messages: 35 (11 generated)
e.hakkinen@samsung.com changed reviewers: + rbyers@chromium.org, sadrul@chromium.org, sky@chromium.org
PTAL.
https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) Why do you need to do this and 113?
rbyers@chromium.org changed reviewers: + jdduke@chromium.org, mustaq@chromium.org - rbyers@chromium.org, sadrul@chromium.org
rbyers@chromium.org changed reviewers: + rbyers@chromium.org, sadrul@chromium.org - jdduke@chromium.org, mustaq@chromium.org
+mustaq who wrote this https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) On 2015/05/26 19:51:04, sky wrote: > Why do you need to do this and 113? Presumably because the TouchEvent extensions note says TouchEvent.rotationAngle is always between 0 and 90: https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html#widl-Touch.... This is because rotationAngle is designed to be about contact geometry, not tool rotation. Personally I think it was a mistake for Android to re-use the same API for these conceptually different scenarios. Fixing this up so we conform to the spec LGTM, thanks! https://codereview.chromium.org/1152463004/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/motion_event.h (right): https://codereview.chromium.org/1152463004/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/motion_event.h:95: // Returns the orientation of the major axis clockwise from vertical, in For stylus this isn't really returning "the orientation of the major axis", right? The Android definition is here: http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI.... I think we should have something similar in our comment - making it clear this 'orientation' is overloaded to mean two very different things depending on the device type.
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) On 2015/05/26 20:21:33, Rick Byers wrote: > On 2015/05/26 19:51:04, sky wrote: > > Why do you need to do this and 113? > > Presumably because the TouchEvent extensions note says TouchEvent.rotationAngle > is always between 0 and 90: > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html#widl-Touch.... > This is because rotationAngle is designed to be about contact geometry, not > tool rotation. Personally I think it was a mistake for Android to re-use the > same API for these conceptually different scenarios. > > Fixing this up so we conform to the spec LGTM, thanks! I agree with Rick that using "orientation" to represent two different concepts in Android MotionEvent spec was a mistake. And I think we should not carry over the same mistake here, for two reasons: - TouchEvent rotationAngle stirctly represents touch geometry, so using this field to represent stylus orientation is like expanding the scope of the Android mistake. - For stylus, forcing the orientation to within +-90 degrees is wrong. For example, +135 degrees means the stylus is pointing downward-right, and fixing it to +135-180 = -45 degrees means it is now pointing upward-left.
On 2015/05/26 21:25:59, mustaq wrote: > https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... > File ui/events/blink/blink_event_util.cc (right): > > https://codereview.chromium.org/1152463004/diff/1/ui/events/blink/blink_event... > ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) > On 2015/05/26 20:21:33, Rick Byers wrote: > > On 2015/05/26 19:51:04, sky wrote: > > > Why do you need to do this and 113? > > > > Presumably because the TouchEvent extensions note says > TouchEvent.rotationAngle > > is always between 0 and 90: > > > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html#widl-Touch.... > > This is because rotationAngle is designed to be about contact geometry, not > > tool rotation. Personally I think it was a mistake for Android to re-use the > > same API for these conceptually different scenarios. > > > > Fixing this up so we conform to the spec LGTM, thanks! > > I agree with Rick that using "orientation" to represent two different concepts > in Android MotionEvent spec was a mistake. And I think we should not carry over > the same mistake here, for two reasons: > > - TouchEvent rotationAngle stirctly represents touch geometry, so using this > field to represent stylus orientation is like expanding the scope of the Android > mistake. > > - For stylus, forcing the orientation to within +-90 degrees is wrong. For > example, +135 degrees means the stylus is pointing downward-right, and fixing it > to +135-180 = -45 degrees means it is now pointing upward-left. Since we currently map stylus devices to touch events on Android, what should we put into the rotation field? I'm OK doing our best to fill in TouchEvent details with stylus properties (as long as we're within the values required by the TouchEvent spec).
On 2015/05/26 23:31:13, Rick Byers wrote: > Since we currently map stylus devices to touch events on Android, what should we > put into the rotation field? I'm OK doing our best to fill in TouchEvent > details with stylus properties (as long as we're within the values required by > the TouchEvent spec). Would it make more sense to break the overloading in AndroidMotionEvent, like in https://codereview.chromium.org/1147083005? https://codereview.chromium.org/1152463004/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/motion_event.h (right): https://codereview.chromium.org/1152463004/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/motion_event.h:95: // Returns the orientation of the major axis clockwise from vertical, in On 2015/05/26 20:21:33, Rick Byers wrote: > For stylus this isn't really returning "the orientation of the major axis", > right? The Android definition is here: > http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI.... > I think we should have something similar in our comment - making it clear this > 'orientation' is overloaded to mean two very different things depending on the > device type. Done.
On 2015/05/27 08:39:39, e_hakkinen wrote: > On 2015/05/26 23:31:13, Rick Byers wrote: > > Since we currently map stylus devices to touch events on Android, what should > we > > put into the rotation field? I'm OK doing our best to fill in TouchEvent > > details with stylus properties (as long as we're within the values required by > > the TouchEvent spec). > > Would it make more sense to break the overloading in AndroidMotionEvent, like in > https://codereview.chromium.org/1147083005 > > https://codereview.chromium.org/1152463004/diff/1/ui/events/gesture_detection... > File ui/events/gesture_detection/motion_event.h (right): > > https://codereview.chromium.org/1152463004/diff/1/ui/events/gesture_detection... > ui/events/gesture_detection/motion_event.h:95: // Returns the orientation of the > major axis clockwise from vertical, in > On 2015/05/26 20:21:33, Rick Byers wrote: > > For stylus this isn't really returning "the orientation of the major axis", > > right? The Android definition is here: > > > http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI.... > > I think we should have something similar in our comment - making it clear > this > > 'orientation' is overloaded to mean two very different things depending on the > > device type. > > Done. I prefer breaking the overloading as in your other CL. I'll comment there in details.
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:109: DCHECK_GT(orientation_deg, -180.01f); I spoke offline with mustaq/tdresser. We decided it might be simplest for us to proceed with this solution, however, we should restrict the validation to match the Android spec. So this will look like: if (event.GetToolType(pointer_index) == MotionEvent::TOOL_TYPE_STYLUS) { *insert code you have now* } else { * insert old [90, 90] validation code * } https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) Let's make this a strict inequality (>, <).
https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:109: DCHECK_GT(orientation_deg, -180.01f); On 2015/05/28 15:01:45, jdduke(OOO_until_June_1) wrote: > I spoke offline with mustaq/tdresser. We decided it might be simplest for us to > proceed with this solution, however, we should restrict the validation to match > the Android spec. So this will look like: > > if (event.GetToolType(pointer_index) == MotionEvent::TOOL_TYPE_STYLUS) { > *insert code you have now* > } else { > * insert old [90, 90] validation code * > } Done. https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) On 2015/05/28 15:01:45, jdduke(OOO_until_June_1) wrote: > Let's make this a strict inequality (>, <). Actually, this one should not be a strict inequality. According to the TouchEvent spec, rotationAngle must be less than (and not equal to) 90. https://codereview.chromium.org/1152463004/diff/20001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:113: if (orientation_deg <= -90.f) But this should be a strict inequality. Done.
Thanks, just one question. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:106: if (event.GetToolType(pointer_index) == MotionEvent::TOOL_TYPE_STYLUS) { I guess at some point we should add some basic test coverage to these conversion routines, but I won't ask you to add those. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) Where does the spec say it's strictly less than 90 degrees? Can't seem to find it. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:120: if (orientation_deg >= 90.f) Nit: If we DCHECK, we shouldn't actually fix it if it's wrong, so I would remove this >= adjustment branch.
tdresser@chromium.org changed reviewers: + tdresser@chromium.org
Driveby nits... https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:107: // Orientation lies in [-180, 180] for a stylus. Use () for open ranges. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:116: // Orientation lies in [-90, 90] for a stylus. stylus -> touch.
https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:110: DCHECK_LT(orientation_deg, 180.01f); Please create a bug saying that "When Touch.rotationAngle represents a stylus orientation, because we are forced to limit the angle to [0,90) as per the TE spec, the angle is exact in one quadrant, off by 90 degree in two quadrants, and off by 180 degrees in one quadrant". Also add a TODO here pointing to the bug.
https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:107: // Orientation lies in [-180, 180] for a stylus. On 2015/05/29 15:01:21, tdresser wrote: > Use () for open ranges. It is not an open but a closed range. From http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI... > The range is from -PI radians to PI radians, where 0 is pointing up, -PI/2 radians is pointing left, -PI or PI radians is pointing down, and PI/2 radians is pointing right. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:110: DCHECK_LT(orientation_deg, 180.01f); On 2015/05/29 15:11:51, mustaq wrote: > Please create a bug saying that "When Touch.rotationAngle represents a stylus > orientation, because we are forced to limit the angle to [0,90) as per the TE > spec, the angle is exact in one quadrant, off by 90 degree in two quadrants, and > off by 180 degrees in one quadrant". Also add a TODO here pointing to the bug. Done. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:111: if (orientation_deg >= 90.f) On 2015/05/29 14:36:14, jdduke(OOO_until_June_1) wrote: > Where does the spec say it's strictly less than 90 degrees? Can't seem to find > it. In section 2.1 the spec says that than rotationAngle must be greater than or equal to (i.e. >=) 0 and less than (i.e. <) 90. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:116: // Orientation lies in [-90, 90] for a stylus. On 2015/05/29 15:01:21, tdresser wrote: > stylus -> touch. Done. https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:120: if (orientation_deg >= 90.f) On 2015/05/29 14:36:14, jdduke(OOO_until_June_1) wrote: > Nit: If we DCHECK, we shouldn't actually fix it if it's wrong, so I would remove > this >= adjustment branch. There is nothing wrong for orientation_deg to be equal to 90. We also allow that in DCHECKs. We also allow a small tolerance, thus there is nothing wrong for orientation_deg to be in [90, 90.01). Here I normalise orientation_deg from a closed range [-90, 90] to a half-open range [-90, 90). That is because the set {-90, 90} forms an equivalence class thus these values should be handled in the same way. The easiest way to do that (IMO) is to normalise these values to -90. Another way would be add a third branch to the if block on the line 123.
https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:107: // Orientation lies in [-180, 180] for a stylus. On 2015/05/29 16:17:47, e_hakkinen wrote: > On 2015/05/29 15:01:21, tdresser wrote: > > Use () for open ranges. > > It is not an open but a closed range. > > From > http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI... > > The range is from -PI radians to PI radians, where 0 is pointing up, -PI/2 > radians is pointing left, -PI or PI radians is pointing down, and PI/2 radians > is pointing right. Sorry, you're right.
On 2015/05/29 16:45:45, tdresser wrote: > https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... > File ui/events/blink/blink_event_util.cc (right): > > https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... > ui/events/blink/blink_event_util.cc:107: // Orientation lies in [-180, 180] for > a stylus. > On 2015/05/29 16:17:47, e_hakkinen wrote: > > On 2015/05/29 15:01:21, tdresser wrote: > > > Use () for open ranges. > > > > It is not an open but a closed range. > > > > From > > > http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI... > > > The range is from -PI radians to PI radians, where 0 is pointing up, -PI/2 > > radians is pointing left, -PI or PI radians is pointing down, and PI/2 radians > > is pointing right. > > Sorry, you're right. Lgtm when others are happy.
On 2015/05/29 16:55:58, jdduke(OOO_until_June_1) wrote: > On 2015/05/29 16:45:45, tdresser wrote: > > > https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... > > File ui/events/blink/blink_event_util.cc (right): > > > > > https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... > > ui/events/blink/blink_event_util.cc:107: // Orientation lies in [-180, 180] > for > > a stylus. > > On 2015/05/29 16:17:47, e_hakkinen wrote: > > > On 2015/05/29 15:01:21, tdresser wrote: > > > > Use () for open ranges. > > > > > > It is not an open but a closed range. > > > > > > From > > > > > > http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI... > > > > The range is from -PI radians to PI radians, where 0 is pointing up, -PI/2 > > > radians is pointing left, -PI or PI radians is pointing down, and PI/2 > radians > > > is pointing right. > > > > Sorry, you're right. > > Lgtm when others are happy. LGTM
On 2015/05/29 16:55:58, jdduke(OOO_until_June_1) wrote: > On 2015/05/29 16:45:45, tdresser wrote: > > > https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... > > File ui/events/blink/blink_event_util.cc (right): > > > > > https://codereview.chromium.org/1152463004/diff/40001/ui/events/blink/blink_e... > > ui/events/blink/blink_event_util.cc:107: // Orientation lies in [-180, 180] > for > > a stylus. > > On 2015/05/29 16:17:47, e_hakkinen wrote: > > > On 2015/05/29 15:01:21, tdresser wrote: > > > > Use () for open ranges. > > > > > > It is not an open but a closed range. > > > > > > From > > > > > > http://developer.android.com/reference/android/view/MotionEvent.html#AXIS_ORI... > > > > The range is from -PI radians to PI radians, where 0 is pointing up, -PI/2 > > > radians is pointing left, -PI or PI radians is pointing down, and PI/2 > radians > > > is pointing right. > > > > Sorry, you're right. > > Lgtm when others are happy. lgtm.
The CQ bit was checked by e.hakkinen@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1152463004/#ps60001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152463004/60001
The CQ bit was unchecked by e.hakkinen@samsung.com
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Oh, OWNERs haven't reviewed yet.
lgtm
The CQ bit was checked by e.hakkinen@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152463004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/100b5ba06a37d2e88c759679e0110b108efde2f9 Cr-Commit-Position: refs/heads/master@{#332010} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
