A quick fix for Issue 19421....

A quick fix for Issue 19421.
This issue is caused by AutocompleteEditViewMac::SetText() that updates the value of an AutocompleteEditField instance while an input method is composing text. Same as Windows, NSTextView finishes an ongoing composition when we update the value of an AutocompleteEditField object.
To fix this issue, we check whether or not NSTextView has marked text and exit if it has.

BUG=19421
"Korean IME does not work in the omnibox"
TEST=Select Korean 2-set keyboard, type 'g', 'k', 's', 'r', 'm', 'f' keys, and verify we can see two Korean syllables.

+rohitrao for reviewers since shess is out for the week

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23702

[John Grabowski, 2009-08-18 20:08:18]

Rohit: please review for Scott.

[rohitrao (ping after 24h), 2009-08-18 21:42:35]

LGTM

http://codereview.chromium.org/171103/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/171103/diff/1/2#newcode365
Line 365: // Exit if an input method is composing text.
Is this guaranteed to be called again once the text is unmarked?

[Hironori Bono, 2009-08-19 05:23:38]

Thank you for your review and comment.

http://codereview.chromium.org/171103/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/171103/diff/1/2#newcode365
Line 365: // Exit if an input method is composing text.
On 2009/08/18 21:42:35, rohitrao wrote:
> Is this guaranteed to be called again once the text is unmarked?

Yes. This function is called when this NSTextField control receives a
controlTextDidChange notification. Also, when the text is unmarked, Cocoa sends
a controlTextDidChange notification to this NSTextField control.
Thus, this function is called when the text is unmarked.

[Scott Hess - ex-Googler, 2009-08-23 14:10:53]

Overriding SetText() is pretty deep - would it be possible to just
catch it in the did-change handler and short-circuit it there?

-scott


On Tue, Aug 18, 2009 at 10:23 PM, <hbono@chromium.org> wrote:
> Thank you for your review and comment.
>
>
> http://codereview.chromium.org/171103/diff/1/2
> File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):
>
> http://codereview.chromium.org/171103/diff/1/2#newcode365
> Line 365: // Exit if an input method is composing text.
> On 2009/08/18 21:42:35, rohitrao wrote:
>>
>> Is this guaranteed to be called again once the text is unmarked?
>
> Yes. This function is called when this NSTextField control receives a
> controlTextDidChange notification. Also, when the text is unmarked,
> Cocoa sends a controlTextDidChange notification to this NSTextField
> control.
> Thus, this function is called when the text is unmarked.
>
> http://codereview.chromium.org/171103
>

[Hironori Bono, 2009-08-25 03:08:00]

Hi Scott,

Thank you for your comment and sorry for my slow response.
If we move this check to controlTextDidChange, we cannot block SetText() calls
from AutocompleteEditModel functions. In fact, this issue once happened also on
Windows. At this time, we added the same sort of check in
SetWindowTextAndCaretPos(). So, my first change added this check to
SetWindowTextAndCaretPos() as I did for Windows. Unfortunately, this first
change didn't fix this issue since EmphasizeURLComponents() also calls SetText()
while an input method is composing a text. To avoid adding the same check two
places, I added it in SetText().
Nevertheless, if you don't like adding code to SetText(), I'm going to send
another change that creates a function IMIsComposing() and call the function is
controlTextDidChange and SetWindowTextAndCaretPos(). Would it be possible to
give me your thoughts?

Regards,

Hironori Bono

On 2009/08/23 14:10:53, shess wrote:
> Overriding SetText() is pretty deep - would it be possible to just
> catch it in the did-change handler and short-circuit it there?
> 
> -scott
> 
> 
> On Tue, Aug 18, 2009 at 10:23 PM, <mailto:hbono@chromium.org> wrote:
> > Thank you for your review and comment.
> >
> >
> > http://codereview.chromium.org/171103/diff/1/2
> > File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):
> >
> > http://codereview.chromium.org/171103/diff/1/2#newcode365
> > Line 365: // Exit if an input method is composing text.
> > On 2009/08/18 21:42:35, rohitrao wrote:
> >>
> >> Is this guaranteed to be called again once the text is unmarked?
> >
> > Yes. This function is called when this NSTextField control receives a
> > controlTextDidChange notification. Also, when the text is unmarked,
> > Cocoa sends a controlTextDidChange notification to this NSTextField
> > control.
> > Thus, this function is called when the text is unmarked.
> >
> > http://codereview.chromium.org/171103
> >

[Scott Hess - ex-Googler, 2009-08-25 05:31:23]

My concern is that SetText() is being called for a reason - are you
positive that in every possible case, it shouldn't interrupt an
in-process composition?  For instance, what if you start composing a
character and select the "Back" function?  What happens if you click
away to a different tab?  Interrupting the call in
controlTextDidChange is great because it means that we short-circuit
the entire problem, I wouldn't be surprised if there weren't other
problematic entry points, but they should be enumerable.  Even if we
end up with the change in SetText(), knowing the cases that need
testing is useful.

Actually, a quick test indicates that your patch definitely breaks
tab-switching.

Can I assume that the composition for CJK languages will work like
Latin languages?  For instance, Option-` then a gives an a with an
accent, and if that works, everything works?

Thanks,
scott


On Mon, Aug 24, 2009 at 8:08 PM, <hbono@chromium.org> wrote:
> Hi Scott,
>
> Thank you for your comment and sorry for my slow response.
> If we move this check to controlTextDidChange, we cannot block SetText()
> calls from AutocompleteEditModel functions. In fact, this issue once
> happened also on Windows. At this time, we added the same sort of check
> in SetWindowTextAndCaretPos(). So, my first change added this check to
> SetWindowTextAndCaretPos() as I did for Windows. Unfortunately, this
> first change didn't fix this issue since EmphasizeURLComponents() also
> calls SetText() while an input method is composing a text. To avoid
> adding the same check two places, I added it in SetText().
> Nevertheless, if you don't like adding code to SetText(), I'm going to
> send another change that creates a function IMIsComposing() and call the
> function is controlTextDidChange and SetWindowTextAndCaretPos(). Would
> it be possible to give me your thoughts?
>
> Regards,
>
> Hironori Bono
>
> On 2009/08/23 14:10:53, shess wrote:
>>
>> Overriding SetText() is pretty deep - would it be possible to just
>> catch it in the did-change handler and short-circuit it there?
>
>> -scott
>
>
>> On Tue, Aug 18, 2009 at 10:23 PM, <mailto:hbono@chromium.org> wrote:
>> > Thank you for your review and comment.
>> >
>> >
>> > http://codereview.chromium.org/171103/diff/1/2
>> > File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm
>
> (right):
>>
>> >
>> > http://codereview.chromium.org/171103/diff/1/2#newcode365
>> > Line 365: // Exit if an input method is composing text.
>> > On 2009/08/18 21:42:35, rohitrao wrote:
>> >>
>> >> Is this guaranteed to be called again once the text is unmarked?
>> >
>> > Yes. This function is called when this NSTextField control receives
>
> a
>>
>> > controlTextDidChange notification. Also, when the text is unmarked,
>> > Cocoa sends a controlTextDidChange notification to this NSTextField
>> > control.
>> > Thus, this function is called when the text is unmarked.
>> >
>> > http://codereview.chromium.org/171103
>> >
>
>
>
> http://codereview.chromium.org/171103
>

[Hironori Bono, 2009-08-25 06:47:49]

Hi Scott,

Thank you for your comments.
I'm very sorry if my last comment made you feel bad. My last comment just
described steps toward implementing this change, and I don't have any objections
against moving this check to controlTextDidChange. (I realize I need a lot of
study about this class and Mac.)

By the way, would it be possible to give me your thoughts about adding this
check to SetWindowTextAndCaretPos() to block AutocompleteEditMode from changing
text while it is composing?

On 2009/08/25 05:31:23, shess wrote:
> My concern is that SetText() is being called for a reason - are you
> positive that in every possible case, it shouldn't interrupt an
> in-process composition?  For instance, what if you start composing a
> character and select the "Back" function?  What happens if you click
> away to a different tab?  Interrupting the call in
> controlTextDidChange is great because it means that we short-circuit
> the entire problem, I wouldn't be surprised if there weren't other
> problematic entry points, but they should be enumerable.  Even if we
> end up with the change in SetText(), knowing the cases that need
> testing is useful.
> 
> Actually, a quick test indicates that your patch definitely breaks
> tab-switching.

Thank you for noticing this.
This regression is definitely bad. I'm going to revert this change and send
another change for this issue.

> Can I assume that the composition for CJK languages will work like
> Latin languages?  For instance, Option-` then a gives an a with an
> accent, and if that works, everything works?

Yes and no. NSTextInput is used not only for inputting CJK characters, but also
used for inputting accented characters used in many European languages. So, it
should work for all languages if Option-` works. On the other hand, composing
CJK characters is much slower than composing accented characters. So, if an
issue is a timing issue, it may not work for all languages even though Option-`
works.

Regards,

Hironori Bono

[Scott Hess - ex-Googler, 2009-09-03 21:38:10]

Sorry about the delayed response.

My concern about overriding SetText() or SetWindowTextAndCaretPos() is
that I think they are internal functions which should not change
operation based on UI state.  Doing it that way assumes that the new
content is somehow related to the content currently in the field,
which is not a valid assumption in all cases.  If the user is in the
middle of composing a character and clicks a menu entry which would
change the field to a completely different value, then that value
should be accepted.  Basically, I think this kind of thing needs to be
handled entirely in the Cocoa code, if there is a change which is not
a real change, then it should be short-circuited.

http://crbug.com/12556 is a similar kind of bug.  I think someone told
me that Safari does not maintain the marked text when you switch tabs
and back.  Another place it might matter is if you mark some text and
then arrow down into the popup - should the marked text return when
you arrow back up?  I could see an argument for removing the marked
text in both of these cases, because composing characters is
implicitly pretty short-term (if you start composing, go to another
tab to read email for awhile, then come back, picking up where you
left off might be kinda weird).

I don't think this is going to be easily resolved.  Would you might
working up a change which short-circuits in -controlTextDidChange:,
and see how well that works to fix this specific bug?  Feel free to
drop in a TODO-type comment refering to this thread.  That should make
things work better so that we can find the next bug or two related to
this, maybe there's a clear solution (or maybe we need to push
first-class support into the autocomplete mvc code, rather than doing
it ad hoc like this).

-scott


On Mon, Aug 24, 2009 at 11:47 PM, <hbono@chromium.org> wrote:
> Hi Scott,
>
> Thank you for your comments.
> I'm very sorry if my last comment made you feel bad. My last comment
> just described steps toward implementing this change, and I don't have
> any objections against moving this check to controlTextDidChange. (I
> realize I need a lot of study about this class and Mac.)
>
> By the way, would it be possible to give me your thoughts about adding
> this check to SetWindowTextAndCaretPos() to block AutocompleteEditMode
> from changing text while it is composing?
>
> On 2009/08/25 05:31:23, shess wrote:
>>
>> My concern is that SetText() is being called for a reason - are you
>> positive that in every possible case, it shouldn't interrupt an
>> in-process composition? =A0For instance, what if you start composing a
>> character and select the "Back" function? =A0What happens if you click
>> away to a different tab? =A0Interrupting the call in
>> controlTextDidChange is great because it means that we short-circuit
>> the entire problem, I wouldn't be surprised if there weren't other
>> problematic entry points, but they should be enumerable. =A0Even if we
>> end up with the change in SetText(), knowing the cases that need
>> testing is useful.
>
>> Actually, a quick test indicates that your patch definitely breaks
>> tab-switching.
>
> Thank you for noticing this.
> This regression is definitely bad. I'm going to revert this change and
> send another change for this issue.
>
>> Can I assume that the composition for CJK languages will work like
>> Latin languages? =A0For instance, Option-` then a gives an a with an
>> accent, and if that works, everything works?
>
> Yes and no. NSTextInput is used not only for inputting CJK characters,
> but also used for inputting accented characters used in many European
> languages. So, it should work for all languages if Option-` works. On
> the other hand, composing CJK characters is much slower than composing
> accented characters. So, if an issue is a timing issue, it may not work
> for all languages even though Option-` works.
>
> Regards,
>
> Hironori Bono
>
> http://codereview.chromium.org/171103
>

[Hironori Bono, 2009-09-04 11:27:27]

Thank you for your comments and suggestions.
I have reverted this change to investigate the better places to check if there
is a marked text since it is not so easy to solve this issue without regressions
on Mac as I did for Windows.
I will send another review request when I find the better solutions.

Regards,

Hironori Bono

[Scott Hess - ex-Googler, 2009-09-08 19:42:38]

Again, is there anything wrong with adding the following to the
beginning of -controlTextDidChange:?

- (void)controlTextDidChange:(NSNotification*)aNotification {
  // Don't autocomplete when the user is composing a character.
  NSText* editor = [[aNotification userInfo] objectForKey:@"NSFieldEditor"];
  if ([editor hasMarkedText]) {
    return;
  }
...

While this might not solve every possible situation where this problem
comes up, it does solve the case in question, so the user experience
is improved while other changes are being considered.

-scott



On Fri, Sep 4, 2009 at 4:27 AM, <hbono@chromium.org> wrote:
> Thank you for your comments and suggestions.
> I have reverted this change to investigate the better places to check if
> there is a marked text since it is not so easy to solve this issue
> without regressions on Mac as I did for Windows.
> I will send another review request when I find the better solutions.
>
> Regards,
>
> Hironori Bono
>
> http://codereview.chromium.org/171103
>