tapted@, could this be an issue caused by your fix for  bug 883952 ?

+cc folks for thoughts on adding views::Textfield plumbing in order to change the omnibox search trigger on macOS.

Note the fix for  Issue 883952  is not merged to m69.

However, there is a Korean IME  issue 860362  which made the original m69 release (before that, Korean IME was rather broken in m69 *dev*).

https://productforums.google.com/forum/#!topic/chrome-ko/zoVhQJCCF6E;context-place=forum/chrome-ko talks about upgrading to Mojave.

I think this is actually a regression caused by Apple updating the behaviour of Korean IME.

In 10.13.5, the IME marks the character that is the active composition. On a 10.13.2 machine I had lying around, it does not. The behavior is the same for the latest m69 and m71 channel on each macOS version -- it is the version of macOS that changes behavior, not the version of Chrome.

This is rather frustrating.

I think we're at a kind of fundamental difference in IME behaviour across platforms, and we need to update *omnibox* code to handle this to be most robust. The old Cocoa omnibox triggered by the IME saying "please insert a newline", however OmniboxViewViews::HandleKeyEvent() watches for a ui::KeyEvent for VKEY_RETURN.

On Mac, IME does not suppress key events. A VKEY_RETURN that IME wants to "use" instead simply never calls -[insertText:@"\r"].

So I propose we change the omnibox trigger on Mac. Probably the way this needs to be done is to add something like `

  // Invoked when newline character would be inserted at the current cursor position,
  // but was ignored due to the Textfield not supporting multiline. This is never
  // called from Textfield::OnKeyPressed(), only calls via the ui::TextInputClient
  // interface trigger this.
  virtual views::Textfield::OnNewLine() {}


The downside of pressing Enter twice in Korean to search is annoying, but it's a lot better than  Issue 883952  or  Issue 860362 . Not worth trying to merge IMO.

Grr. Now on macOS 10.13.6 (17G65) and I can't reproduce this. TouchBar mac.

Anyway, while procrastinating, I've thought of a small tweak I can make to r593766 that should be a robust way to cover this, without needing changes elsewhere.

Basically, we can't detect when IME "eats" an Enter keypress (e.g. to dismiss the dialog). However, we can detect whether the IME definitely tried to insert a newline (i.e. even if there is marked text). In that case, on Enter, we can assume Textfield ignores InsertChar('\r') (via -insertText:) and propagate the Enter as a ui::KeyEvent. But if the Enter keyDown *didn't* result in a call to -insertText:, it gets eaten whenever there is marked text.

Yeah my current state is:
 macOS 10.13.6 (17G65) - macpro - no repro
 macOS 10.13.5 (17F77) - macbook pro - does repro

Haven't tested mojave yet (our offcorp test machine doesn't support it).

But it gets weirder. If I compile Chromium on the 10.13.5 machine, it does _not_ repro. So.. I can't even compile a test program to capture a trace of the messages we are getting from the IME engine.

restarting Chrome on the 10.13.5 machine had no effect - still repros.

So my current theory is that at some point in the 94 days that `uptime` says this mac has been running, the Korean IME engine got tricked into a state where it went into some kind of ~"compatibility" (or just buggy) mode that his somehow pinned to the Chrome (stable and canary) bundle IDs, but never pinned this buggy mode to the Chromium bundle ID.

I suspect rebooting the machine will resolve this issue. Temporarily perhaps. I tried a bunch of things on Chromium, but couldn't trick the IME engine to enter this weird mode.

Anyway.. I think #c4 will still work. But I'll need to make some educated guesses about the message sequence coming from the IME engine.

I think I have a fix for this issue: https://chromium-review.googlesource.com/c/chromium/src/+/1239955

terazawa/haowen : Note there is a change for Japanese already landed in Chrome beta (another,  Issue 883952 ), in case that's affecting you. We think the fix for that is correct, but if you're able to provide feedback on the change live in the beta channel, it would be greatly appreciated :). https://www.google.com/chrome/beta/?platform=mac

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

commit a0d8e67e49b985becf1a297795202acaf8e5881c
Author: Trent Apted <tapted@chromium.org>
Date: Wed Oct 10 22:18:58 2018

MacViews: Handle IME engines that -insertNewLine: when confirming compositions.

r593766 added logic to suppress accelerator key handling when an IME merely
dismisses its candidate window on a Return keypress. The suppression only
occurs when there is marked text.

This doesn't handle two cases:
 - IME that marks text, but has no candidate window to dismiss (e.g. Korean).
 - IME that does _not_ mark text (and has no candidate window), but re-sends
   the entire composition rather than a single character (e.g. Telex).

r593766 didn't regress these cases - it just did nothing to improve them.

These new IME cases both invoke -insertNewLine: at the end of their
-insertText:replacementRange: operations. Fortunately, the case fixed in
r593766 (_with_ a candidate window) that needs its keyEvent suppressed
does _not_ invoke -insertNewLine:.

So, to ensure we process accelerators whenever an IME (or something else) would
explicitly call -insertNewLine: (or insertTab:, etc.), we flag this, and
"resurrect" the keyEvent if it was suppressed (for any reason).

Change-Id: I7e5cf6e6b7bc9285a7af10e9a2eecadbfcc0ae6f
Bug:  890190 
Reviewed-on: https://chromium-review.googlesource.com/c/1239955
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598540}
[modify] https://crrev.com/a0d8e67e49b985becf1a297795202acaf8e5881c/ui/views/cocoa/bridged_native_widget_unittest.mm
[modify] https://crrev.com/a0d8e67e49b985becf1a297795202acaf8e5881c/ui/views_bridge_mac/bridged_content_view.h
[modify] https://crrev.com/a0d8e67e49b985becf1a297795202acaf8e5881c/ui/views_bridge_mac/bridged_content_view.mm

 Issue 894313  has been merged into this issue.

removing restrictions - I think this got auto-tagged due to the @google.com reporter, but the link in the description is a public URL and there's nothing sensitive here.

An external user helpfully helped verify the issue being fixed for Korean at  https://crbug.com/894313#c2  in Chrome 71.0.3577.0 canary

(I've still been unable to trick that same Canary running on my 10.13 systems into going into the 2-set Korean mode that underlines the active composition).