Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2101663002:
Send synthetic GestureTapDown to focus BrowserPlugin (Closed)
|
Created:
4 years, 5 months ago by wjmaclean Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, Jay Lee Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend synthetic GestureTapDown to focus BrowserPlugin
Since we now route Gesture events directly to the guest renderer, there
is no longer any event to focus the BrowserPlugin in the embedder
renderer, meaning that even though touch/gesture events work properly,
the guest may not receive keyboard input.
This CL provides a temporary fix by sending a synthetic GestureTapDown
to the embedder on TouchStarti, in parallel with sending the TouchStart
directly to the guest. The synthetic event focuses the BrowserPlugin.
This CL also fixes two tests that were still sending Gestures to the
embedder.
When BrowserPlugin is removed, this code will disappear along with
RenderWidgetHostViewGuest.
BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/347f2466211ae7ebbbc0a01f0f564274dc5fa3c2
Cr-Commit-Position: refs/heads/master@{#403308}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix coordinate conversions for focusing gestures. #
Total comments: 6
Patch Set 3 : Address suggestions. #
Messages
Total messages: 36 (13 generated)
Description was changed from ========== Send synthetic GestureTapDown to focus BrowserPlugin Since we now route Gesture events directly to the guest renderer, there is no longer any event to focus the BrowserPlugin in the embedder renderer, meaning that even though touch/gesture events work properly, the guest may not receive keyboard input. This CL provides a temporary fix by sending a synthetic GestureTapDown to the embedder on TouchStarti, in parallel with sending the TouchStart directly to the guest. The synthetic event focuses the BrowserPlugin. This CL also fixes two tests that were still sending Gestures to the embedder. When BrowserPlugin is removed, this code will disappear along with RenderWidgetHostViewGuest. BUG=619906 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Send synthetic GestureTapDown to focus BrowserPlugin Since we now route Gesture events directly to the guest renderer, there is no longer any event to focus the BrowserPlugin in the embedder renderer, meaning that even though touch/gesture events work properly, the guest may not receive keyboard input. This CL provides a temporary fix by sending a synthetic GestureTapDown to the embedder on TouchStarti, in parallel with sending the TouchStart directly to the guest. The synthetic event focuses the BrowserPlugin. This CL also fixes two tests that were still sending Gestures to the embedder. When BrowserPlugin is removed, this code will disappear along with RenderWidgetHostViewGuest. BUG=619906 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
wjmaclean@chromium.org changed reviewers: + creis@chromium.org, tdresser@chromium.org
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
creis@ - small CL, please take a look? This is to fix a regression that unfortunately made it to stable. tdresser@ - I just wanted to verify with you that it's OK to send a lone GestureTapDown (it seems to be). If not, we can send a more complete sequence.
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.
Happy to rubber stamp once an input reviewer approves, since I don't have the background for a knowledgable review here. Also, it would be ideal to include a test in this CL, if that's possible. https://codereview.chromium.org/2101663002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2101663002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:195: ui::LatencyInfo()); Is there any need to send an equivalent "up" event?
On 2016/06/27 17:14:56, wjmaclean wrote: > creis@ - small CL, please take a look? This is to fix a regression that > unfortunately made it to stable. > > tdresser@ - I just wanted to verify with you that it's OK to send a lone > GestureTapDown (it seems to be). If not, we can send a more complete sequence. tdresser is on vacation today but back tomorrow. I dug around a bit to see if it would be easy to explicitly trigger the focus rather than relying on a fake input event. It looks like this works today by WebPluginContainerImpl::handleGestureEvent invoking WebPluginContainerImpl::focusPlugin on every 'gesturetapdown' event. It's probably non-trivial to trigger that today from the content layer without following the gesture event path. Relying on a duplicate GTD event in the embedder seems like an OK compromise to me. However, a bare GestureTapDown is an invalid gesture sequence which could cause confusion in theory, but I'm not aware of any issue in practice offhand (most of the system ignores GTD events). It would probably be less risky IMHO to immediately also include a GestureTapCancel.
On 2016/06/28 14:22:36, tdresser wrote: > I'm holding off on review until you've fixed the source of flake. Ok, I've made some headway. It turns it it's not 'flake' (definitely does not seem to be racy). It *appears* to be a matter of the initial focusing of the view, though I don't fully understand it yet. Here's what I know: 1) If I want to give focus to a WebView, and the embedder window is at (0,0) in the screen, then this all works fine. 2) As the embedder window moves, the ability of the GestureTapDown/Cancel to work diminishes ... in particular, once we've moved by more that one window width (and presumably height), then focus never works. 3) But focus will succeed if we call RenderWidgetHostViewGuest::Focus and then send the focussing gestures to the embedder. But this fails in that it seems the Guest window then never loses focus afterwards, say we tap outside the guest's area in the embedder. By contrast, the focus works as intended no matter where the window is if we are using mouse clicks. It should be noted that for touches and mouse clicks on the same screen location, both will show the same local coordinates, but very different "global/screen" coordinates. But I don't know if this is related. I kind of wonder if this isn't maybe an issue that's been around for a while; after all, without --site-per-process (and related flags) how often do we get a focusable view that isn't a WebView guest? Landing a modified version of this CL (I still have to upload the version with the GestureTapCancel) is no worse than what we have at present, and better w.r.t the bug, but it's not the whole story.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Ok, I think this works properly now. It seems that before the BrowserPlugin is focused, we need to send the GestureTapDown in screen coordinates. I'm also send the GestureTapCancel in screen coordinates, though not sure if that is needed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/28 23:06:50, wjmaclean wrote: > Ok, I think this works properly now. It seems that before the BrowserPlugin is > focused, we need to send the GestureTapDown in screen coordinates. I'm also send > the GestureTapCancel in screen coordinates, though not sure if that is needed. I describe things incorrectly: it seems that, before the view is focused, somewhere along the way the screen offset of the window *gets added* to the event, meaning that blink cannot correctly hit test it ... so we subtract the offset before send the event to the embedder.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:188: GetOwnerRenderWidgetHostView()->GetBoundsInRootWindow(); I think using an explicit type here would be better. Maybe just: gfx::Vector2D offset = GetViewBounds().origin() - GetOwnerRenderWidgetHostView()->GetBoundsInRootWindow(); You might want to add a comment describing why the offset is needed, or pick a more descriptive name. https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:195: event.touches[0].position.y + offset_in_owner.y() - owner_offset.y(); Can you do this math directly on the Point/Vector objects, instead of handling x/y separately? (If it isn't cleaner, don't bother) https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:202: ui::LatencyInfo()); We should send a tap cancel here, to ensure the event stream is valid. This seems correct.
LGTM, but please do add a test for this if possible (either here or in a followup).
On 2016/06/30 18:35:17, Charlie Reis wrote: > LGTM, but please do add a test for this if possible (either here or in a > followup). Thanks Charlie ... I have a test about 99% complete, but will land in a separate CL after this to simplify the merging of this CL.
I'll give this a dry-run before putting in the CQ. https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:188: GetOwnerRenderWidgetHostView()->GetBoundsInRootWindow(); On 2016/06/30 13:20:57, tdresser wrote: > I think using an explicit type here would be better. > > Maybe just: > gfx::Vector2D offset = GetViewBounds().origin() - > GetOwnerRenderWidgetHostView()->GetBoundsInRootWindow(); > > You might want to add a comment describing why the offset is needed, or pick a > more descriptive name. Done. https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:195: event.touches[0].position.y + offset_in_owner.y() - owner_offset.y(); On 2016/06/30 13:20:57, tdresser wrote: > Can you do this math directly on the Point/Vector objects, instead of handling > x/y separately? (If it isn't cleaner, don't bother) WebFloatPoint supports conversion to gfx::PointF but that won't add with gfx::Vector2d, so we need to deal with individual coordinates somewhere along the way. I've simplified it somewhat. https://codereview.chromium.org/2101663002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:202: ui::LatencyInfo()); On 2016/06/30 13:20:57, tdresser wrote: > We should send a tap cancel here, to ensure the event stream is valid. This > seems correct. Acknowledged.
The CQ bit was checked by wjmaclean@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 wjmaclean@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...
On 2016/06/30 20:33:47, wjmaclean wrote: > I'll give this a dry-run before putting in the CQ. > Ok, maybe I'll upload the changes, *then* do the dry-run ;-)
Test in separate CL at https://codereview.chromium.org/2116663002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2101663002/#ps40001 (title: "Address suggestions.")
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 ========== Send synthetic GestureTapDown to focus BrowserPlugin Since we now route Gesture events directly to the guest renderer, there is no longer any event to focus the BrowserPlugin in the embedder renderer, meaning that even though touch/gesture events work properly, the guest may not receive keyboard input. This CL provides a temporary fix by sending a synthetic GestureTapDown to the embedder on TouchStarti, in parallel with sending the TouchStart directly to the guest. The synthetic event focuses the BrowserPlugin. This CL also fixes two tests that were still sending Gestures to the embedder. When BrowserPlugin is removed, this code will disappear along with RenderWidgetHostViewGuest. BUG=619906 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Send synthetic GestureTapDown to focus BrowserPlugin Since we now route Gesture events directly to the guest renderer, there is no longer any event to focus the BrowserPlugin in the embedder renderer, meaning that even though touch/gesture events work properly, the guest may not receive keyboard input. This CL provides a temporary fix by sending a synthetic GestureTapDown to the embedder on TouchStarti, in parallel with sending the TouchStart directly to the guest. The synthetic event focuses the BrowserPlugin. This CL also fixes two tests that were still sending Gestures to the embedder. When BrowserPlugin is removed, this code will disappear along with RenderWidgetHostViewGuest. BUG=619906 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Send synthetic GestureTapDown to focus BrowserPlugin Since we now route Gesture events directly to the guest renderer, there is no longer any event to focus the BrowserPlugin in the embedder renderer, meaning that even though touch/gesture events work properly, the guest may not receive keyboard input. This CL provides a temporary fix by sending a synthetic GestureTapDown to the embedder on TouchStarti, in parallel with sending the TouchStart directly to the guest. The synthetic event focuses the BrowserPlugin. This CL also fixes two tests that were still sending Gestures to the embedder. When BrowserPlugin is removed, this code will disappear along with RenderWidgetHostViewGuest. BUG=619906 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Send synthetic GestureTapDown to focus BrowserPlugin Since we now route Gesture events directly to the guest renderer, there is no longer any event to focus the BrowserPlugin in the embedder renderer, meaning that even though touch/gesture events work properly, the guest may not receive keyboard input. This CL provides a temporary fix by sending a synthetic GestureTapDown to the embedder on TouchStarti, in parallel with sending the TouchStart directly to the guest. The synthetic event focuses the BrowserPlugin. This CL also fixes two tests that were still sending Gestures to the embedder. When BrowserPlugin is removed, this code will disappear along with RenderWidgetHostViewGuest. BUG=619906 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/347f2466211ae7ebbbc0a01f0f564274dc5fa3c2 Cr-Commit-Position: refs/heads/master@{#403308} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/347f2466211ae7ebbbc0a01f0f564274dc5fa3c2 Cr-Commit-Position: refs/heads/master@{#403308} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
