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

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 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 Kenichi Ishibashi 2012-01-20 00:19:12 PST
Created attachment 123259 [details]
Patch
Comment 2 Adam Barth 2012-01-20 00:21:13 PST
Who should review this change?
Comment 3 Kenichi Ishibashi 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 Ryosuke Niwa 2012-01-20 00:24:37 PST
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.
Comment 5 Kenichi Ishibashi 2012-01-20 00:27:19 PST
Created attachment 123260 [details]
Patch
Comment 6 Kenichi Ishibashi 2012-01-20 00:28:21 PST
(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
Comment 7 Ryosuke Niwa 2012-01-20 00:39:10 PST
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?
Comment 8 Hironori Bono 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 Kenichi Ishibashi 2012-01-20 03:00:59 PST
Created attachment 123278 [details]
Patch
Comment 10 Kenichi Ishibashi 2012-01-20 03:04:15 PST
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.
Comment 11 Kenichi Ishibashi 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 Kenichi Ishibashi 2012-01-22 17:15:45 PST
rniwa, could you please take another look?
Comment 13 Ryosuke Niwa 2012-01-22 17:37:49 PST
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.
Comment 14 Kenichi Ishibashi 2012-01-22 17:49:01 PST
Created attachment 123504 [details]
Patch for landing
Comment 15 Kenichi Ishibashi 2012-01-22 17:49:49 PST
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.
Comment 16 Kenichi Ishibashi 2012-01-23 02:11:59 PST
Comment on attachment 123504 [details]
Patch for landing

Looks like the cq bot doesn't respond cq+. I'll land the patch by hand.
Comment 17 Csaba Osztrogonác 2012-01-23 03:52:32 PST
(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
Comment 18 Kenichi Ishibashi 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!