ananta/robliao: I'm looking to see if we can get this fixed, but I'm having trouble repro'ing it.  I just tried on a Windows 10 tablet (detached from its keyboard) that I borrowed from rpop@ and I wasn't able to get the on screen keyboard to show up at all, even for taps on the omnibox or in the main frame.  I had to use a taskbar icon to manually show the keyboard.

Would that have been just an issue with that particular device, or maybe the 57.0.2939.0 Canary I was using?  I'm not sure how to confirm the bug or a fix at the moment.

wjmaclean: You mentioned there's a flag I might need to set to get the onscreen keyboard to display when focusing an input field.  Is that --disable-usb-keyboard-detect?  (I missed that in the original bug report when testing.)

I was thinking of

--enable-virtual-keyboard

There are a number of "virtual keyboard" flags:
--use-new-virtual-keyboard-behavior
--disable-smart-virtual-keyboard
--disable-virtual-keyboard-overscroll
--disable-smart-virtual-keyboard 

I'm not sure what distinguishes "smart" from "regular", and it's been a while since I used the basic --enable flag, so I'm not quite sure of the current status of any of these.

I think a proper fix should move the "focusedNodeChanged" to WebWidgetClient from WebViewClient and have the IPC sending in RenderWidget. Then on the browser side handle it in RWHImpl and some coordination between messages from multiple RWHs.

But I guess a short term but racy solution for this problem is to send the IPC through focused frame and forward it to RenderViewHostImpl.

Ok, I think the ananta's original bug report had all the info we needed, but I didn't know the meaning of some of the info.  For those unfamiliar with Windows tablets, here's a bit more detail:

1) On a Microsoft Surface or other Windows tablet, start Chrome with --site-per-process --disable-usb-keyboard-detect (using a command prompt, with the keyboard attached).
2) Visit http://csreis.github.io/tests/cross-site-iframe-simple.html and verify that there's a subframe process in Chrome's Task Manager.
3) Detach the keyboard.
4) In the Windows sidebar, enable Tablet Mode.
5) Tap on the omnibox and ensure the onscreen keyboard shows up.  (This won't happen unless you're in Tablet Mode with the keyboard detached and running with a flag like --disable-usb-keyboard-detect.)
6) Tap on the page outside a text input to dismiss the onscreen keyboard.
7) Tap on a form input inside the out-of-process iframe.

The onscreen keyboard should show up but doesn't.  Works without --site-per-process.

There is a workaround: you can open the onscreen keyboard manually using the icon in the tray.

ekaramad@ offered to take a look, or possibly help find an owner if things are busy.  Let's aim to get this into M56 if we can, though it's probably not strictly a blocker since there's a usable workaround.

Note: ekaramad@ has a CL in progress here: https://codereview.chromium.org/2571583008/

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

commit a110f64a89bad15abb5322be8e720fae40a0f7f1
Author: ekaramad <ekaramad@chromium.org>
Date: Wed Dec 21 19:47:28 2016

Support tracking focused node element for OOPIFs.

Focused elements are tracked by RenderViewHostImpl. Upon a change in focused
element, RenderViewImpl sends ViewHostMsg_FocusedNodeChanged to the browser.

With --site-per-process, RenderViewImpl corresponding to OOPIFs is swapped
out and cannot send the IPC. Even by allowing sending such IPCs in swapped
out state, the RenderViewHost cannot properly handle them when there are
multiple RenderWidgetHosts (same process) on the page. This is now causing
regressions with OSK for windows tablets.

This CL will move the logic from RenderViewImpl to RenderFrameImpl.
When focus moves from one WebNode to another, the corresponding frames are
already getting notified by RenderViewImpl. This CL relieves RenderViewImpl
from sending the IPC, and reuses the RenderFrameImpl::FocusNodeChanged() for
sending the update to RenderFrameHostImpl, which then stores the state and
notifies its delegate (WebContentsImpl) about the change.

For similar reasons, the logic to update FocusedNodeTouched is removed from
RenderViewImpl to RenderWidget and is now handled by RenderWidgetHostImpl.

The public API methods in RenderViewHost are unchanged but they are now
handled through RenderViewHostDelegate (WebContentsImpl), which will
query the information or send the command through the focused frame.

BUG= 613326 
TBR=creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_view_host_delegate.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_view_host_delegate.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/common/frame_messages.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/common/view_messages.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/renderer/render_frame_impl.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/renderer/render_view_impl.cc
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/renderer/render_view_impl.h
[modify] https://crrev.com/a110f64a89bad15abb5322be8e720fae40a0f7f1/content/renderer/render_widget.cc

I verified that the bug is fixed on Canary (57.0.2970.0). I tested comment #8 on a Windows 8 touch machine.

However, when the <input> would be obscured by the OSK, in SPP, we do not scroll the root view up. This is because ScollFocusedElementIntoRect is not implemented for OOPIF. This is being tracked in  bug 676037 .

As far as the tracking of the two IPCs mentioned above is concerned, we should be good now. I will mark this as fixed.

Do we want to merge this fix to M56?

Comment 12: That's a good question.  It would be nice to have this bug fixed for M56, since it may affect users as we turn on --isolate-extensions, such that the keyboard may not automatically appear when tapping on an OOPIF.  That said, it's a large CL, and there is a workaround (just tapping the keyboard icon in the tray if the keyboard doesn't show up).

Nick mentioned to me that the CL is large but not that complicated-- just a lot of plumbing.  Plus it has been baking on M57 for several weeks.  Those are points in favor of merging.

ekaramad: Can you give us a sense for what risks there would be for merging it?  I'd suggest posting that to the bug and adding the Merge-Request-56 label, so that the TPMs can help make the call.

Comment 13: I can't quite see a big risk in merging to M56. Basically,

1-Risk of crashes and major regressions: minimal since it is mostly (as Nick said) moving code around.

2- Risk of breaking features for main frame/view: The features involved seem to be:
  1- OSK for Windows (8 and 10) which is fixed by tracking    FocusedNodeTouched.
  2- AccessbilityHighlightManager and MaginificationController which are both used in ChromeOS.

Without this patch 2.1 is broken for OOPIFs. 2.2 is probably broken although I have not tested it yet (I will do so by Monday).

With this patch I saw no difference in behavior of 2.1 for main frame and it fixes it for OOPIFs for the most part (minus  issue 676037  which causes the focused <input> inside OOPIF not to be scrolled up when OSK covers it). Again, for 2.2 I have to verify what is broken and what would be fixed.

Overall I find this safe to be merged and will add the label for now. Meanwhile, I will poke around 2.2 on my chromebook and will post updates here.

Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

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

Some updates:
A- I have created the Merge to M-56 CL. Will probably land soon.

B- Experimenting with ChromeOS:
1- AccessibilityHighlightManager: I believe this class is supposed to put a circle where caret goes. It already works on M-56 since as I understand, it is based on InputMethodObserver logic. In fact, I think, AccessibilityHightlightManager should not even observe the focused node changed to begin with.

2- MagnificationController: There seems to be a change from M56 to M57. On M56 when focus jumps betweem buttons (non-editable element), the window does not center on the element position. In M57 it does, which is most probably due to this patch.

All in all I guess we should be all set for merging.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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/+/5148b7c299c9adc09ca11cce1b16fe33febf749e

commit 5148b7c299c9adc09ca11cce1b16fe33febf749e
Author: ekaramad <ekaramad@chromium.org>
Date: Tue Jan 10 17:37:47 2017

Support tracking focused node element for OOPIFs.

Focused elements are tracked by RenderViewHostImpl. Upon a change in focused
element, RenderViewImpl sends ViewHostMsg_FocusedNodeChanged to the browser.

With --site-per-process, RenderViewImpl corresponding to OOPIFs is swapped
out and cannot send the IPC. Even by allowing sending such IPCs in swapped
out state, the RenderViewHost cannot properly handle them when there are
multiple RenderWidgetHosts (same process) on the page. This is now causing
regressions with OSK for windows tablets.

This CL will move the logic from RenderViewImpl to RenderFrameImpl.
When focus moves from one WebNode to another, the corresponding frames are
already getting notified by RenderViewImpl. This CL relieves RenderViewImpl
from sending the IPC, and reuses the RenderFrameImpl::FocusNodeChanged() for
sending the update to RenderFrameHostImpl, which then stores the state and
notifies its delegate (WebContentsImpl) about the change.

For similar reasons, the logic to update FocusedNodeTouched is removed from
RenderViewImpl to RenderWidget and is now handled by RenderWidgetHostImpl.

The public API methods in RenderViewHost are unchanged but they are now
handled through RenderViewHostDelegate (WebContentsImpl), which will
query the information or send the command through the focused frame.

BUG= 613326 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2571583008
Cr-Commit-Position: refs/heads/master@{#440185}
(cherry picked from commit a110f64a89bad15abb5322be8e720fae40a0f7f1)

Review-Url: https://codereview.chromium.org/2623483003
Cr-Commit-Position: refs/branch-heads/2924@{#717}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_view_host_delegate.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_view_host_delegate.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/common/frame_messages.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/common/view_messages.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/renderer/render_frame_impl.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/renderer/render_view_impl.cc
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/renderer/render_view_impl.h
[modify] https://crrev.com/5148b7c299c9adc09ca11cce1b16fe33febf749e/content/renderer/render_widget.cc

Tested the issue on Dell Win 10.0 touch screen using chrome latest Beta M56-56.0.2924.59 by following steps mentioned in the original comment and comment #8. Observed that the on screen keyboard is displaying as expected. Hence adding TE-Verified label.

Please find the screen shot for reference.

Thank you!

Deleted: 613326.mp4 1.9 MB

	613326.mp4 1.9 MB View Download