input events during composition are not trusted events

RESOLVED FIXED in mozilla12

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla12
inputmethod
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
input event handlers:

http://mxr.mozilla.org/mozilla-central/search?string=addEventListener%28\%22input\%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=event%3D\%22input\%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=oninput%3D&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Created attachment 587590 [details] [diff] [review]
Patch part.2 Don't do autocomplete during composition
Created attachment 587598 [details] [diff] [review]
Patch part.3 Findbar shouldn't do incremental search during composition
Attachment #587584 - Flags: review?(ehsan)
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/.
Attachment #587590 - Attachment is obsolete: true
Attachment #587684 - Flags: review?(dolske)
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.
Attachment #587598 - Attachment is obsolete: true
Attachment #587686 - Flags: review?(gavin.sharp)
Keywords: dev-doc-needed
Attachment #587584 - Flags: review?(ehsan) → review+
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...
Attachment #587686 - Flags: review?(gavin.sharp) → review+
> Jean-Yves Perrier [:teoli] 2012-01-11 08:02:04 PST
> Keywords: dev-doc-needed

Why?
: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?
(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 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?
Attachment #587684 - Flags: review?(dolske) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f64197c3878
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a71337e5cf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e58e679fca
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/1f64197c3878
https://hg.mozilla.org/mozilla-central/rev/1a71337e5cf7
https://hg.mozilla.org/mozilla-central/rev/78e58e679fca
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Depends on: 722961
You need to log in before you can comment on or make changes to this bug.