Last Comment Bug 672697 - Use nsTextEvent for rendering text coming from QInputMethodEvent.
: Use nsTextEvent for rendering text coming from QInputMethodEvent.
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: Widget: Qt (show other bugs)
: Trunk
: x86 MeeGo
: -- normal (vote)
: mozilla8
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-19 17:38 PDT by Oleg Romashin (:romaxa)
Modified: 2016-07-11 21:54 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use nsTextEvent when QInputMethod event has non empty commit string (4.87 KB, patch)
2011-07-20 10:35 PDT, Oleg Romashin (:romaxa)
jeremias.bosch: review+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2011-07-19 17:38:57 PDT
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) 2011-07-20 10:35:14 PDT
Created attachment 547140 [details] [diff] [review]
Use nsTextEvent when QInputMethod event has non empty commit string
Comment 2 Jeremias Bosch (:jbos) 2011-07-20 13:43:15 PDT
this will break plugin support for keyboard.
Comment 3 Jeremias Bosch (:jbos) 2011-07-20 13:44:55 PDT
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) 2011-07-20 14:49:46 PDT
> 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) 2011-07-20 14:55:36 PDT
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) 2011-07-25 13:14:02 PDT
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) 2011-07-25 13:20:31 PDT
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) 2011-07-27 05:36:13 PDT
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) 2011-07-27 05:42:56 PDT
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) 2011-07-27 11:03:24 PDT
> 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) 2011-07-28 00:26:43 PDT
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) 2011-07-29 13:46:20 PDT
http://hg.mozilla.org/mozilla-central/rev/60617a76c65c
Comment 13 Boris Zbarsky [:bz] 2011-07-29 18:52:44 PDT
Actually, this got pushed to mozilla-inbound, too.  It'll make the merge fun.  :(
Comment 14 Boris Zbarsky [:bz] 2011-07-29 18:52:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a59a63155ea8 to be precise
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-01 07:57:45 PDT
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) 2011-08-01 08:36:40 PDT
I think hg merge detect correctly changes which are the same...

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