critical

alexmos: How is focus set on the parent frame? Mouse clicks focus the webview in browser plugin without oopif, but fail with oopif enabled.


I seem to be able to focus the <webview> in the embedder's console with wv.focus().


====
Testing the webview browser sample, when I click on the webviewL 
no flags: focus
--isolate-extensions: no focus
--isolate-extensions --use-cross-process-frames-for-guests: no focus
--use-cross-process-frames-for-guests: no focus

avallee: I'm not sure what you meant by "on the parent frame", but in general, if you have an A-embed-B scenario with OOPIFs enabled and you click on the B subframe, here's the current flow of events:

1. Browser process hit-tests the click (see RenderWidgetHostInputEventRouter::FindEventTarget) and sends it to B's RenderWidget in B's process (see OnHandleInputEvent).

2. On the blink side, the click is sent to the right element, and FocusController figures out that the B frame should now become focused, so it sends FrameHostMsg_FrameFocused to the browser process.  The flow is EventHandler::handleMousePressEvent -> EventHandler::handleMouseFocus -> FocusController::setFocusedElement -> FocusController::setFocusedFrame -> FrameLoaderClientImpl::frameFocused -> RenderFrameImpl::frameFocused.

3. FrameTree::SetFocusedFrame() updates the focused frame in the browser process and notifies the frame's proxies in other processes involved in rendering the current FrameTree.  In this case, it sends a FrameMsg_SetFocusedFrame to A's process, telling it to unfocus frame A and update the currently focused frame to its (remote) subframe B.

I haven't really looked at how <webview> works with this flow; I'd double check all of these steps.  I'd guess that browser process hit testing works properly, since I can see <webview> reacting to mousemove events (cursor changes, hover effects, etc.), but beyond that I'm not sure.

#6, that's the gist of what I understood from looking at the code.

After speaking with lfg, it seems that the issue might be that: from the webview's point of view, it is a main frame and has no parent. It does not notify the proxy in the embedder. I think the fix involves WebContentsImpl::GetOuterWebContents().

I'm writing a fix that involves moving most of the logic from FrameTree::SetFocusedFrame to WebContentsImpl::SetFocusedFrame.

Knowledge of inner/outer webcontents is necessary to correctly notify loss of focus to frames in separate frame trees.

So I have a WIP patch which deals with sending input to the right place and it feels mostly done.

The problem seems to be page focus. If I select text outside the webview, if starts out blue and then goes dim when focusing in the webview. Clicking back in the embedded makes it blue again. The webview will let me select text, but it is always dimmed.

My guess is that you'll have to make page focus, and more specifically FrameTree::ReplicatePageFocus, aware of inner/outer WebContents as well.

With webview, I'm not sure what the correct behaviour should be. It seems that both the embedder and webview can have a selection. I'm not sure if it makes sense to see that, it could be argued either way.

It feels like the embedder and guest are one page and should have unique focus/selection.

I have a feeling this is why the caret is never activated in the guest.

WIP cl: https://codereview.chromium.org/1934703002/

 Issue 601060  has been merged into this issue.

Issue 608118 has been merged into this issue.

Marking as a release blocker, since this breaks any Chrome app that uses chrome.identity.launchWebAuthFlow (bug 608118, closed as a dupe of this, was a release blocker).

Comment 16: Thanks for pointing that out.  I looked more closely, and I think this is only a problem for users in the SiteIsolationExtensions field trial (where --isolate-extensions is enabled), and that's only live on Canary and Dev.  It shouldn't affect Beta users on M52, which we can verify by passing --force-fieldtrials=SiteIsolationExtensions/Control on M52 Dev.  Can someone confirm?

That said, I'll mark this as a launch blocker for --isolate-extensions.

Re: Comment 17, I've just confirmed that the bug isn't present when you pass --force-fieldtrials=SiteIsolationExtensions/Control, so it won't affect M52 Beta.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

My patch is still in the works.

Without --site-per-process it seems that input in the webview works with both --use-cross-process-frames-for-guests and without. However, it seems that with --site-per-process but without --u-c-p-f-g, input events are routed to the embedder and not forwarded to the guest.

Issue 622505 has been merged into this issue.

Status: My current patch seems to pass my test that I wrote. Input seems to work in all cases except when neither --u-c-p-f-g nor --site-per-process is set.

The no-flag case will be handled (I'm fairly sure) by the fix for https://bugs.chromium.org/p/chromium/issues/detail?id=619906 (which hopefully we'll land in the next day or so).

This issue is Pri-1 but has already been moved once. Lowering the priority and moving to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit 0206f78f31bafd45b2a9479d0311b4e7e11acf7a
Author: avallee <avallee@chromium.org>
Date: Thu Jul 28 18:55:33 2016

Fix keyboard focus for OOPIF-<webview>.

When using --use-cross-process-frames-for-guests input events were being
routed to the embedder which should not be able to process guests' input
events.

+ WebContents tree traversal from parent to child.
+ WebContents keeps global track of focused frame, unlike frame tree
  which might not know about inner/outer web contents.
+ RenderFrameHostImpl::OnFrameFocused delegates back to WebContents
  instead of frame tree.
+ Send input to Webcontents that is focused.
+ Add unfocus message to renderer. When moving focus from a frame in the
  webview back to the embedder, the inner contents no longer has any focus
  (the frame with focus is not visible to the inner guest).
~ Changed frame focusing with the keyboard. Tabbing into the webview did not cause focus to properly propagate everywhere.
~ Disabled NOTREACHED when finished tabbing through the webview. Left note to crbug for properly moving focus to the next element in tab order.
~ Left note on re-enabling WebViewAccessibilityTest::FocusAccessibility.
~ Reworked RenderWidgetHostViewGuest to use
  MaybeSendSyntheticTapGesture for Touch and Mouse event.
~ Edited SelectShowHide for direct routing.

BUG= 600395 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/chrome/test/data/extensions/platform_apps/web_view/focus/inject_focus.js
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/chrome/test/data/extensions/platform_apps/web_view/popup_positioning/main.js
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/render_frame_proxy_host.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/browser/web_contents/web_contents_view_child_frame.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/common/frame_messages.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/public/test/browser_test_utils.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/content/renderer/render_frame_impl.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/0206f78f31bafd45b2a9479d0311b4e7e11acf7a/third_party/WebKit/public/web/WebView.h

Thanks!  Can this be marked fixed now?

@marcin3k: Could you please have a look at the attached video and let us know if this is the correct procedure/expected result ?

Thank you.

Deleted: 600395.mp4 1.3 MB

	600395.mp4 1.3 MB View Download

@rnimmagadda: Yes, this looks good/fixed to me. Thanks! :)

Verified the fix on Windows 7, MAC (10.11.6) & Ubuntu Trusty (14.04) for Google Chrome Dev Version - 54.0.2816.0

Screen-recording is attached in the comment #27

TE-Verified labels are added.

@avallee: Please change the Status accordingly.

Thank you.

There is still some general cleanup in this area, but this specific issue is fixed.