672697 - Use nsTextEvent for rendering text coming from QInputMethodEvent.

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Oleg Romashin (:romaxa)]

right now we are converting InputMethod text string into QKeyEvent, which is not very good.

I think we should use nsTextEvent for QInputMethod

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Use nsTextEvent when QInputMethod event has non empty commit string

Comment 2

[Jeremias Bosch (:jbos)]

this will break plugin support for keyboard.

Comment 3

[Jeremias Bosch (:jbos)]

Comment on attachment 547140 [details] [diff] [review]
Use nsTextEvent when QInputMethod event has non empty commit string

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

As said, for going i.e. into flash or any other plugin we need to have keyevents. nsTextevents are not working with that.

Comment 4

[Oleg Romashin (:romaxa)]

> As said, for going i.e. into flash or any other plugin we need to have
> keyevents. nsTextevents are not working with that.

We don't have keyEvents in QInputMethod mode....
Also same text events (pure unicode string) could happen on another platform.
So I guess it is not good to convert Text events on widget side, when we have nsTextEvents...
I think it should be better to handle QInputMethod events as nsTextEvents, and do Text->Key conversion only where it is needed (X-plugins for example).

Comment 5

[Oleg Romashin (:romaxa)]

With current implementation we also don't have proper key events, also in some cases we are keeping only first character of "commitString" and killing the rest
http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp#1593

so as result we are doing dark Magic instead of just sending Text as it is into DOM...

For plugins we can do Text->XSym mapping right before sending event to desktop-plugin,
for Maemo6 we can do quick hack and post QKey and QInputMethod - Events directly to plugin-process main loop:
see https://bugzilla.mozilla.org/show_bug.cgi?id=672857

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 547140 [details] [diff] [review]
Use nsTextEvent when QInputMethod event has non empty commit string

So any other comments?

Comment 7

[Jeremias Bosch (:jbos)]

I got some strange (prefilled) text fields on websites - which i still need to compare with the same fennec version without the patch.

Also selection of text and putting the courser in textfields seems to way more complicated than it used to be. Also here i need to compare versions.

Basic Textinput seems to work. 

Error Correction and such needs still to be tested.

It will take another 2-3 days in order to finish testcases.

Comment 8

[Jeremias Bosch (:jbos)]

Comment on attachment 547140 [details] [diff] [review]
Use nsTextEvent when QInputMethod event has non empty commit string

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

I tried with swipe keyboard and error correct provided through this. The issue is that the text (preedit and not commited text) is already commited to fennec. This cause that the preedited and not yet valid text does not get removed when the user select the correct word. You should correctly implement the preedit / commit states of nsTextEvent.

Comment 9

[Jeremias Bosch (:jbos)]

You may want to learn from https://bugzilla.mozilla.org/show_bug.cgi?id=583286 this does at least something (but it never worked well)

Comment 10

[Oleg Romashin (:romaxa)]

> I tried with swipe keyboard and error correct provided through this. The
> issue is that the text (preedit and not commited text) is already commited
> to fennec. This cause that the preedited and not yet valid text does not get
wait does it work with current implementation? I did not get any preedit stuff working with current implementation, so I don't see what I'm breaking here...

could you provide steps exactly how to reproduce case which I'm breaking?

Comment 11

[Jeremias Bosch (:jbos)]

Comment on attachment 547140 [details] [diff] [review]
Use nsTextEvent when QInputMethod event has non empty commit string

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

Actually after more testing I conclude that the code is fine. The issue is within swype not your code.

Comment 12

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/60617a76c65c

Comment 13

[Boris Zbarsky [:bzbarsky]]

Actually, this got pushed to mozilla-inbound, too.  It'll make the merge fun.  :(

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a59a63155ea8 to be precise

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a59a63155ea8

hm, the merge didn't show anything special, should I be scared? Looking at the file in central, everything looks correct.

Comment 16

[Oleg Romashin (:romaxa)]

I think hg merge detect correctly changes which are the same...