Bug 26310 – Webkit should generate "compositionstart", "compositionend", and "text" events like firefox

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

[Daniel Danilatos]

For IME input, firefox generates compositionstart and compositionend events to signify the start and end of a sequence of IME input. Additionally, for each individual change in the dom, it generates a "text" event.

It should be possible to make arbitrary changes to the dom, and move the caret, during compositionstart. The IME input should then take place in the location where the caret has been placed.

This all works on firefox, tested 3.0, 3.5, 3.6

I understand that this will still have some issues relating to recomposition of text, but since recomposition isn't even implemented yet, at least this will allow solving 99%-100% of current problems that exist regarding IMEs and unexpected DOM modifications.

To illustrate the events, please use http://www.danilatos.com/event-test/ExperimentTest.html

To illustrate the caret movement, please use http://www.danilatos.com/event-test/ime-test.html
Instructions: 
1. Select action "Split at cursor, move cursor into span.", and tick when "compositionstart". 
2. Switch to an IME (e.g. Chinese pinyin, or tablet input, or whatever). 
3. Click somewhere in the red editable area, and begin typing. The IME input should successfully get transported into the middle of some green coloured text.

In Firefox 3.0+, this works great, both for "compositionstart" and for "key event". In Safari/Chrome, the DOM changes first, THEN the green text comes up (only for "key event" since there is no composition start in webkit), then the cursor gets moved, and the IME input state is broken (e.g. the underline for chinese input vanishes, as the cursor has moved after the input began, not before).

[Hironori Bono]

Created attachment 42203 [details]
a work-in-progress patch

Even though this change works good on my Macbook, I'm not sure it works on Windows or Linux.
(That is, this change is not ready for review.)
So, I just would like to hear opinions.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 42736 [details]
A proposed patch (only for Mac)

Sorry for my slow updates.
Even though I tried setting up build environments for WebKit GTK, WebKit QT, and WebKit Win,  I wasn't able to set up build them.
So, I enclosed some part of my previous change with #if PLATFORM(MAC) and #endif to avoid possible build breaks occurred by the lack of the new files.
Is it possible to review this change and give me your comments?

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 42744 [details]
A proposed patch (only for Mac)

[Hironori Bono]

Created attachment 42745 [details]
A proposed patch 2 (only for Mac)

Sorry. I attached a wrong patch. (The previous one may send a compositionstart event even when m_compositionNode == 0 && text.isEmpty(), when setComposition() doesn't create a composition node.)

[Oliver Hunt]

Comment on attachment 42745 [details]
A proposed patch 2 (only for Mac)

What is the reason for having the code be Mac only? I can't see any mac specific code in it...

Your test case should describe how to run the test manually

--Oliver

[Hironori Bono]

Created attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

Thank you for your comments.

> What is the reason for having the code be Mac only? I can't see any mac specific code in it...

This is just because I haven't had any idea how I can add files to GTK (or QT) projects.
I have added them and checked this change can be compiled on WebKit GTK and WebKit Win. (I'm not confident this is correct, though. Please feel free to blame me if I'm wrong.)

> Your test case should describe how to run the test manually

Thank you for noticing this.
I added a brief comment how to use my layout test manually

Regards,

Hironori Bono

[Oliver Hunt]

Comment on attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

r=me

[Eric Seidel (no email)]

Comment on attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

Bono-san doesn't seem to be a committer yet, so adding to the cq.

[WebKit Commit Bot]

Comment on attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

Rejecting patch 42854 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
e/rendering/RenderRubyRun.cpp -o /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/RenderRubyRun.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCompositionEvent.o /Users/eseidel/Projects/CommitQueue/WebCore/../WebKitBuild/Release/DerivedSources/WebCore/JSCompositionEvent.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)

[Eric Seidel (no email)]

Bono-san will need to investigate the build failure and post a new patch.

[Hironori Bono]

Created attachment 43031 [details]
proposed patch 4 (for WebKit and Chromium)

Hi Eric,

Thank you so much for your review, and sorry for this build failure caused by my change.
I noticed the xcode project file in my previous change always refers to "WebKitBuild/Release/DerivedSources/WebCore/JSCompositionEvent.cpp" and it causes a build break when we build the debug version without building the release version. (This is a bonehead mistake of mine in adding JSCompositionEvent.{cpp,h} to this project.)
To fix this problem, I have changed the project file, and confirmed I can compile the debug version without building the release one.
(Except this change, this patch is same as the previous one.)
Would it be possible to review the update one?

Regards,

Hironori Bono

[Adam Barth]

Comment on attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

Clean up patch from pending-commit.

[Dmitry Titov]

Comment on attachment 43031 [details]
proposed patch 4 (for WebKit and Chromium)

The patch 43031 is identical to the one Oliver r+'ed, except the fixed path to JSCompositionEvent files in pbxproj.

One more thing:
> Index: WebCore/WebCore.vcproj/WebCore.vcproj
>  				<File
> +					RelativePath="..\inspector\front-end\DOMStorageDataGrid.js"
> +					>
> +				</File>
> +				<File

I think this is not an intended change. This file does not exist and seems not relevant to the patch.
Since the rest of the change is already reviewed, I will try to fix up vcproj and land this manually.

[Dmitry Titov]

Comment on attachment 43031 [details]
proposed patch 4 (for WebKit and Chromium)

Removing r? since it doesn't need another review, just landing.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/50968

[Hironori Bono]

(In reply to comment #15)
> Landed: http://trac.webkit.org/changeset/50968

Thank you so much for fixing my mistake in the vcproj file and land this change.