Bug 31902 – WebKit modifies the DOM after the compositionend event

• Component: HTML Editing
• Status: NEW

[Daniel Danilatos]

All DOM changes resulting from IME composition should be strictly bounded between compositionstart and compositionend events

Please see here, using either Linux or Windows IME:
http://www.danilatos.com/event-test/ExperimentTest.html
Use any Webkit that supports composition events, e.g. Chrome 4 or Webkit after http://trac.webkit.org/changeset/50968/trunk
(Note, I haven't updated the test page to show the compositionupdate event yet, but this is not particularly relevant).

Also note that, if the composition is not cancelled, i.e. if there is to be a textInput event as well, then we get a 2nd compositionstart/update/end cycle after the normal one, where the content is basically deleted and re-inserted. I think this is bad too (but webkit has been doing this for a while).

A few possible ways to fix:
a) Simply ensure compositionend gets fired after the last dom change (don't fire it and then change the dom once more).
b) Same as (a), but to avoid the 2nd round of composition events, have textInput come before compositionend. That way when textInput is triggered, the content has been removed so textInput is still a preview of what is about to be inserted (if that is in fact desirable, I'm guessing that would be why webkit deletes then re-inserts the content)
c) Same as (a), but to avoid the 2nd round of composition events, *and* still have textInput fire after compositionend, stop the deleting and re-inserting of content. Instead, textInput is just informational and happens after-the-fact (thus cancelling it has no effect).

b) is preferred by Hironori (cc'd) and me, but contradicts the current DOM Level 3 spec, which says textInput must come after compositionend. That said, hbono and I are involved with that part of the spec (and he drafted it), so based on our implementation experience it could very well be worth trying to change the spec while it's still fresh.

This issue is also tracked at:
http://code.google.com/p/chromium/issues/detail?id=28246

[James Su]

(In reply to comment #0)
> All DOM changes resulting from IME composition should be strictly bounded
> between compositionstart and compositionend events
How the DOM will be changed by a compositionend event? For now, a compositionend event will only update the composition text (the text being selected). If the following textInput event is cancelled, the composition text will be left unconfirmed. See chrome issue: http://crbug.com/30982 for reference.

The current spec states that composition events can be cancelled. Are you plan to modify the spec to make them non-cancellable? IMHO the semantic of cancelling these events is confusing and may not be possible to be implemented.

> 
> Please see here, using either Linux or Windows IME:
> http://www.danilatos.com/event-test/ExperimentTest.html
> Use any Webkit that supports composition events, e.g. Chrome 4 or Webkit after
> http://trac.webkit.org/changeset/50968/trunk
> (Note, I haven't updated the test page to show the compositionupdate event yet,
> but this is not particularly relevant).
> 
> Also note that, if the composition is not cancelled, i.e. if there is to be a
> textInput event as well, then we get a 2nd compositionstart/update/end cycle
> after the normal one, where the content is basically deleted and re-inserted. I
> think this is bad too (but webkit has been doing this for a while).
I tested it on Chrome Linux with SCIM input method, but didn't encounter the 2nd composition cycle issue. See following sample event sequence of inputting "啊" with Chinese pinyin input method: (D=keydown, U=keyup, CS = compositionstart, and so on.)

D 229 0 false false false
CS 啊
U 65 0 false false false
D 229 0 false false false
CU 啊
CE 啊
T 啊
U 32 0 false false false

So I believe the 2nd cycle issue is caused by the input method, rather than the webkit.

And following is the sequence of cancelling a composition:

D 229 0 false false false
CS 啊
U 65 0 false false false
D 229 0 false false false
CE 
U 27 0 false false false

In this case, webkit doesn't fire compositionupdate event before the last compositionend event, which is different than the behavior described in the current spec: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-compositionupdate


> 
> A few possible ways to fix:
> a) Simply ensure compositionend gets fired after the last dom change (don't
> fire it and then change the dom once more).
We need to clarify how the DOM will be changed by compositionend.

> b) Same as (a), but to avoid the 2nd round of composition events, have
> textInput come before compositionend. That way when textInput is triggered, the
> content has been removed so textInput is still a preview of what is about to be
> inserted (if that is in fact desirable, I'm guessing that would be why webkit
> deletes then re-inserts the content)
Semantically, a textInput event is actually a result of a confirm composition action (compositionend with a non-empty text), so it would be confusing if textInput events are fired after compositionend events.

> c) Same as (a), but to avoid the 2nd round of composition events, *and* still
> have textInput fire after compositionend, stop the deleting and re-inserting of
> content. Instead, textInput is just informational and happens after-the-fact
> (thus cancelling it has no effect).
The spec states that the textInput event can be cancelled. And it may be useful. See chrome issue: http://crbug.com/30982 for reference, which is actually a webkit issue.

> 
> b) is preferred by Hironori (cc'd) and me, but contradicts the current DOM
> Level 3 spec, which says textInput must come after compositionend. That said,
> hbono and I are involved with that part of the spec (and he drafted it), so
> based on our implementation experience it could very well be worth trying to
> change the spec while it's still fresh.
> 
> This issue is also tracked at:
> http://code.google.com/p/chromium/issues/detail?id=28246

[Hironori Bono]

Created attachment 52618 [details]
A proposed fix (including a layout-test change)

Thank you for your bug report and sorry for my slow response.
This issue is caused by a race condition that updates a composition node after sending a composiionend event. Unfortunately, this change needs to change my previous layout test because it assumes updating a composition node before sending a compositionend event.

Regards,

Hironori Bono

[Hironori Bono]

I would like to cancel this review request because the ChangeLog files in this patch did not describe this issue correctly. I will fix the descriptions and send the updated one tomorrow.

Regards,

Hironori Bono

(In reply to comment #2)
> Created an attachment (id=52618) [details]
> A proposed fix (including a layout-test change)
> 
> Thank you for your bug report and sorry for my slow response.
> This issue is caused by a race condition that updates a composition node after
> sending a composiionend event. Unfortunately, this change needs to change my
> previous layout test because it assumes updating a composition node before
> sending a compositionend event.
> 
> Regards,
> 
> Hironori Bono

[Hironori Bono]

Created attachment 52705 [details]
The second proposed fix (including a layout-test change)

Greetings webkit-reviewers,

I have updated my patch to fix descriptions. Would it be possible to review it and give me your comments?
Thank you for your help in advance.

Regards,

Hironori Bono

[Hironori Bono]

Greetings,

Unfortunately, this change seems to be desolated. It is definitely helpful for someone to review this change.

Regards,

Hironori Bono

(In reply to comment #4)
> Created an attachment (id=52705) [details]
> The second proposed fix (including a layout-test change)
> 
> Greetings webkit-reviewers,
> 
> I have updated my patch to fix descriptions. Would it be possible to review it
> and give me your comments?
> Thank you for your help in advance.
> 
> Regards,
> 
> Hironori Bono

[Darin Adler]

Alexey, Mitz, any comments on this patch?

[Alexey Proskuryakov]

The bug description mentioned that the spec can be changed. Is the patch expected to implement what the current editor's draft says? I see that compositionend is no longer cancelable there - but its default action is to fire textInput, which this bug disagrees with.

Is there proposed spec text that describes the behavior that this patch implements? It needn't be hosted by W3C, but a coherent description that can be reviewed on its own would help. Currently, the patch itself is the only documentation of proposed behavior, so it's hard to review.

[Hironori Bono]

Thank you for your comments.

(In reply to comment #7)
> The bug description mentioned that the spec can be changed. Is the patch
> expected to implement what the current editor's draft says? I see that
> compositionend is no longer cancelable there - but its default action is to
> fire textInput, which this bug disagrees with.

As an editor of the spec, this behavior is not compliant with the spec as of 26 April, 2010. (This is exactly the reason why I cannot push this change.)

> Is there proposed spec text that describes the behavior that this patch
> implements? It needn't be hosted by W3C, but a coherent description that can be
> reviewed on its own would help. Currently, the patch itself is the only
> documentation of proposed behavior, so it's hard to review.

This comment is totally right. It is better for us to work for the spec before changing WebKit.

Sorry for disturbing you with my spam change, and regards,

Hironori Bono

[Alexey Proskuryakov]

Comment on attachment 52705 [details]
The second proposed fix (including a layout-test change)

OK - clearing the review flag for now per the above comment.