Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2269483002:
Set the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura (Closed)
|
Created:
4 years, 4 months ago by lanwei Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura.
In ConvertLocationToTarget function, the offset added to the event location is not scaled,
so we should apply the screen scale factor to the event location after it has been transformed to
the target. Then we can find the right target to send the event using the correct coordinates.
BUG=634343
Committed: https://crrev.com/34bff62e925142091ce98722c7bd447f51b3794b
Cr-Commit-Position: refs/heads/master@{#415119}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add test #
Total comments: 20
Patch Set 3 #
Total comments: 3
Patch Set 4 : Scale the position later #
Messages
Total messages: 67 (52 generated)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== tap win tap win tap win ========== to ========== Avoid transform the synthetic touch event again in SyntheticGestureTargetAura. BUG=634343 ==========
Description was changed from ========== Avoid transform the synthetic touch event again in SyntheticGestureTargetAura. BUG=634343 ========== to ========== Avoid transform the synthetic touch event again in SyntheticGestureTargetAura. The synthetic touch event is already transformed, if we transform it again based on the device scale factor, then we would not find the right target to send the event. BUG=634343 ==========
lanwei@chromium.org changed reviewers: + tdresser@chromium.org
Description was changed from ========== Avoid transform the synthetic touch event again in SyntheticGestureTargetAura. The synthetic touch event is already transformed, if we transform it again based on the device scale factor, then we would not find the right target to send the event. BUG=634343 ========== to ========== Avoid transform the synthetic touch event again in SyntheticGestureTargetAura. The synthetic touch event is already transformed, if we transform it again based on the device scale factor, then we would not find the right target to send the event. BUG=634343 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we write a test for this? https://codereview.chromium.org/2269483002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:45: touch_with_latency, &events, LOCAL_COORDINATES); Should we just be constructing these in a different coordinate space?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2269483002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:45: touch_with_latency, &events, LOCAL_COORDINATES); On 2016/08/22 12:52:39, tdresser wrote: > Should we just be constructing these in a different coordinate space? Done.
tdresser@chromium.org changed reviewers: + bokan@chromium.org
This LGTM, but I'd appreciate a second pair of eyes confirming that this approach to testing is reasonable. +bokan https://codereview.chromium.org/2269483002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:45: touch_with_latency, &events, LOCAL_COORDINATES); On 2016/08/24 11:56:06, lanwei wrote: > On 2016/08/22 12:52:39, tdresser wrote: > > Should we just be constructing these in a different coordinate space? > > Done. Are you sure SCREEN_COORDINATES is correct? I was mistakenly thinking we could simplify this a bunch by setting the coordinate space correctly, but it doesn't look like that's the case. https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:41: touch_with_latency.event.touches[i].radiusY *= device_scale_factor_;; Remove extra ; https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:51: // Synthetic touch events will be transformed here before send to before being sent to the EventProcessor https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:52: // EventProcessor, so it should not be transformed again later in so they should not be transformed again later https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html (right): https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:31: //function tap() { Remove commented out line (and below). https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:38: testTap.done();}); I think we'd normally put }); on the line below.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nit in description: Avoid transform the synthetic... -> Avoid transforming the synthetic... https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:45: touch_with_latency, &events, SCREEN_COORDINATES); Why is this change needed? The call below to ConvertLocationToTarget seems to imply we're going from one set of window coordinates to another, rather than any screen coordinates. It happens that on Android screen coordinates are window coordinates (perhaps off by the system toolbar) but I wonder if this breaks on desktops. https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:856: crbug.com/613672 [ Mac ] virtual/scalefactor200/fast/events/synthetic-events/tap-on-scaled-screen.html [ Skip ] Why is Mac special for this test? https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html (right): https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> please use <!DOCTYPE html> https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:27: assert_equals(event.screenX, x); This will only work if the window is at the top left corner of the screen. It'd be better to compare to |x+window.screenX| and |y+window.screenY| to make it more general.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
Description was changed from ========== Avoid transform the synthetic touch event again in SyntheticGestureTargetAura. The synthetic touch event is already transformed, if we transform it again based on the device scale factor, then we would not find the right target to send the event. BUG=634343 ========== to ========== Avoid transforming the synthetic touch event again in SyntheticGestureTargetAura. The synthetic touch event is already transformed, if we transform it again based on the device scale factor, then we would not find the right target to send the event. BUG=634343 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:41: touch_with_latency.event.touches[i].radiusY *= device_scale_factor_;; On 2016/08/24 12:27:48, tdresser wrote: > Remove extra ; Done. https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:45: touch_with_latency, &events, SCREEN_COORDINATES); On 2016/08/24 15:56:57, bokan wrote: > Why is this change needed? The call below to ConvertLocationToTarget seems to > imply we're going from one set of window coordinates to another, rather than any > screen coordinates. It happens that on Android screen coordinates are window > coordinates (perhaps off by the system toolbar) but I wonder if this breaks on > desktops. Actually for SyntheticTouchEvents, the screen position and position are set to the same. like point.position.x = point.screenPosition.x = x;, so I guess it would not make much difference. https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:51: // Synthetic touch events will be transformed here before send to On 2016/08/24 12:27:48, tdresser wrote: > before being sent to the EventProcessor Done. https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:52: // EventProcessor, so it should not be transformed again later in On 2016/08/24 12:27:49, tdresser wrote: > so they should not be transformed again later Done. https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:856: crbug.com/613672 [ Mac ] virtual/scalefactor200/fast/events/synthetic-events/tap-on-scaled-screen.html [ Skip ] On 2016/08/24 15:56:57, bokan wrote: > Why is Mac special for this test? Because Mac does not support touch events, gpuBenchmarking.tap generates touch events, and they are not sent to Mac. https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html (right): https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2016/08/24 15:56:57, bokan wrote: > please use <!DOCTYPE html> Done. https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:27: assert_equals(event.screenX, x); On 2016/08/24 15:56:57, bokan wrote: > This will only work if the window is at the top left corner of the screen. It'd > be better to compare to |x+window.screenX| and |y+window.screenY| to make it > more general. Acknowledged. https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:31: //function tap() { On 2016/08/24 12:27:49, tdresser wrote: > Remove commented out line (and below). Done. https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/synthetic-events/tap-on-scaled-screen.html:38: testTap.done();}); On 2016/08/24 12:27:49, tdresser wrote: > I think we'd normally put }); on the line below. Done.
https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (right): https://codereview.chromium.org/2269483002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:45: touch_with_latency, &events, SCREEN_COORDINATES); On 2016/08/25 02:28:55, lanwei wrote: > On 2016/08/24 15:56:57, bokan wrote: > > Why is this change needed? The call below to ConvertLocationToTarget seems to > > imply we're going from one set of window coordinates to another, rather than > any > > screen coordinates. It happens that on Android screen coordinates are window > > coordinates (perhaps off by the system toolbar) but I wonder if this breaks on > > desktops. > > Actually for SyntheticTouchEvents, the screen position and position are set to > the same. like point.position.x = point.screenPosition.x = x;, so I guess it > would not make much difference. Ugh, it looks like the synthetic gestures don't really set screen position properly. That seems problematic to me but that's not a fault in this patch. Anyway, I'd leave this as LOCAL_COORDINATES (as you currently have) since the screen coordinates are sometimes set to 0. (e.g. https://cs.chromium.org/chromium/src/content/common/input/synthetic_web_input...) https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2269483002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:856: crbug.com/613672 [ Mac ] virtual/scalefactor200/fast/events/synthetic-events/tap-on-scaled-screen.html [ Skip ] On 2016/08/25 02:28:55, lanwei wrote: > On 2016/08/24 15:56:57, bokan wrote: > > Why is Mac special for this test? > > Because Mac does not support touch events, gpuBenchmarking.tap generates touch > events, and they are not sent to Mac. Ah, sorry, the comment above the block was cut off, it's clearer in context. https://codereview.chromium.org/2269483002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (left): https://codereview.chromium.org/2269483002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:38: touch_with_latency.event.touches[i].position.y *= device_scale_factor_; Why did this get removed in the latest patch?
https://codereview.chromium.org/2269483002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (left): https://codereview.chromium.org/2269483002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:38: touch_with_latency.event.touches[i].position.y *= device_scale_factor_; On 2016/08/25 12:36:51, bokan wrote: > Why did this get removed in the latest patch? If we use local coordinates, then we will use event.touches[i].position as the position in the ui::TouchEvent, when we create ui::TouchEvent from WebTouchEvent in MakeUITouchEventsFromWebTouchEvents. Then we do not need to apply the device_scale_factor_, because later we do not converted back as I set transform event to false, host->dispatcher()->set_transform_events(false). I think here the order is wrong which makes the coordinates is not the same as we pass at beginning. Here when we get a pair of x, y from gpu_benchmarking.tap function, we apply the device_scale_factor and then ConvertLocationToTarget, but in EventProcessor::OnEventFromSource, it tries to transform the event position back from device scale factor. Later when it is finding the target for the event, it tries to convert point to target, then it gets a wrong coordinate. I feel it is easy we do not device_scale_factor at all, and we can make sure we get the right coordinate. What do you think?
https://codereview.chromium.org/2269483002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc (left): https://codereview.chromium.org/2269483002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:38: touch_with_latency.event.touches[i].position.y *= device_scale_factor_; On 2016/08/25 15:19:57, lanwei wrote: > On 2016/08/25 12:36:51, bokan wrote: > > Why did this get removed in the latest patch? > > If we use local coordinates, then we will use event.touches[i].position as the > position in the ui::TouchEvent, when we create ui::TouchEvent from WebTouchEvent > in MakeUITouchEventsFromWebTouchEvents. Then we do not need to apply the > device_scale_factor_, because later we do not converted back as I set transform > event to false, host->dispatcher()->set_transform_events(false). Hmm, in that case, why did we need this in the first place? In other words, if this works, why wouldn't leaving the |*= device_scale_factor| here and removing the |set_transform_events(false)| work? Shouldn't it be equivalent? There's something I'm not understanding here... > > I think here the order is wrong which makes the coordinates is not the same as > we pass at beginning. > Here when we get a pair of x, y from gpu_benchmarking.tap function, we apply the > device_scale_factor and then ConvertLocationToTarget, but in > EventProcessor::OnEventFromSource, it tries to transform the event position back > from device scale factor. Later when it is finding the target for the event, it > tries to convert point to target, then it gets a wrong coordinate. I feel it is > easy we do not device_scale_factor at all, and we can make sure we get the right > coordinate. What do you think? > The comment at https://cs.chromium.org/chromium/src/ui/aura/window_event_dispatcher.h?dr=CSs... seems to say that the dispatcher gets events in physical pixel coordinates (i.e. with device_scale_factor applied. [DeviceIndependentPixels * device_scale_factor_ = physical pixels]). The web_touch we get as a parameter in this method comes as input from tests so it's in DIPs (device independent pixels). So to me it seems we *should* be multiplying by device_scale_factor_ to get it into physical pixels and then handing it off to the EventDispatcher which then *should* divide by device_scale_factor (in WindowEventDispatcher::OnEventProcessingStarted the TransformEventForDeviceScaleFactor the function divides by DSF). Any idea where this chain of events is breaking?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:160001) has been deleted
Description was changed from ========== Avoid transforming the synthetic touch event again in SyntheticGestureTargetAura. The synthetic touch event is already transformed, if we transform it again based on the device scale factor, then we would not find the right target to send the event. BUG=634343 ========== to ========== Set the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura. In ConvertLocationToTarget function, the offset added to the event location is not scaled, so we should apply the screen scale factor to the event location after it has been transformed to the target. Then we can find the right target to send the event using the correct coordinates. BUG=634343 ==========
This looks better, thanks! LGTM
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2269483002/#ps180001 (title: "Scale the position later")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dsansome@chromium.org
dsansome@chromium.org changed reviewers: + dsansome@chromium.org
(I unset the commit bit on this change to try fix a stuck CQ - https://bugs.chromium.org/p/chromium/issues/detail?id=642077. The issue is fixed now so I'm sending this to CQ again for you)
The CQ bit was checked by dsansome@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Set the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura. In ConvertLocationToTarget function, the offset added to the event location is not scaled, so we should apply the screen scale factor to the event location after it has been transformed to the target. Then we can find the right target to send the event using the correct coordinates. BUG=634343 ========== to ========== Set the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura. In ConvertLocationToTarget function, the offset added to the event location is not scaled, so we should apply the screen scale factor to the event location after it has been transformed to the target. Then we can find the right target to send the event using the correct coordinates. BUG=634343 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Set the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura. In ConvertLocationToTarget function, the offset added to the event location is not scaled, so we should apply the screen scale factor to the event location after it has been transformed to the target. Then we can find the right target to send the event using the correct coordinates. BUG=634343 ========== to ========== Set the coordinates of the synthetic touch event correctly in SyntheticGestureTargetAura. In ConvertLocationToTarget function, the offset added to the event location is not scaled, so we should apply the screen scale factor to the event location after it has been transformed to the target. Then we can find the right target to send the event using the correct coordinates. BUG=634343 Committed: https://crrev.com/34bff62e925142091ce98722c7bd447f51b3794b Cr-Commit-Position: refs/heads/master@{#415119} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/34bff62e925142091ce98722c7bd447f51b3794b Cr-Commit-Position: refs/heads/master@{#415119} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
