IME does not work inside OOPIFs |
|||||||||||
Issue descriptionChrome 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.
,
Feb 5 2017
+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?
,
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
,
Feb 14 2017
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.
,
Feb 14 2017
I verified that IME does not work in hangouts for Chrome beta 57.0.2987.0. Adding the label for Merge to M-57.
,
Feb 14 2017
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.
,
Feb 14 2017
,
Feb 14 2017
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.
,
Feb 14 2017
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
,
Feb 14 2017
If possible, could you please merge your CL into M57 branch 2987 before 5 PM PT today, Tuesday (02/14/17). Thank you.
,
Feb 14 2017
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
,
Feb 15 2017
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.!
,
Feb 15 2017
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.
,
Feb 15 2017
Verified the issue on Windows 10(Thank you ekaramad@) on latest Chrome beta 57.0.2987.54.
,
Feb 15 2017
,
Feb 16 2017
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 |
|||||||||||
Comment 1 by ekaramad@chromium.org
, Feb 5 2017