Bug 49523 – IME candidate characters inserted unexpectedly to textarea which listens compositionupdate event

• Component: HTML Editing
• Status: NEW

[Kenichi Ishibashi]

Steps to repro:
1. Open the attached HTML file.
2. In the textarea, type a Japanese character sequence like "てすと" [tesuto] with IME enabled.

Expected result:
An uncommitted IME composition text "てすと" should appear and any other committed characters should not be inserted in the textarea.

Actual result:
Some committed characters appears with IME composition text (See the attached image).

[Kenichi Ishibashi]

Created attachment 73866 [details]
An HTML file to reproduce

[Kenichi Ishibashi]

Created attachment 73867 [details]
Actual result

[Kenichi Ishibashi]

The cause of this bug is almost the same as the bug 46868. A similar text mismatch could occur when the textarea listens the compositonupdate event and changes its style on the event handler. This bug could occur not only on Mac but on other platforms. I'll post a trial patch to fix this, but I'm not confident whether the fix is suitable. I'd like to ask some advice.

Thanks,

[Kenichi Ishibashi]

Created attachment 73868 [details]
Trial patch V0

[Alexey Proskuryakov]

This seems to be a patch for a Cromium-specific problem in cross platform code. Why is that necessary? What does Chromium do differently to trigger this problem? Could it be a bug in Chromium code?

I guess I have the same concern about bug 46868, where this new call to updateStyleIfNeeded() has been added.

[Kenichi Ishibashi]

Hi,

(In reply to comment #5)
> This seems to be a patch for a Cromium-specific problem in cross platform code. Why is that necessary? What does Chromium do differently to trigger this problem? Could it be a bug in Chromium code?

Whether this bug occurs depends on when the event handler is invoked. The primary cause of this bug is text mismatch of the textarea element and its render object. This mismatch will occur when the user agent invokes the event handler which changes style of the textarea before updating the composition text. Since the stable version of Safari invokes the event handler after updating composition text, this bug doesn't apper on it. However, IMHO, the text mismatch should not occur irrespective of the order of the process because there is no dependency between invoking event handlers that changes styles and updating composition text. So I think this bug is not only a Chromium specific issue but also a issue on other platforms. Actually, Webkit nightly builds (r71897 for Mac and r71499 for Windows) seem to have same problem. I tried to input some Japanese text with IME on the attached HTML page, the nightly builds inserted uncommitted composition text unexpectedly, even if the stable version of Safari doesn't do that.

Anyway, I'm not really sure about the fix. There might be better place to call updateStyleIfNeeded(). So please let me know if you have another solution.

Regards,

[Kenichi Ishibashi]

Could anyone take a look at this?

[Alexey Proskuryakov]

I'm not even sure if this is a bug (same about bug 46868). Changing style in a sync event handler is just an invitation for trouble.

The general direction has been to not dispatch synchronous event during DOM manipulation, and perhaps we just shouldn't dispatch compositionupdate synchronously (or at all).

[Ryosuke Niwa]

Comment on attachment 73868 [details]
Trial patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=73868&action=review

(In reply to comment #8)
> I'm not even sure if this is a bug (same about bug 46868). Changing style in a sync event handler is just an invitation for trouble.
> The general direction has been to not dispatch synchronous event during DOM manipulation, and perhaps we just shouldn't dispatch compositionupdate synchronously (or at all).

Unlike DOM mutation events which could fire in the middle of executing editing commands, IME composition events are fired per user actions.  
So I don't think it'll cause too much trouble for us even it fired synchronously just like keydown or mousedown although we must be extremely careful when to fire them.

> LayoutTests/platform/chromium/fast/text/chromium-duplicate-ime-composition.html:1
> +<html>

Why is this test under chromium?  I can reproduce the same issue on WebKit r72487.  Shouldn't we test it on all platforms?

> WebCore/editing/Editor.cpp:-1601
> -    // Updates styles before setting selection for composition to prevent
> -    // inserting the previous composition text into text nodes oddly.
> -    // See https://bugs.webkit.org/show_bug.cgi?id=46868
> -    m_frame->document()->updateStyleIfNeeded();
> -

I don't quite get why this change is correct.  Could you at least explain why you're removing this?  Because as far as I checked, we don't always call deleteTextFromNode when we call setComposition.  I want to know why moving this layout update as supposed to adding another one in replaceTextInNode is correct.

[Ryosuke Niwa]

Comment on attachment 73868 [details]
Trial patch V0

r- because I want to see the test being ran on all platforms given that the bug reproduced on mac port.

[Kenichi Ishibashi]

Hi Niwa-san,

Thank you for reviewing and sorry for late reply. I've checked this problem with ToT and I couldn't reproduce the problem on both Safari and Chromium. I cannot figure out which change fix the problem, but it might be good to withdraw the review for now.

Regards,

(In reply to comment #10)
> (From update of attachment 73868 [details])
> r- because I want to see the test being ran on all platforms given that the bug reproduced on mac port.

[Ryosuke Niwa]

(In reply to comment #11)
> Hi Niwa-san,
> 
> Thank you for reviewing and sorry for late reply. I've checked this problem with ToT and I couldn't reproduce the problem on both Safari and Chromium. I cannot figure out which change fix the problem, but it might be good to withdraw the review for now.

Could be https://bugs.webkit.org/show_bug.cgi?id=51693.  It'll be still nice to check in a test to make sure this doesn't regress in the future.

[Kenichi Ishibashi]

Hi Niwa-san,

(In reply to comment #12)
> Could be https://bugs.webkit.org/show_bug.cgi?id=51693.  It'll be still nice to check in a test to make sure this doesn't regress in the future.

Thank you for the information and suggestion. I agree with you adding a test for this problem, but unfortunately, DRT doesn't provide functions for testing this problem except for Chromium port for now. Would it be worth adding functions for this problem in other ports?

[Darin Adler]

(In reply to comment #13)
> I agree with you adding a test for this problem, but unfortunately, DRT doesn't provide functions for testing this problem except for Chromium port for now. Would it be worth adding functions for this problem in other ports?

Sure.

[Ryosuke Niwa]

(In reply to comment #13)
> Thank you for the information and suggestion. I agree with you adding a test for this problem, but unfortunately, DRT doesn't provide functions for testing this problem except for Chromium port for now. Would it be worth adding functions for this problem in other ports?

I agree with Darin that adding such features to DRT will be worthwhile.  Even if you could not implement it for all ports for now, implementing it for Mac & Windows ports will give us much greater coverage.

[Kenichi Ishibashi]

Hi,

I've tried to adding the feature for mac, but I can't come up with how to implement it. I tried to almost the same approach as chromium: introducing a method that sends keydown event with keycode = 0xE5 , then invokes editor()->setComposition() by calling [textInput setMarkedText:selectedRange:], into TextInputController. However, it doesn't work as expected. When I call the introduced method with a string, I expect that DRT replaces the text of the textarea to given string, but it just appends the string..

Is there any way to emulate IME input behavior on DRT mac (and Windows) port?

(In reply to comment #15)
> (In reply to comment #13)
> > Thank you for the information and suggestion. I agree with you adding a test for this problem, but unfortunately, DRT doesn't provide functions for testing this problem except for Chromium port for now. Would it be worth adding functions for this problem in other ports?
> 
> I agree with Darin that adding such features to DRT will be worthwhile.  Even if you could not implement it for all ports for now, implementing it for Mac & Windows ports will give us much greater coverage.

[Ryosuke Niwa]

(In reply to comment #16)
> I've tried to adding the feature for mac, but I can't come up with how to implement it. I tried to almost the same approach as chromium: introducing a method that sends keydown event with keycode = 0xE5 , then invokes editor()->setComposition() by calling [textInput setMarkedText:selectedRange:], into TextInputController. However, it doesn't work as expected. When I call the introduced method with a string, I expect that DRT replaces the text of the textarea to given string, but it just appends the string..
> 
> Is there any way to emulate IME input behavior on DRT mac (and Windows) port?

Enrica, Dan, Darin, or Justin: do you know the answer to this question?  If adding the said feature to DRT is too hard, it might make sense to land the current patch as is.  Although I still want explanation as to why moving updateLayout from one place to another fixes the bug.

[Kenichi Ishibashi]

Hi Niwa-san,

Thank you for taking care of this bug. 

(In reply to comment #17)
> Although I still want explanation as to why moving updateLayout from one place to another fixes the bug.

During investigating another chromium bug (http://crbug.com/66848), I realized that the essential cause of this bug is not IME related stuff. Under some particular situation, modifying style attribute in event handlers causes an inconsistency between m_value of HTMLTextAreaElement and the return value of RenderTextControl::text() even if HTMLTextAreaElement::formControlValueMachesRenderer() returns true. It is very complicate to explain why such inconsistency could occur. In short, the reason is not calling HTMLTextAreaElement::setFormControlValueMachesRenderer(false) immediately after inserting text into the element.

As far as I investigated, the reason why Safari doesn't have such problems seems that the order of calling TypingCommand::markMisspellingsAfterTyping() and Editor::appliedEditing() in TypingCommand::typingAddedToOpenCommand().

I'll file a bug for http://crbug.com/66848 to bugs.webkit.org and try to explain a little more minutely.

[Ryosuke Niwa]

(In reply to comment #18)
> During investigating another chromium bug (http://crbug.com/66848), I realized that the essential cause of this bug is not IME related stuff. Under some particular situation, modifying style attribute in event handlers causes an inconsistency between m_value of HTMLTextAreaElement and the return value of RenderTextControl::text() even if HTMLTextAreaElement::formControlValueMachesRenderer() returns true.

Ah, I see.  But doesn't that mean your initial approach is incorrect?  Do you think fixing the root cause will help resolve this bug as well?

> It is very complicate to explain why such inconsistency could occur. In short, the reason is not calling HTMLTextAreaElement::setFormControlValueMachesRenderer(false) immediately after inserting text into the element.

Oops! That sounds like a really bad bug.

> As far as I investigated, the reason why Safari doesn't have such problems seems that the order of calling TypingCommand::markMisspellingsAfterTyping() and Editor::appliedEditing() in TypingCommand::typingAddedToOpenCommand().

Ok.

> I'll file a bug for http://crbug.com/66848 to bugs.webkit.org and try to explain a little more minutely.

Looking forward to see it.

[Kenichi Ishibashi]

Hi All,

(In reply to comment #19)
> But doesn't that mean your initial approach is incorrect?  Do you think fixing the root cause will help resolve this bug as well?

I agree that fixing the root cause will help even this bug is no longer occurred. However, I don't think my initial approach is appropriate way to fix the problem now. I think we can fix the cause more suitable way.

> > As far as I investigated, the reason why Safari doesn't have such problems seems that the order of calling TypingCommand::markMisspellingsAfterTyping() and Editor::appliedEditing() in TypingCommand::typingAddedToOpenCommand().
> 
> Ok.

As for this, I'd like to ask Apple folks about the changeset 43419 (http://trac.webkit.org/changeset/43419). Could you give me the reason why this change is needed? As far as I tested, calling Editor::appliedEditing() before markMisspelingsAfterTyping() will fix the problem because appliedEditing() will promptly match the m_value of HTMLTextAreaElement and text value of its renderer. I think calling appliedEditing() before markMisspelingsAfterTyping() is simple and most suitable way to fix the problem.

Regards,

[Alexey Proskuryakov]

Sounds like Safari on 10.5 and on Windows should have the same issue as Chromium then.

Yes, I can tell you what the steps to reproduce were in more detail. By the way, Radar number is wrong in  r43419, it should be <rdar://problem/6864072>.

1. Open Mail or open Safari and login to GMail
2. Compose a new message
3. Type "This doesn't work."

Results: "doesn" is underlined as a misspelling.

I don't know why there is a PLATFORM(MAC) check, it seems wrong. The second code path that was added in r43419 was needed to fix a bug on Mac OS X 10.5.

[Kenichi Ishibashi]

Hi Alexey,

Thank you for your prompt response and sorry for late reply since I was off couple of days.

> Sounds like Safari on 10.5 and on Windows should have the same issue as Chromium then.

I tried reproduce the problem with Safari on Windows, but it didn't reproduce. It seems that there is another factor in the cause. I'll continue to investigate it.

> Yes, I can tell you what the steps to reproduce were in more detail. By the way, Radar number is wrong in  r43419, it should be <rdar://problem/6864072>.
> 
> 1. Open Mail or open Safari and login to GMail
> 2. Compose a new message
> 3. Type "This doesn't work."
> 
> Results: "doesn" is underlined as a misspelling.

I see.

> I don't know why there is a PLATFORM(MAC) check, it seems wrong. The second code path that was added in r43419 was needed to fix a bug on Mac OS X 10.5.

So I think removing PLATFORM(MAC) is better to fix the bug described at http://crbug.com/66848. I'd like to ask your opinion.

Thanks,

[Alexey Proskuryakov]

See also: bug 54975, which looks closely related to the history of this change.

[Jia Pu]

(In reply to comment #22)
> > I don't know why there is a PLATFORM(MAC) check, it seems wrong. The second code path that was added in r43419 was needed to fix a bug on Mac OS X 10.5.
> 
> So I think removing PLATFORM(MAC) is better to fix the bug described at http://crbug.com/66848. I'd like to ask your opinion.
> 
> Thanks,

(In reply to comment #23)
> See also: bug 54975, which looks closely related to the history of this change.

The change for bug 54975 is in a Mac specific function. I'm not sure how relevant it is to this bug.

[Alexey Proskuryakov]

Jia, this bug deals with effects of a fix for <rdar://problem/6864072>, "Contraction base marked as misspelled even though contraction is a word." I don't know if there is any direct relation, but the symptoms are definitely similar.

Regardless of whether there is any relation, your comments about this bug would be appreciated. In particular, I'm not sure about potential effects a fix could have on Safari for Windows, as well as on older Mac OS X versions.

[Jia Pu]

(In reply to comment #25)
> Jia, this bug deals with effects of a fix for <rdar://problem/6864072>, "Contraction base marked as misspelled even though contraction is a word." I don't know if there is any direct relation, but the symptoms are definitely similar.
> 
> Regardless of whether there is any relation, your comments about this bug would be appreciated. In particular, I'm not sure about potential effects a fix could have on Safari for Windows, as well as on older Mac OS X versions.

I added Doug to the CC list. He's the author of changeset 43419, and may want to add my comment below.

We'd recommend to use caution when removing "PLATFORM(MAC)". Looking at changeset 43419 alone can be misleading. Prior to changeset 42911 (http://trac.webkit.org/changeset/42911/trunk/WebCore/editing/TypingCommand.cpp), markMisspellingsAfterTyping() is called before appliedEditing() on all platforms. During Snow Leopard development, the order of these two calls are switched in changeset 42911. Upon realizing that the switching has caused regression, the preprocessor was added for Snow Leopard. In other words, appliedEditing() is called before markMisspellingAfterTyping() only on Snow Leopard. So what changeset 43419 does is simply making sure the behavior on non-SL platforms is what is used to be.

Given this history, it seems removing "PLATFORM(MAC)" will not change the order of these two function calls on older Mac OS X versions. But it will change the behavior on other platforms, including Safari for Windows. And the new behavior hasn't been exercised on those platforms before.

[Kenichi Ishibashi]

Filed a bug for http://crbug.com/66848

https://bugs.webkit.org/show_bug.cgi?id=55133