Bug 76690 - WebKit fails IETC composition event types
: WebKit fails IETC composition event types
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://samples.msdn.microsoft.com/iet...
:
:
: 76198
  Show dependency treegraph
 
Reported: 2012-01-20 00:15 PST by
Modified: 2012-01-23 16:00 PST (History)


Attachments
Patch (4.36 KB, patch)
2012-01-20 00:19 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.47 KB, patch)
2012-01-20 00:27 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2012-01-20 03:00 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (5.16 KB, patch)
2012-01-22 17:49 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-20 00:15:53 PST
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.
------- Comment #1 From 2012-01-20 00:19:12 PST -------
Created an attachment (id=123259) [details]
Patch
------- Comment #2 From 2012-01-20 00:21:13 PST -------
Who should review this change?
------- Comment #3 From 2012-01-20 00:23:12 PST -------
(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.
------- Comment #4 From 2012-01-20 00:24:37 PST -------
(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.
------- Comment #5 From 2012-01-20 00:27:19 PST -------
Created an attachment (id=123260) [details]
Patch
------- Comment #6 From 2012-01-20 00:28:21 PST -------
(In reply to comment #4)
> (From update of attachment 123259 [details] [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
------- Comment #7 From 2012-01-20 00:39:10 PST -------
(From update of attachment 123260 [details])
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?
------- Comment #8 From 2012-01-20 00:43:00 PST -------
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
------- Comment #9 From 2012-01-20 03:00:59 PST -------
Created an attachment (id=123278) [details]
Patch
------- Comment #10 From 2012-01-20 03:04:15 PST -------
(From update of attachment 123260 [details])
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.
------- Comment #11 From 2012-01-20 03:05:53 PST -------
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.
------- Comment #12 From 2012-01-22 17:15:45 PST -------
rniwa, could you please take another look?
------- Comment #13 From 2012-01-22 17:37:49 PST -------
(From update of attachment 123278 [details])
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.
------- Comment #14 From 2012-01-22 17:49:01 PST -------
Created an attachment (id=123504) [details]
Patch for landing
------- Comment #15 From 2012-01-22 17:49:49 PST -------
(From update of attachment 123278 [details])
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.
------- Comment #16 From 2012-01-23 02:11:59 PST -------
(From update of attachment 123504 [details])
Looks like the cq bot doesn't respond cq+. I'll land the patch by hand.
------- Comment #17 From 2012-01-23 03:52:32 PST -------
(In reply to comment #16)
> (From update of attachment 123504 [details] [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
------- Comment #18 From 2012-01-23 16:00:28 PST -------
(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!