Last Comment Bug 717147 - input events during composition are not trusted events
: input events during composition are not trusted events
Status: RESOLVED FIXED
: inputmethod
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 722961
Blocks: 713502
  Show dependency treegraph
 
Reported: 2012-01-10 20:33 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-01-31 19:18 PST (History)
5 users (show)
masayuki: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part.1 input events which are fired after compositionupdate should be trusted events (2.57 KB, patch)
2012-01-10 20:33 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
ehsan: review+
Details | Diff | Review
Patch part.2 Don't do autocomplete during composition (1.04 KB, patch)
2012-01-10 21:25 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch part.3 Findbar shouldn't do incremental search during composition (1.18 KB, patch)
2012-01-10 22:37 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch part.2 Don't do autocomplete during composition (4.77 KB, patch)
2012-01-11 06:49 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
dolske: review+
Details | Diff | Review
Patch part.3 Findbar shouldn't do incremental search during composition (4.73 KB, patch)
2012-01-11 06:56 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
gavin.sharp: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-10 20:33:35 PST
Created attachment 587584 [details] [diff] [review]
Patch part.1 input events which are fired after compositionupdate should be trusted events

The text event handler of editor doesn't make nsAutoEditorKeypressOperation instance. Therefore, the input events during compsition (and at commit the composition) are not trusted events.

With the attached patch, input events during composition become trusted events. However, it causes some regressions in chrome UI.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-10 21:25:10 PST
Created attachment 587590 [details] [diff] [review]
Patch part.2 Don't do autocomplete during composition
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-10 22:37:18 PST
Created attachment 587598 [details] [diff] [review]
Patch part.3 Findbar shouldn't do incremental search during composition
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-11 06:49:03 PST
Created attachment 587684 [details] [diff] [review]
Patch part.2 Don't do autocomplete during composition

autocomplete shouldn't show popup during composition because it disturbs IME's candidate window. See bug 165055's screenshots.

The new test is copied from editor/libeditor/text/tests/test_bug527935.html, therefore it's in the path rather than under toolkit/.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-11 06:56:39 PST
Created attachment 587686 [details] [diff] [review]
Patch part.3 Findbar shouldn't do incremental search during composition

Findbar shouldn't find a word for unconverted composition string.

For example, if user wants to convert "ABC" to "DEF" by IME, user types "ABC" by keyboard. Then, if findbar did incremental search in "DEFABCDEF", the "ABC" would be selected. And when the user did convert to "DEF", findbar found the *second* "DEF". It's bad behavior. The first "DEF" should be selected.

Therefore, I think that findbar shouldn't do incremental search during composition.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-11 13:31:56 PST
Comment on attachment 587686 [details] [diff] [review]
Patch part.3 Findbar shouldn't do incremental search during composition

nit: for tests, the comment should be an assertion about the correct behavior, not a description of the error. So you should have:

>++      ok(gFindBar.hidden, "Failed to close findbar after testNormalFindWithComposition");

-> "findbar should be hidden after testNormalFindWithComposition"

>+         "testNormalFindWithComposition: find field is not focused");

-> "testNormalFindWithComposition: find field should be focused"

>+         "testNormalFindWithComposition: failed to find at committing composition '" + searchStr + "'");

-> "testNormalFindWithComposition: text should be found after committing composition"

etc. This applies to a bunch of places in the existing test, it looks like...
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-11 19:26:30 PST
> Jean-Yves Perrier [:teoli] 2012-01-11 08:02:04 PST
> Keywords: dev-doc-needed

Why?
Comment 8 Jean-Yves Perrier [:teoli] 2012-01-12 09:52:51 PST
:masayuki: I wasn't sure, so I put the dev-doc-needed so that we could track if some doc is needed.

So you do think this is only an "implementation fix" change that doesn't need to be documented?
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-12 18:05:03 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #8)
> So you do think this is only an "implementation fix" change that doesn't
> need to be documented?

Yes.
Comment 10 Justin Dolske [:Dolske] 2012-01-12 18:32:10 PST
Comment on attachment 587684 [details] [diff] [review]
Patch part.2 Don't do autocomplete during composition

Review of attachment 587684 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable. Do any of the other ::Handle* methods need a similar treatment?
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-12 18:35:57 PST
(In reply to Justin Dolske [:Dolske] from comment #10)
> Do any of the other ::Handle* methods need a similar
> treatment?

In the future, they might need. Currently, key events are not still fired during composition. However, D3E spec thinks only keydown/keyup events should be fired even during composition. When we fix it, we need to do it, maybe.

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