MacViews: 2-Set-Korean IME enters some sort of ~compatibility mode after the first omnibox input. Requires OS reboot. |
||||||
Issue descriptionChrome Version : 69.0.3481.0 OS Version: OS X 10.13.5 What steps will reproduce the problem? 1. Add/Enable 2-Set Korean input method via System Prefs -> Keyboard 2. Focus omnibox and type 'ko' 3. Press Enter (once) 4. Repeat step 2 and observe 5. Press Enter (now you need to press it a second time). What is the expected result? Same behaviour each time. Also, step 3 should trigger a search for "ㅏㅐ" not just "ㅏ". Step (with double-enter) correctly searches for "ㅏㅐ", but it shouldn't need two enters. What happens instead of that? Different. The second time, the active composition gets an underline on the most recent character input, which only goes away after the first press of Enter, to "commit" the composition. Restarting chrome doesn't restore the old behaviour. However, rebooting the *OS* does (once, until you repro again). This is related to Issue 817097 , which fixed a crash / lifetime issue around this. After r572396 we no longer crash, and stuff "works". However, I suspect that by ignoring AppKit's attempt to access an object we're trying to invalidate, it's saying "OK, fine, you don't support me doing that, so I'm going to put you in the mode that doesn't attempt to UAF". Other input methods for phonetic languages (e.g. Vietnamese) are likely affected as well. Safari and CocoaChrome support the lazy-composition mode. Now, technically, this wouldn't UAF since we're only moving focus _away_ from a textfield - not destroying it. We don't currently distinguish between the two, but I don't think we need to: "There is hope". That is, a views::Textfield has Textfield::~Textfield() { if (GetInputMethod()) { // The textfield should have been blurred before destroy. DCHECK(this != GetInputMethod()->GetTextInputClient()); } } That is, toolkit-views _already_ guarantees that it will shift focus away before destroying the Textfield. Hopefully... all we need to do is tweak the ordering in -[BridgedContentView setTextInputClient:] so that it invokes -[NSApp updateWindows] *before* changing the data member, not after. The annoying part is that we apparently need to reboot every time we want to verify the fix :/. (even recompiling Chromium or hacking the bundle ID doesn't unstick it -- must be some special state in the macOS IME engine #whoreallyknows).
,
Jul 12
,
Jul 12
,
Jul 12
,
Jul 12
,
Jul 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27a4cd8f3d841335f7954b48e162084a19336d80 commit 27a4cd8f3d841335f7954b48e162084a19336d80 Author: Trent Apted <tapted@chromium.org> Date: Tue Jul 17 00:05:52 2018 MacViews IME: Fix handling of phonetic/inline IME (e.g Korean) Currently IME for phonetic languages such as Korean is pretty broken in MacViews text fields. Even in stable, the last character entered in a textfield ('ㅂ', say, from pressing 'q') will disappear when pressing tab. A similar thing happens with the Return key in the Omnibox. This is a huge dealbreaker. Phonetic languages such as Korean and Vietnamese use an IME style that doesn't _mark_ text while it is being composed. This breaks some assumptions around how unicode insertText: events are attached to ui::KeyEvents. It also comes with some weird behaviour from the IME on macOS (e.g. sending 2 identical replaceText messages), adding extra complexity. The handling got extra bad after r565848 when attaching to a Tab key could cause 2 focus changes while the IME was dismissing itself, causing the input context to change again, thereby triggering an IME dismissal recursively. To fix these issues, only dispatch character events as a ui::KeyEvent in insertText: when the character attached to the key event matches the first unicode text given to insertText: (in addition to the other checks). This prevents a tab keypress that dismisses an IME (and inserts text) as being treated as a tab. But, for inline IME, we still want to process that tab, so also leave it "unhandled" to be picked up by the unhandled key event handler. This also prevents recursive calls to updateWindows coming from known client code. However, the badness is bad enough that we should also protect against recursive calls. Add logic for that and test coverage that simulates the IME interaction. Test: Enable the "2-Set Korean" IME on Mac and interact with the omnibox. E.g. pressing the return key should not cause the last character to disappear (and not appear in the search result). Bug: 860362 Change-Id: I04fc7d9590563413021de99921864ab079f9b044 Reviewed-on: https://chromium-review.googlesource.com/1128685 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#575488} [modify] https://crrev.com/27a4cd8f3d841335f7954b48e162084a19336d80/ui/views/cocoa/bridged_content_view.h [modify] https://crrev.com/27a4cd8f3d841335f7954b48e162084a19336d80/ui/views/cocoa/bridged_content_view.mm [modify] https://crrev.com/27a4cd8f3d841335f7954b48e162084a19336d80/ui/views/cocoa/bridged_native_widget_unittest.mm
,
Jul 17
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by tapted@chromium.org
, Jul 9Status: Started (was: Assigned)