New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 688842 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

IME does not work inside OOPIFs

Project Member Reported by ekaramad@chromium.org, Feb 5 2017

Issue description

Chrome Version: 58.0.3003.0 (Official Build) canary (64-bit)
OS: All

What steps will reproduce the problem?
(1) Open chrome with --site-per-process.
(2) Navigate to a page with an OOPIF which contains a text field.
(3) Enable some IME and type.

What is the expected result?
IME should work.

What happens instead?
Composition window appears and shows current options but the <input> remains empty.


 
This is a regression caused by my own CL:
https://codereview.chromium.org/2608293002/.

For OOPIFs IME events to set and commit composition are not handled since they now rely on has_focus_ == true. |has_focus_| reflects page focus and is only updated for RenderViewImpl.
Cc: alex...@chromium.org
+alexmos@: Alex, I noticed you had worked on the page focus propagation. 

Issue: The CL in comment #1 made some IME IPC handlers in RenderWidget early return when |has_focus_| is false. |has_focus_| seems to be always false for RenderWidgets corresponding to OOPIFs (which might be incorrect?).

Before the CL in comment #1 we would look at WebFrameWidgetImpl::m_imeAcceptEvents to determine wether or not WebFrameWidgetImpl handles IME calls. m_imeAcceptEvents tracks WebWidget focus which is the same as |has_focus_| with the only exception that it is initialized to true. Therefore, for OOPIFs we were still not tracking the page focus as intended but we were always accepting the IME commands (which might be incorrect in some cases).

A temporary fix for me would be to change RenderWidget::ShouldHandleImeEvents to return true when the widget is for OOPIF. But if page focus should be tracked by all OOPIF RenderWidgets, then I could as well as the logic (perhaps inside RenderViewImpl::OnSetFocus) to propagate page focus inside each renderer. WDYT?
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 10 2017

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

commit b6483a05119a0ebb57d3968783371c119a83d49c
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Feb 10 02:23:26 2017

Fix a recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG= 688842 ,  602723 

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

[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/content/renderer/render_widget.cc

I verified that this issue is fixed on Chrome Canary for MacOSX version 58.0.3011.0. That being said, the culprit was at 441788 which is before the position for current beta. I think we need to merge this back to M57. I will verify that soon.

Labels: Merge-Request-57
I verified that IME does not work in hangouts for Chrome beta 57.0.2987.0. Adding the label for Merge to M-57.

Comment 6 by creis@chromium.org, Feb 14 2017

Labels: M-57
Thanks for catching this and landing the fix!  Yes, let's definitely get it merged to M57.  And from comment 1, it sounds like this was a regression from r441788, and thus not in M56?  Glad it doesn't affect stable users.
Labels: M-58
Status: Fixed (was: Started)
Sure. I will merge this as soon as I get the approval. Just FYI, the underlying issue for the regression was page focus which is tracked in bug 689777.
Also marking this bug as fixed. 

It is also worth noting that the fix is small and has been on Canary since Feb 11.
Project Member

Comment 9 by sheriffbot@chromium.org, Feb 14 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
If possible, could you please merge your CL into M57 branch 2987 before 5 PM PT today, Tuesday (02/14/17). Thank you.
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 14 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba75a5aca3afb730adf8699e9f6951031291d58a

commit ba75a5aca3afb730adf8699e9f6951031291d58a
Author: ekaramad <ekaramad@chromium.org>
Date: Tue Feb 14 21:21:41 2017

Fix a recent regression in IME inside OOPIFs (Merge to M-57)

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG= 688842 ,  602723 
NOTRY=TRUE
NOPRESUBMIT=TRUE
Review-Url: https://codereview.chromium.org/2681473002
Cr-Commit-Position: refs/heads/master@{#449521}
(cherry picked from commit b6483a05119a0ebb57d3968783371c119a83d49c)

Review-Url: https://codereview.chromium.org/2696883002
Cr-Commit-Position: refs/branch-heads/2987@{#509}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/content/renderer/render_widget.cc

Cc: ranjitkan@chromium.org pbomm...@chromium.org gov...@chromium.org
Labels: Needs-Feedback
To verify this issue, created the attached examples and added IME extension from Webstore and followed the below steps:

1) Launch chrome with flag --site-per-process enabled 
2) Launched iframe.html page (Downloaded the 3 attached files in a single folder)
3) Downloaded and installed chrome IME from the webstore and set some input languages (Japanese)
4) Typed some input text but IME options were not displayed.

Not sure if the test case I created are correct. Could someone please help us with a sample test case, so that the issue can be verified.

Thanks.!
Iframe.html
275 bytes View Download
tests.css
202 bytes View Download
formPage.html
184 bytes View Download
This bug was related to OS installed IMEs and not the IME extension. To verify you would need to install another language and keyboard on your system, e.g., Pinyin - Simplified which I just tried on MacOSX.

As for the IME extension, the behavior I am finding is quite curios. 

On Chrome 58.0.3012.0 (Official Build) canary (64-bit)
   Chrome 57.0.2987.37 (Official Build) beta (64-bit)
   Chrome 56.0.2924.87 (Official Build) stable (64-bit)
If we go to "a.com" and then load an iframe in "a.com" the IME extension seems to work (I am trying Google Input Methods and the emoji input). If I navigate the iframe to "b.com" then the Emoji does not work. This happens with and without site-per-process. Even more curious; if I navigate back to "a.com" the extension still won't work.

I will track this issue in a separate bug.
Labels: TE-Verified-
Verified the issue on Windows 10(Thank you  ekaramad@) on latest Chrome beta 57.0.2987.54.
Labels: -TE-Verified- TE-Verified-57.0.2987.54
IME works fine on http://csreis.github.io/tests/cross-site-iframe-simple.html.
Test on Chrome 57.0.2987.54/CrOS 9202.28.0 - Reks

Sign in to add a comment