Bug 33165 – [Qt] onKeyPress/Down/Up event handlers cannot be used when the text is entered through InputMethod

• Component: Platform
• Status: RESOLVED INVALID

[Vikram]

STEPS TO REPRODUCE:
1.Open Browser and Load http://waplabdc.nokia-boston.com/Browser/users/diana/enteralphabets.html
2.Begin entering text in the field given 
ACTUAL RESULTS:
Accepts all the input characters

EXPECTED RESULTS:
Only alphabetical input should be accepted

Linked to https://qtrequirements.europe.nokia.com/browse/BR-976

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component

[Robert Hogan]

I can't reproduce this in QtLauncher - only alphabetical characters are accepted (and backspace of course). I notice firefox, konqueror and opera prevent backspace but that seems wrong to me.

[Robert Hogan]

Pretty sure webkit has nothing to do with this, but will reopen since I'm not testing on S60.

[Petri Ojala]

The problem is that a function defined to 'onKeyPress' never gets called. 
(<input type='text' onkeypress='validate(event)' />).

With QtWebKit, InputBox gets its content via InputmethodEvents, not via  keyevents.

[Robert Hogan]

(In reply to comment #4)
> The problem is that a function defined to 'onKeyPress' never gets called. 
> (<input type='text' onkeypress='validate(event)' />).
> 
> With QtWebKit, InputBox gets its content via InputmethodEvents, not via 
> keyevents.

Are you sure? It seems to me that in QtLauncher the validate() function _is_ getting called and only allowing alphabetical characters as per the regex defined in the function. It also allows backspace/DEL but that seems a good thing. The test case is working fine so far as I can tell.

[Petri Ojala]

Yes, I'm sure. I tested it on N97 emulator and in N97 hw.

This bug seems to exists with Symbian platform only.

[Jocelyn Turcotte]

(In reply to comment #4)
> The problem is that a function defined to 'onKeyPress' never gets called. 
> (<input type='text' onkeypress='validate(event)' />).
> 
> With QtWebKit, InputBox gets its content via InputmethodEvents, not via 
> keyevents.
It might be possible to call onKeyPress for each char in the string returned
from the input method event, however I guess that this would create other
problems related to onKeyDown and onKeyUp not being called.

What kind of problem would the end user encounter on a real website because of
this limitation?

[Jocelyn Turcotte]

*** Bug 29372 has been marked as a duplicate of this bug. ***

[Jocelyn Turcotte]

(In reply to comment #8)
> *** Bug 29372 has been marked as a duplicate of this bug. ***

This seems like the same problem (if I understood well that S60 input is using the same channel as input method internationalization on other platforms)
If I'm wrong please re-open the other bug.

The other bug report also states that this works well in Safari and IE.

[Petri Ojala]

*** Bug 33166 has been marked as a duplicate of this bug. ***

[Jay Tucker]

Created attachment 54086 [details]
patch file

First attempt at patch. Tested on both Symbian and Linux platforms.

[WebKit Review Bot]

Attachment 54086 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jay Tucker]

Created attachment 54168 [details]
patch file

OK, let's try it again without the tab characters!

[Eric Seidel (no email)]

Are you using webkit-patch upload to post your patch?  Why is the first one not getting obsoleted and its review flags cleared?

[Simon Hausmann]

Comment on attachment 54086 [details]
patch file

I'll do a more thorough review tomorrow. Here are a few comments:

> +        //simulate keypress and keyrelease event for QWebpage to fire the javascript key event listeners
> +        //some input method can input more than 1 character at one time like Chinese input method, so a loop is needed.
> +        for (int i = 0; i < cmtedit.length(); ++i) {
> +            QChar echar = cmtedit.at(i);
> +            //if it's a ascii char, just transform it to key event to fire the javascript key event listeners

QKeyEvents can be constructed with unicode characters.

> +            if (echar >= 0 && echar <= 255) {
> +                QKeyEvent* keypressevent = new QKeyEvent(QEvent::KeyPress, echar.toAscii(), Qt::NoModifier, echar);
> +                qApp->sendEvent(q, keypressevent);
> +                QKeyEvent* keyreleaseevent = new QKeyEvent(QEvent::KeyRelease, echar.toAscii(), Qt::NoModifier, echar);
> +                qApp->sendEvent(q, keyreleaseevent);

I think you should allocate the key event on the stack instead of on the heap, to avoid leaking it.

> +            } else {
> +                //if it's an unicode char, insert it to the editor directly, since unicode char cannot be from a keyevent.
> +                editor->confirmComposition(QString(echar));
> +          }
> +        }
> +    } else if (!ev->preeditString().isEmpty()) {
>          QString preedit = ev->preeditString();
>          editor->setComposition(preedit, underlines, preedit.length(), 0);
>      }

[Simon Hausmann]

Comment on attachment 54168 [details]
patch file

> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 58169)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Tucker Jay  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] onKeyPress/Down/Up event handlers cannot be used when the text is entered through InputMethod
> +        https://bugs.webkit.org/show_bug.cgi?id=33165
> +
> +        Fixed bug preventing alpha-only and numeric-only text entry
> +        fields from working on Symbian platform.
> +
> +        * Api/qwebpage.cpp:
> +        (QWebPagePrivate::inputMethodEvent):
> +
>  2010-04-22  John Pavan  <[e-mail]>
>  
>          Reviewed by Laszlo Gombos.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp	(revision 58169)
> +++ WebKit/qt/Api/qwebpage.cpp	(working copy)
> @@ -1188,9 +1188,26 @@ void QWebPagePrivate::inputMethodEvent(Q
>          }
>      }
>  
> -    if (!ev->commitString().isEmpty())
> -        editor->confirmComposition(ev->commitString());
> -    else if (!ev->preeditString().isEmpty()) {
> +    if (!ev->commitString().isEmpty()) {
> +        QString cmtedit = ev->commitString();
> +        //delete the selection
> +        editor->confirmComposition("");
> +        //simulate keypress and keyrelease event for QWebpage to fire the javascript key event listeners
> +        //some input method can input more than 1 character at one time like Chinese input method, so a loop is needed.
> +        for (int i = 0; i < cmtedit.length(); ++i) {
> +            QChar echar = cmtedit.at(i);
> +            //if it's a ascii char, just transform it to key event to fire the javascript key event listeners
> +            if (echar >= 0 && echar <= 255) {
> +                QKeyEvent* keypressevent = new QKeyEvent(QEvent::KeyPress, echar.toAscii(), Qt::NoModifier, echar);
> +                qApp->sendEvent(q, keypressevent);
> +                QKeyEvent* keyreleaseevent = new QKeyEvent(QEvent::KeyRelease, echar.toAscii(), Qt::NoModifier, echar);
> +                qApp->sendEvent(q, keyreleaseevent);
> +            } else {
> +                //if it's an unicode char, insert it to the editor directly, since unicode char cannot be from a keyevent.
> +                editor->confirmComposition(QString(echar));
> +          }
> +        }
> +    } else if (!ev->preeditString().isEmpty()) {
>          QString preedit = ev->preeditString();
>          editor->setComposition(preedit, underlines, preedit.length(), 0);
>      }
WebKit/qt/Api/qwebpage.cpp:1193
 +          //delete the selection
Please add a space after the comment sign

WebKit/qt/Api/qwebpage.cpp:1194
 +          editor->confirmComposition("");
Please use String()'s default constructor instead of passing "", which does an implicit conversion from ascii and also allocate a StringImpl object on the heap for no reason.

WebKit/qt/Api/qwebpage.cpp:1206
 +                  //if it's an unicode char, insert it to the editor directly, since unicode char cannot be from a keyevent.
This comment doesn't make sense to me. Why can't be a unicode char be sent from a key event? QKeyEvent takes a QString, and certainly there are keyboards out there that produce characters outside the ASCII range.

WebKit/qt/Api/qwebpage.cpp:1201
 +                  QKeyEvent* keypressevent = new QKeyEvent(QEvent::KeyPress, echar.toAscii(), Qt::NoModifier, echar);
This QKeyEvent is allocated on the heap, but it is never deleted. Please allocate it on the stack instead.


Please also add a test for this. For example you could write a unit test that generates synthetic input method events and before sending them initialize a snippet of JS that registers event listeners. Then you can verify if they were called correctly during the synthetic composition sequence or not.

[Jay Tucker]

Created attachment 54950 [details]
revised patch

Please take a look at the revised patch and the associated test case.

[Jay Tucker]

Comment on attachment 54950 [details]
revised patch

Please cancel the requested review. First of all, we've discovered that the test case being used (the test website) is itself flawed. More importantly, it's become apparent that it's not even sufficiently clear what the bug is and how it manifests itself, particularly on handheld devices.

[Jesus Sanchez-Palencia]

(In reply to comment #18)
> (From update of attachment 54950 [details])
> Please cancel the requested review. First of all, we've discovered that the test case being used (the test website) is itself flawed. More importantly, it's become apparent that it's not even sufficiently clear what the bug is and how it manifests itself, particularly on handheld devices.


Is this still a valid bug report?

[Jay Tucker]

Yes, it is. It's still a topic of heated discussion. :-)

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.