New issue
Advanced search Search tips

Issue 640706 link

Starred by 2 users
Status: Fixed
Owner:
alex...@chromium.org
Closed: Sep 2016
Cc:
kenrb@chromium.org
a...@chromium.org
creis@chromium.org
alex...@chromium.org
editing-dev@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Some key combinations seem not to work with --site-per-process in either hangout's extension or a cross origin frame

Project Member Reported by ekaramad@chromium.org, Aug 24 2016 
Version: 54.0.2838.0 (Official Build) canary (64-bit)
OS: Mac OSX El Capitan 10.11.6

What steps will reproduce the problem?
(1) Run chrome with --site-per-process.
(2-a) Open hangout extensions, OR
(2-b) Alternatively, open page with cross origin <iframe> and focus and editable region.
(3) Press Ctrl+A or Ctrl+E.

What is the expected output?
The caret should go to the beginning or end of text.

What do you see instead?
The caret does not move.

In addition, the Command + Left Arrow and Right Arrow do not work on hangout (to go to beginning or end of line). For <iframe>, Command+Left/Right arrow is to navigate through history which works as intended. All these key combinations work as intended without --site-per-process.

Command+C/X/V works fine in both extension and <iframe>.

Popular key combinations on Mac for reference: https://support.apple.com/en-ca/HT201236

Comment 1 by lfg@chromium.org, Aug 24 2016

Cc: kenrb@chromium.org a...@chromium.org creis@chromium.org

Comment 2 by karandeepb@chromium.org, Aug 25 2016

Owner: creis@chromium.org
Status: Assigned (was: Untriaged)
Assigning to creis@ who owns the Internals>Sandbox>SiteIsolation label, for further triage.

Comment 3 by creis@chromium.org, Aug 25 2016

Cc: alex...@chromium.org
Owner: kenrb@chromium.org
Thanks for catching this, Ehsan!  I assume this affects OOPIFs in any mode, including --isolate-extensions?

Ken, can you help find an owner for this?  (Ehsan, feel free to take it if you have a lead on where the problem is.)

Also CC'ing Alex, who has worked on keyboard input before.  I imagine this is more of a Mac-specific thing, though, so maybe Avi has ideas.

Comment 4 by ekaramad@chromium.org, Aug 25 2016 

No problem! With --site-per-process we see this on OOPIFs of any kind. On Canary with SiteIsolationExtensions-Enabled this happens in hangout. This issue does not happen in non-OOPIF.

I don't have a lead yet. If this turns out to be a non-urgent, non-blocking issue, I will be interested in poking at it. Otherwise I think Ken should take a look/reassign.

Comment 5 by creis@chromium.org, Aug 25 2016

Labels: -Pri-3 Pri-2
Owner: alex...@chromium.org
Alex offered to help triage this, since Ken is out for a bit.

Comment 6 by creis@chromium.org, Aug 25 2016

Components: UI>Input Blink>Editing
Taking a guess at some other components.

Comment 7 by alex...@chromium.org, Aug 26 2016 

Thanks for filing, Ehsan!  I spent some time digging into this.  The cause appears to be that before we send these keyboard events to the renderer, we also send a InputMsg_SetEditCommandsForNextKeyEvent to the RenderView, which uses it to populate edit_commands_:

  // Stores edit commands associated to the next key event.
  // Shall be cleared as soon as the next key event is processed.
  EditCommands edit_commands_;

With OOPIF modes, we send this IPC to the focused RenderWidget instead, which doesn't handle this IPC, so it just gets dropped.  Later on, during the keydown event dispatch, things eventually go into RenderViewImpl::handleCurrentKeyboardEvent, which sees an empty edit_commands_ and leaves the event unhandled.

The IPC is sent just before calling ForwardKeyboardEvent on the (focused) RWHI.   E.g., for Mac, it's here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_mac.mm?l=2052

For Aura, there's even a TODO which I've totally forgotten about (though not sure how the bug would manifest there):

    // TODO(alexmos): This needs to be refactored to work with subframe
    // RenderWidgetHosts for OOPIF.  See  https://crbug.com/549334 .
    target_host->Send(new InputMsg_SetEditCommandsForNextKeyEvent(
        target_host->GetRoutingID(), edit_commands));
    target_host->ForwardKeyboardEvent(event);

To fix this, we can probably just move edit_commands_ and the IPC handling from RenderView to RenderWidget.  It probably doesn't make sense to store them on RenderView, since they could potentially clash when typing into multiple frame widgets.  I can take a stab at this unless someone else wants to.

Comment 8 by alex...@chromium.org, Aug 26 2016 

Turns out on Linux, the InputMsg_SetEditCommandsForNextKeyEvent is also dropped in OOPIFs, but most of these key bindings are then picked up and translated into editing commands by Editor::handleEditingKeyboardEvent, using one of the tables in EditingBehavior.cpp [1], which are heavily ifdef'ed for Mac.  E.g., ctrl-A translates to SelectAll on Linux but not on Mac.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/EditingBehavior.cpp?l=85.

Comment 9 by alex...@chromium.org, Aug 27 2016

Labels: -OS-Mac OS-All
Status: Started (was: Assigned)
I've got a draft fix and test working; will finish it up next week when I'm back.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 13 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5656749d827dc6af03caed97958bd1ff035d1e61

commit 5656749d827dc6af03caed97958bd1ff035d1e61
Author: alexmos <alexmos@chromium.org>
Date: Tue Sep 13 00:52:46 2016

Add support for edit commands in OOPIFs.

Previously, edit commands from the browser were sent via
InputMsg_SetEditCommandsForNextKeyEvent to focused RenderWidgets, but
for OOPIFs the IPC was dropped since it was only handled in
RenderViews.  To fix this, this CL moves the IPC handling and
associated edit command bookkeeping to RenderWidget.

BUG= 640706 

Review-Url: https://codereview.chromium.org/2287803002
Cr-Commit-Position: refs/heads/master@{#418126}

[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/public/renderer/render_view.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_frame_impl.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_view_impl.cc
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_view_impl.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_widget.cc
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_widget.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/content/renderer/render_widget_owner_delegate.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/Source/core/page/EditorClient.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/Source/web/EditorClientImpl.cpp
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/Source/web/EditorClientImpl.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/5656749d827dc6af03caed97958bd1ff035d1e61/third_party/WebKit/public/web/WebViewClient.h

Comment 11 by alex...@chromium.org, Sep 14 2016 

Verified that this is now fixed in 55.0.2860.0 Mac canary.  Any opinions on whether this is worth merging to M54?

Comment 12 by nasko@chromium.org, Sep 14 2016

Labels: Merge-Request-54
I think it is worthy for merging.

Comment 13 by dimu@chromium.org, Sep 14 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)

Comment 14 by ligim...@chromium.org, Sep 15 2016 

Could you please confirm whether this change is baked/verified in Canary and safe to merge?If yes, merge your change to M54 (branch: 2840) so that we could take this for next Beta Release.

Comment 15 by alex...@chromium.org, Sep 15 2016 

Yes, I've verified the change in Canary and will merge to M54 shortly.
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 15 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b57300894f57164a7edf8d6189cf7fce0854ee57

commit b57300894f57164a7edf8d6189cf7fce0854ee57
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Sep 15 22:11:19 2016

Add support for edit commands in OOPIFs.

Previously, edit commands from the browser were sent via
InputMsg_SetEditCommandsForNextKeyEvent to focused RenderWidgets, but
for OOPIFs the IPC was dropped since it was only handled in
RenderViews.  To fix this, this CL moves the IPC handling and
associated edit command bookkeeping to RenderWidget.

BUG= 640706 

Review-Url: https://codereview.chromium.org/2287803002
Cr-Commit-Position: refs/heads/master@{#418126}
(cherry picked from commit 5656749d827dc6af03caed97958bd1ff035d1e61)

Review URL: https://codereview.chromium.org/2349523002 .

Cr-Commit-Position: refs/branch-heads/2840@{#383}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/public/renderer/render_view.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_frame_impl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_view_impl.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_view_impl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget_owner_delegate.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/page/EditorClient.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/EditorClientImpl.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/EditorClientImpl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/public/web/WebViewClient.h
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b57300894f57164a7edf8d6189cf7fce0854ee57

commit b57300894f57164a7edf8d6189cf7fce0854ee57
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Sep 15 22:11:19 2016

Add support for edit commands in OOPIFs.

Previously, edit commands from the browser were sent via
InputMsg_SetEditCommandsForNextKeyEvent to focused RenderWidgets, but
for OOPIFs the IPC was dropped since it was only handled in
RenderViews.  To fix this, this CL moves the IPC handling and
associated edit command bookkeeping to RenderWidget.

BUG= 640706 

Review-Url: https://codereview.chromium.org/2287803002
Cr-Commit-Position: refs/heads/master@{#418126}
(cherry picked from commit 5656749d827dc6af03caed97958bd1ff035d1e61)

Review URL: https://codereview.chromium.org/2349523002 .

Cr-Commit-Position: refs/branch-heads/2840@{#383}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/public/renderer/render_view.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_frame_impl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_view_impl.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_view_impl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget_owner_delegate.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/page/EditorClient.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/EditorClientImpl.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/EditorClientImpl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/public/web/WebViewClient.h

Comment 18 by alex...@chromium.org, Sep 15 2016

Status: Fixed (was: Started)

Comment 19 by krajshree@chromium.org, Sep 21 2016

Labels: TE-Verified-M54 TE-Verified-54.0.2840.34
Verified the issue on Mac 10.11.6 using chrome beta version #54.0.2840.34 as per the comment #0
Observed that the fix is working as expected.

Attaching screencast for reference

Hence, adding the verified labels.
640706.mp4
1.8 MB View Download
Project Member

Comment 20 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b57300894f57164a7edf8d6189cf7fce0854ee57

commit b57300894f57164a7edf8d6189cf7fce0854ee57
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Sep 15 22:11:19 2016

Add support for edit commands in OOPIFs.

Previously, edit commands from the browser were sent via
InputMsg_SetEditCommandsForNextKeyEvent to focused RenderWidgets, but
for OOPIFs the IPC was dropped since it was only handled in
RenderViews.  To fix this, this CL moves the IPC handling and
associated edit command bookkeeping to RenderWidget.

BUG= 640706 

Review-Url: https://codereview.chromium.org/2287803002
Cr-Commit-Position: refs/heads/master@{#418126}
(cherry picked from commit 5656749d827dc6af03caed97958bd1ff035d1e61)

Review URL: https://codereview.chromium.org/2349523002 .

Cr-Commit-Position: refs/branch-heads/2840@{#383}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/public/renderer/render_view.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_frame_impl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_view_impl.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_view_impl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget.cc
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/content/renderer/render_widget_owner_delegate.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/core/page/EditorClient.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/EditorClientImpl.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/EditorClientImpl.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/b57300894f57164a7edf8d6189cf7fce0854ee57/third_party/WebKit/public/web/WebViewClient.h
►

Sign in to add a comment