Bug 76690

Created attachment 123259 [details] Patch

Who should review this change?

(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 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.

Created attachment 123260 [details] Patch

(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 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?

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

Created attachment 123278 [details] Patch

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.

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.

rniwa, could you please take another look?

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.

Created attachment 123504 [details] Patch for landing

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 on attachment 123504 [details] Patch for landing Looks like the cq bot doesn't respond cq+. I'll land the patch by hand.

(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

(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!