Bug 26310 - Webkit should generate "compositionstart", "compositionend", and "text" events like firefox
Summary: Webkit should generate "compositionstart", "compositionend", and "text" event...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://www.danilatos.com/event-test/E...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-10 22:40 PDT by Daniel Danilatos
Modified: 2009-11-15 18:48 PST (History)
7 users (show)

See Also:


Attachments
a work-in-progress patch (29.50 KB, patch)
2009-10-30 03:15 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
A proposed patch (only for Mac) (27.30 KB, patch)
2009-11-08 23:47 PST, Hironori Bono
no flags Details | Formatted Diff | Diff
A proposed patch (only for Mac) (27.59 KB, text/plain)
2009-11-09 02:50 PST, Hironori Bono
no flags Details
A proposed patch 2 (only for Mac) (27.59 KB, patch)
2009-11-09 02:53 PST, Hironori Bono
no flags Details | Formatted Diff | Diff
A proposed patch 3 (for WebKit and Chromium) (35.09 KB, patch)
2009-11-10 02:11 PST, Hironori Bono
commit-queue: commit-queue-
Details | Formatted Diff | Diff
proposed patch 4 (for WebKit and Chromium) (35.04 KB, patch)
2009-11-11 23:00 PST, Hironori Bono
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Danilatos 2009-06-10 22:40:14 PDT
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).
Comment 1 Hironori Bono 2009-10-30 03:15:47 PDT
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
Comment 2 Hironori Bono 2009-11-08 23:47:33 PST
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
Comment 3 Hironori Bono 2009-11-09 02:50:48 PST
Created attachment 42744 [details]
A proposed patch (only for Mac)
Comment 4 Hironori Bono 2009-11-09 02:53:33 PST
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.)
Comment 5 Oliver Hunt 2009-11-09 14:10:42 PST
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
Comment 6 Hironori Bono 2009-11-10 02:11:41 PST
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
Comment 7 Oliver Hunt 2009-11-10 04:16:19 PST
Comment on attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

r=me
Comment 8 Eric Seidel (no email) 2009-11-10 11:04:43 PST
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.
Comment 9 WebKit Commit Bot 2009-11-10 11:12:27 PST
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)
Comment 10 Eric Seidel (no email) 2009-11-11 10:41:49 PST
Bono-san will need to investigate the build failure and post a new patch.
Comment 11 Hironori Bono 2009-11-11 23:00:43 PST
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
Comment 12 Adam Barth 2009-11-12 18:22:15 PST
Comment on attachment 42854 [details]
A proposed patch 3 (for WebKit and Chromium)

Clean up patch from pending-commit.
Comment 13 Dmitry Titov 2009-11-13 12:59:51 PST
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.
Comment 14 Dmitry Titov 2009-11-13 13:16:54 PST
Comment on attachment 43031 [details]
proposed patch 4 (for WebKit and Chromium)

Removing r? since it doesn't need another review, just landing.
Comment 15 Dmitry Titov 2009-11-13 13:52:26 PST
Landed: http://trac.webkit.org/changeset/50968
Comment 16 Hironori Bono 2009-11-15 18:48:28 PST
(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.