Bug 76690 – WebKit fails IETC composition event types

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Kenichi Ishibashi]

http://samples.msdn.microsoft.com/ietestcenter/domevents/domevents_harness.htm?url=CompositionEvent.html

The test requires one compositionupdate event, but WebKit doesn't dispatch it. The spec says:

> a composition session consists of one compositionstart event, one or more compositionupdate events, and one compositionend event

In this test, IE and Firefox dispatch composition events like:
1: compositionstart (data: empty)
2: compositionupdate (data: あ)
3: compositionend (data: あ)

I'll send a patch to follow Firefox and IE's behavior. I'm not sure we should follow their behavior, though.

[Kenichi Ishibashi]

Created attachment 123259 [details]
Patch

[Adam Barth]

Who should review this change?

[Kenichi Ishibashi]

(In reply to comment #2)
> Who should review this change?

Bono-san is the original author of the code and I think rniwa can review it.
As I mentioned above, I'm not sure we should do this.

[Ryosuke Niwa]

Comment on attachment 123259 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Dispatches at least one compositionupdate event.
> +        The spec says that a composition session includes one or more compositionupdate event(s).

Where is the spec? Please include it in the change log entry.

[Kenichi Ishibashi]

Created attachment 123260 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #4)
> (From update of attachment 123259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123259&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Dispatches at least one compositionupdate event.
> > +        The spec says that a composition session includes one or more compositionupdate event(s).
> 
> Where is the spec? Please include it in the change log entry.

Sorry, added the spec URL(*) to ChangeLog.

(*)http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-compositionevents

[Ryosuke Niwa]

Comment on attachment 123260 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:1469
>          if (!m_compositionNode) {

You should update the comment above.

> Source/WebCore/editing/Editor.cpp:1475
> +                // Dispatch two event: a compositionstart event with empty text,
> +                // then a compositionupdate event with the given text.
> +                // This ensures that at least one composition event is dispatched.

It seems dangerous to set data empty here given that we've always included text in data. Since the spec allows current WebKit behavior, I'd prefer keeping data=text for now:
"Some implemenations may populate the data attribute of the compositionstart event with the text currently selected in the document (for editing and replacement); otherwise, the value of the data attribute must be the empty string."

Or did we very recently add a support for compositionstart event?

[Hironori Bono]

Greetings,

Thanks for your bug report.
This issue is caused by a change of the spec has after I implemented this event. (I do not have clear ideas about the time when we should synchronize our implementation with the spec, though.) To synchronize it with the spec now, I would recommend to add the 'locale' attribute added in the spec.

Regards,

Hironori Bono

[Kenichi Ishibashi]

Created attachment 123278 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 123260 [details]
Patch

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

Thank you for review!

>> Source/WebCore/editing/Editor.cpp:1469
>>          if (!m_compositionNode) {
> 
> You should update the comment above.

Done. Also moved the below comment to here.

>> Source/WebCore/editing/Editor.cpp:1475
>> +                // This ensures that at least one composition event is dispatched.
> 
> It seems dangerous to set data empty here given that we've always included text in data. Since the spec allows current WebKit behavior, I'd prefer keeping data=text for now:
> "Some implemenations may populate the data attribute of the compositionstart event with the text currently selected in the document (for editing and replacement); otherwise, the value of the data attribute must be the empty string."
> 
> Or did we very recently add a support for compositionstart event?

I see. Changed to include the text in data attribute of compositionstart event.

[Kenichi Ishibashi]

Hi Bono-san,
 
> This issue is caused by a change of the spec has after I implemented this event. (I do not have clear ideas about the time when we should synchronize our implementation with the spec, though.) To synchronize it with the spec now, I would recommend to add the 'locale' attribute added in the spec.

Thank you for your comment. I'll create a separate bug entry for adding 'locale' attribute.

[Kenichi Ishibashi]

rniwa, could you please take another look?

[Ryosuke Niwa]

Comment on attachment 123278 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Other major browsers (Firefox and IE) don't populate the data attribute  of the

Nit: two spaces between "attribute" and "of"

> Source/WebCore/ChangeLog:11
> +        compositionstart event, but we set the given text in the data attribute  to minimize the effect of this change.

Ditto. Also, isn't this line too long?

> Source/WebCore/editing/Editor.cpp:1460
> +        //    Send a compositionstart and a compositionupdate event when this function creates a new composition node, i.e.

Looking at other lines, you should probably wrap the line earlier.

[Kenichi Ishibashi]

Created attachment 123504 [details]
Patch for landing

[Kenichi Ishibashi]

Comment on attachment 123278 [details]
Patch

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

Thank you for review!

>> Source/WebCore/ChangeLog:10
>> +        Other major browsers (Firefox and IE) don't populate the data attribute  of the
> 
> Nit: two spaces between "attribute" and "of"

Done.

>> Source/WebCore/ChangeLog:11
>> +        compositionstart event, but we set the given text in the data attribute  to minimize the effect of this change.
> 
> Ditto. Also, isn't this line too long?

Done.

>> Source/WebCore/editing/Editor.cpp:1460
>> +        //    Send a compositionstart and a compositionupdate event when this function creates a new composition node, i.e.
> 
> Looking at other lines, you should probably wrap the line earlier.

Done.

[Kenichi Ishibashi]

Comment on attachment 123504 [details]
Patch for landing

Looks like the cq bot doesn't respond cq+. I'll land the patch by hand.

[Csaba Osztrogonác]

(In reply to comment #16)
> (From update of attachment 123504 [details])
> Looks like the cq bot doesn't respond cq+. I'll land the patch by hand.

Landed in http://trac.webkit.org/changeset/105605 and I landed an expected file update in http://trac.webkit.org/changeset/105609

[Kenichi Ishibashi]

(In reply to comment #17)
> Landed in http://trac.webkit.org/changeset/105605 and I landed an expected file update in http://trac.webkit.org/changeset/105609

I should have updated the expectation. Thanks!