Bug 33179 - [Qt] Enterkey to go to Newline does not work in the text area(in HTML form)
Summary: [Qt] Enterkey to go to Newline does not work in the text area(in HTML form)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P1 Blocker
Assignee: Janne Koskinen
URL:
Keywords: Qt, QtTriaged
: 52572 (view as bug list)
Depends on: 57472
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-04 12:50 PST by Vikram
Modified: 2011-05-20 05:48 PDT (History)
20 users (show)

See Also:


Attachments
patch (2.29 KB, patch)
2010-02-10 00:33 PST, sapeltom
no flags Details | Formatted Diff | Diff
patch (2.31 KB, patch)
2010-02-10 02:05 PST, sapeltom
no flags Details | Formatted Diff | Diff
patch (2.31 KB, patch)
2010-02-10 02:22 PST, sapeltom
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch reworked to 2.1 (1.33 KB, patch)
2011-03-01 06:47 PST, Janne Koskinen
no flags Details | Formatted Diff | Diff
Patch for trunk (3.02 KB, patch)
2011-03-10 03:19 PST, Janne Koskinen
no flags Details | Formatted Diff | Diff
Patch for trunk2 (3.12 KB, patch)
2011-03-10 03:27 PST, Janne Koskinen
no flags Details | Formatted Diff | Diff
Patch for trunk3 (3.18 KB, patch)
2011-03-10 03:39 PST, Janne Koskinen
no flags Details | Formatted Diff | Diff
Patch for trunk4 (3.19 KB, patch)
2011-03-10 04:25 PST, Janne Koskinen
kenneth: review+
akling: commit-queue-
Details | Formatted Diff | Diff
QtWebkit2.1 backport (3.24 KB, patch)
2011-03-10 05:11 PST, Janne Koskinen
no flags Details | Formatted Diff | Diff
Allows QtWebKit to insert a new line for a enter key event without key text. (2.38 KB, patch)
2011-04-11 08:48 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
proposed fix (2.38 KB, patch)
2011-04-22 08:18 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
proposed fix (2.38 KB, patch)
2011-04-22 08:35 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
add more tests (6.06 KB, patch)
2011-04-26 10:24 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
updated with Chang's comments (7.95 KB, patch)
2011-05-16 08:50 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
updated Changelog (8.07 KB, patch)
2011-05-16 10:19 PDT, Yi Shen
akling: review-
Details | Formatted Diff | Diff
fix the issue found by kling (8.05 KB, patch)
2011-05-18 12:20 PDT, Yi Shen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vikram 2010-01-04 12:50:03 PST
STEPS TO REPRODUCE:
1.Open browser and Load http://waplabdc.nokia-boston.com/browser/users/diana/inputtest.html
2.Enter text in the textarea and press the enter key to go to new line. 
ACTUAL RESULTS:
The enter key does not lead to newline

EXPECTED RESULTS:
Hitting the enter key should lead to the newline within the textarea

Internally linked to - https://qtrequirements.europe.nokia.com/browse/BR-974
Comment 1 sapeltom 2010-02-10 00:33:25 PST
Created attachment 48471 [details]
patch
Comment 2 WebKit Review Bot 2010-02-10 00:34:28 PST
Attachment 48471 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:385:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:384:  Missing space before ( in switch(  [whitespace/parens] [5]
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:384:  Missing space before {  [whitespace/braces] [5]
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:392:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:393:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 5


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 sapeltom 2010-02-10 02:05:46 PST
Created attachment 48477 [details]
patch
Comment 4 sapeltom 2010-02-10 02:06:36 PST
Added switch / case with two editor handling cases.
WebCoreSupport\EditorClientQt.cpp (EditorClientQt::handleKeyboardEvent)
Comment 5 WebKit Review Bot 2010-02-10 02:11:53 PST
Attachment 48477 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:391:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 sapeltom 2010-02-10 02:22:11 PST
Created attachment 48478 [details]
patch
Comment 7 sapeltom 2010-02-10 02:24:07 PST
Added switch / case with two editor handling cases.
WebCoreSupport\EditorClientQt.cpp (EditorClientQt::handleKeyboardEvent)
Comment 8 Simon Hausmann 2010-02-10 13:34:57 PST
This code has been fragile in the past. Could you provide a layout test with it? I'm worried about introducing regressions here...
Comment 9 Antonio Gomes 2010-02-10 19:14:38 PST
(In reply to comment #7)
> Added switch / case with two editor handling cases.
> WebCoreSupport\EditorClientQt.cpp (EditorClientQt::handleKeyboardEvent)

typos:

"+            // Added two case for enter handling used in text editor. E.g. creates new line in multi line editor, 
+            // or starts search if enter is presses in google's text editor field.   
             if (cmd && frame->editor()->command(cmd).isTextInsertion()
"

* two caseS
* is presseD

maybe the whole comments needs better English.

content LGTM
Comment 10 Kenneth Rohde Christiansen 2010-02-11 04:15:30 PST
"+            // Added two case for enter handling used in text editor. E.g.
creates new line in multi line editor, 
+            // or starts search if enter is presses in google's text editor
field.  

// Add two special cases for Enter key:
// 1) When in multiline editor, Enter release creates a newline.
// 2) When in Google's text editor (link pleease!), make Enter start a search

Something like that?
Comment 11 Simon Hausmann 2010-02-15 03:56:39 PST
Comment on attachment 48478 [details]
patch

I'm okay with this change, but I'd really like to see an automated layout or unit test for this issue, so avoid this regressing.

A link to a nokia-internal website is not sufficient.
Comment 12 Tor Arne Vestbø (not active) 2010-03-05 09:39:40 PST
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
Comment 13 Arend van Beelen jr. 2011-02-08 03:24:46 PST
Sir, I just noticed I reported a similar bug (#52572) and I'm a little bit disappointed that a patch from a year ago apparently is not being committed.

From what I understand the only reason for not committing is the absence of an automated test. Now I admit I don't know how thorough your tests are, but this bug is only reproducible on Symbian while interacting with the keyboard and therefore very hard to replicate in a test environment.

Is it really worthwhile to wait with committing while users are actually complaining about this?
Comment 14 Janne Koskinen 2011-02-08 03:53:01 PST
Hi, if you want to continue with this issue it would be excellent.
I wasn't sure if the issue existed anymore as nobody has complained and we have changed how Qt inputcontext works. Can you reproduce this using QtSDK 1.1 TP ?

Clearing assignment flag. Sakari hasn't worked with webkit for a year now..
I'll close the other issue as duplicate.
Comment 15 Janne Koskinen 2011-02-08 03:54:36 PST
*** Bug 52572 has been marked as a duplicate of this bug. ***
Comment 16 Arend van Beelen jr. 2011-02-08 05:37:28 PST
I just installed QtSDK 1.1 TP, installed Qt 4.7.1 and Qt 4.7.1 WebKit to my phone and recompiled my application using the new SDK. Unfortunately, I can still reproduce the bug.

I did notice one difference with before (apart from some WebKit rendering issues compared to Qt 4.6), which is that the latest character you're typing stays highlighted (like an override cursor) until the entry times out and the character is fixed. It's no more than a cosmetic change though :)

Btw, I'm not sure if this is valuable to this bugreport, but apart from the inability to enter newlines, auto-capitalization also doesn't work in QtWebKit, whereas it does in the native browser.
Comment 17 Janne Koskinen 2011-02-08 06:23:22 PST
Thanks for the reproduce effort.
Auto-capitalization doesn't work and is a known issue. I recall we had to disable it for some reason... maybe it is time to try to enable it again. Underlining is precommit string at work. It should show only if you use T9 or having a language that uses characters to represent syllables.
Comment 18 Arend van Beelen jr. 2011-02-08 06:26:01 PST
One more note regarding Qt 4.7.1 WebKit on the N8. I just noticed that when trying to enter something into a password field, the first character will keep adding "stars" for every keypress, even when the final character was not yet selected.

For example, if I have to type "3", I have to hit the 3 key 4 times. However, the keyboard will write "def3" instead of "3". (At least that's what I think it does, since the password field will only show "****" instead of "*")

Imho, this is quite a serious regression as it will become virtually impossible to enter correct passwords. Should I create a new bug for this?
Comment 19 Arend van Beelen jr. 2011-02-08 06:35:05 PST
@Janne: Regarding the auto-capitalization, I found the following commit: http://gitorious.org/webkit/qtwebkit/commit/9036178

So yes, it does appear to be disabled on purpose. However, I think I read (can't find the link for that anymore :( ) this was done because you don't want auto-capitalization in password fields.

Looking at the source in question:

webPageClient->setInputMethodHint(Qt::ImhHiddenText, isPasswordField);
#if defined(Q_WS_MAEMO_5) || defined(Q_OS_SYMBIAN)
// disables auto-uppercase and predictive text for mobile devices
webPageClient->setInputMethodHint(Qt::ImhNoAutoUppercase, true);
webPageClient->setInputMethodHint(Qt::ImhNoPredictiveText, true);
#endif // Q_WS_MAEMO_5 || Q_OS_SYMBIAN

I think this should then become:

webPageClient->setInputMethodHint(Qt::ImhHiddenText, isPasswordField);
#if defined(Q_WS_MAEMO_5) || defined(Q_OS_SYMBIAN)
if (isPasswordField) {
    // disables auto-uppercase and predictive text in password fields for mobile devices
    webPageClient->setInputMethodHint(Qt::ImhNoAutoUppercase, true);
    webPageClient->setInputMethodHint(Qt::ImhNoPredictiveText, true);
}
#endif // Q_WS_MAEMO_5 || Q_OS_SYMBIAN

Unfortunately I cannot easily compile and install a custom QtWebKit on my phone, otherwise I could try for myself...
Comment 20 Janne Koskinen 2011-02-08 07:11:23 PST
(In reply to comment #18)
> One more note regarding Qt 4.7.1 WebKit on the N8. I just noticed that when trying to enter something into a password field, the first character will keep adding "stars" for every keypress, even when the final character was not yet selected.

Starring without character echo is Bug 32509

> Imho, this is quite a serious regression as it will become virtually impossible to enter correct passwords. Should I create a new bug for this?

That has been fixed already. I think it was part of Bug 49787 .

>you don't want auto-capitalization in password field

Which we still don't want. there was something else...
Let's keep this bug report about the Newline issue. For discussion there is mail lists you can and should join, see http://lists.webkit.org/mailman/listinfo
Comment 21 Benjamin Poulain 2011-02-09 03:05:47 PST
Janne, I raise as P1 because that looks like pretty bad bug.

If it is not as important as it looks, please reset the priority to P2.
Comment 22 Janne Koskinen 2011-02-09 23:53:51 PST
(In reply to comment #21)
> Janne, I raise as P1 because that looks like pretty bad bug.
It is what it is. Without the fix you can't add newline on this comment box. So yeah, all forums, feedback forms etc. are affected.
Comment 23 Suresh Voruganti 2011-02-14 06:57:37 PST
(In reply to comment #22)
> (In reply to comment #21)
> > Janne, I raise as P1 because that looks like pretty bad bug.
> It is what it is. Without the fix you can't add newline on this comment box. So yeah, all forums, feedback forms etc. are affected.

@Janne, are you working on this bug?
Comment 24 Suresh Voruganti 2011-02-14 07:19:44 PST
Adding to Qtwebkit 2.1 Master bug.
Comment 25 Ademar Reis 2011-02-17 13:12:01 PST
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Janne, I raise as P1 because that looks like pretty bad bug.
> > It is what it is. Without the fix you can't add newline on this comment box. So yeah, all forums, feedback forms etc. are affected.
> 
> @Janne, are you working on this bug?

The 2.1 window is about to close. Janne?
Comment 26 Janne Koskinen 2011-02-18 00:09:39 PST
> The 2.1 window is about to close. Janne?

Ok, I'll take a look at it. I have a gut feeling that this might be fixable in Qt instead of QtWebkit.

Maybe the patch could be applied as OS(SYMBIAN) only. I find it very odd that other systems are not affected by this, indicating to Qt inputcontext being wrong in Symbian... Which is where I base my assumption.
Comment 27 Suresh Voruganti 2011-02-22 14:17:36 PST
(In reply to comment #26)
> > The 2.1 window is about to close. Janne?
> 
> Ok, I'll take a look at it. I have a gut feeling that this might be fixable in Qt instead of QtWebkit.
> 
> Maybe the patch could be applied as OS(SYMBIAN) only. I find it very odd that other systems are not affected by this, indicating to Qt inputcontext being wrong in Symbian... Which is where I base my assumption.

Can we assume that you are fixing in Qt?
Comment 28 Janne Koskinen 2011-02-23 06:47:44 PST
Created new task to Qt that I found when investigating this http://bugreports.qt.nokia.com/browse/QTBUG-17639
Patches listed here won't work fully. We are missing few microfocus changes somewhere.
One weird thing is that if input field already contains text the caret position and text insertion position are offset by the amount of text i.e. caret doesn't know there is text and insertion does.
Comment 29 Suresh Voruganti 2011-02-28 07:30:39 PST
As this issue is getting fixed in Qt, removing the block for Qtwebkit 2.1.
Comment 30 Janne Koskinen 2011-02-28 08:37:24 PST
(In reply to comment #29)
> As this issue is getting fixed in Qt, removing the block for Qtwebkit 2.1.

No it ain't. This still needs a fix here. There is a API design issue in Qt that the report is about. I'll post a fix to this tomorrow that will still need to be done. Sorry, if I mislead you.
Comment 31 Suresh Voruganti 2011-02-28 09:03:48 PST
(In reply to comment #30)
> (In reply to comment #29)
> > As this issue is getting fixed in Qt, removing the block for Qtwebkit 2.1.
> 
> No it ain't. This still needs a fix here. There is a API design issue in Qt that the report is about. I'll post a fix to this tomorrow that will still need to be done. Sorry, if I mislead you.

Thank you. I have added this error back to Qtwebkit 2.1 master bug
Comment 32 Janne Koskinen 2011-03-01 06:47:53 PST
Created attachment 84225 [details]
patch reworked to 2.1

Adding reworked patch here. I'll add tests and stuff and make a proper one once I get past that QWebPage autotests crash on Symbian.
This patch due how Qt behaves will not show lines in VKB i.e. everything gets drawn on to same line. underlying textarea will get the newlines and characters correctly.
It is even more trouble as there is one more bug that doesn't clear the lines and since caret is not inline with text insertion it is really hard to see from sea of old characters what you are typing.
Comment 33 Ademar Reis 2011-03-03 09:01:52 PST
Janne, do you plan to submit a patch for trunk? 2.1 should receive cherry-picks or backports after the patch is submited to trunk, not before.
Comment 34 Janne Koskinen 2011-03-03 12:22:39 PST
(In reply to comment #33)
> Janne, do you plan to submit a patch for trunk? 2.1 should receive cherry-picks or backports after the patch is submited to trunk, not before.

Yes. I started setting up environment where I can build trunk so I can create that patch. It takes ~10 hours to get the environment so I should have it by tomorrow.
Comment 35 Joel Parks 2011-03-08 13:13:00 PST
this error is on the QtWebKit 2.1 Top error list and on the QtWebKit 2.1.x Top error list.  Janne if you have an ETA(LE) it would be appreciated.
Comment 36 Janne Koskinen 2011-03-10 03:19:59 PST
Created attachment 85303 [details]
Patch for trunk

Patch for trunk inclusion.
Tested on Nokia N8 and Nokia E7. Additional test run in WinXP to see that the patch is needed there as well.
I can mark this Symbian only if we want to avoid possible regressions that this will cause.
Comment 37 WebKit Review Bot 2011-03-10 03:22:54 PST
Attachment 85303 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:539:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2121:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2124:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2125:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2140:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Janne Koskinen 2011-03-10 03:27:15 PST
Created attachment 85305 [details]
Patch for trunk2

Copy/paste fail
Comment 39 WebKit Review Bot 2011-03-10 03:32:21 PST
Attachment 85305 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:539:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2121:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2124:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2125:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2140:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Janne Koskinen 2011-03-10 03:39:05 PST
Created attachment 85306 [details]
Patch for trunk3

And style fixed <sigh> :)
Related note: Why does check-webkit-style try to download irclib? Because of this download the script fails on my machine.
Comment 41 WebKit Review Bot 2011-03-10 03:41:03 PST
Attachment 85306 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:539:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Early Warning System Bot 2011-03-10 03:52:59 PST
Attachment 85303 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8125019
Comment 43 Janne Koskinen 2011-03-10 04:25:53 PST
Created attachment 85308 [details]
Patch for trunk4

Not my day obviously :)
Comment 44 Janne Koskinen 2011-03-10 05:11:44 PST
Created attachment 85311 [details]
QtWebkit2.1 backport

Backport added. I'll run some additional (manual) testing in platforms that I have access to.
I won't be at office tomorrow so replies to inquiries on Monday (latest).
Just FYI as there were a lot of people asking about this and other stuff. Hah, putting OoO messages into bugzilla :)
Comment 45 Caio Marcelo de Oliveira Filho 2011-03-11 09:28:57 PST
(In reply to comment #44)
> Created an attachment (id=85311) [details]
> QtWebkit2.1 backport
> 
> Backport added. I'll run some additional (manual) testing in platforms that I have access to.

Ok, then we are not integrating this now to 2.1, please let us know when it's ready.

And I think you missed the cq? on the patch for trunk4.
Comment 46 Janne Koskinen 2011-03-15 07:47:16 PDT
(In reply to comment #45)
> (In reply to comment #44)
> > Created an attachment (id=85311) [details] [details]
> > QtWebkit2.1 backport
> > 
> > Backport added. I'll run some additional (manual) testing in platforms that I have access to.
> 
> Ok, then we are not integrating this now to 2.1, please let us know when it's ready.
> 
> And I think you missed the cq? on the patch for trunk4.

Since it is reviewed and no further comments then let's put it in and see if it works on all OSes. Not having it in 2.1 means there is not much testing done on Symbian side.
Comment 47 Luiz Agostini 2011-03-15 14:17:33 PDT
Patch added to 2.1 as 4c627893fc0270728804e185a15245b59e508de9, merged into 2.1.x as well.
Comment 48 Andreas Kling 2011-03-29 05:58:16 PDT
Comment on attachment 85308 [details]
Patch for trunk4

View in context: https://bugs.webkit.org/attachment.cgi?id=85308&action=review

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:541
> +        case QWebPage::InsertLineSeparator:
> +            m_page->triggerAction(action);

Missing break; statement here.
Comment 49 Alexis Menard (darktears) 2011-03-29 06:04:55 PDT
Committed r82238: <http://trac.webkit.org/changeset/82238>
Comment 50 Alexis Menard (darktears) 2011-03-29 06:05:53 PDT
(In reply to comment #48)
> (From update of attachment 85308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85308&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:541
> > +        case QWebPage::InsertLineSeparator:
> > +            m_page->triggerAction(action);
> 
> Missing break; statement here.

The patch I landed contains it, lord Kling.
Comment 51 WebKit Review Bot 2011-03-29 06:21:27 PDT
http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal
Comment 52 Alexis Menard (darktears) 2011-03-29 06:26:45 PDT
(In reply to comment #51)
> http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal

Fix is on the way
Comment 53 Alexis Menard (darktears) 2011-03-29 06:38:01 PDT
(In reply to comment #52)
> (In reply to comment #51)
> > http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal
> 
> Fix is on the way

Landed http://trac.webkit.org/changeset/82243
Comment 54 Yi Shen 2011-04-11 07:58:07 PDT
It seems this patch has caused a regression issue on Linux, which sends onsearch events twice. (https://bugs.webkit.org/show_bug.cgi?id=57472)

This bug (enterkey to go to newline doesn't work) is only reproducible on Symbian, and the root cause is that, when user presses on the enter key through the virtual Keyboard on Symbian, a QKeyEvent WITHOUT text gets fired to the QWebPage, just like below,

    QKeyEvent keyEnter(QEvent::KeyPress, Qt::Key_Enter, Qt::NoModifier);
    page->event(&keyEnter);

However, the problem is that WebKit's EventHandler requires the keyEnter event contains a text "\r" or "\n", if you want to insert a newline. Otherwise, this insertNewLine request won't be handled.

    PlatformKeyboardEvent keyPressEvent = initialKeyEvent;
    if (keyPressEvent.text().isEmpty())
        return keydownResult;
    ... 
    node->dispatchEvent(keypress, ec); // insert new line here

If we look back the Linux, things are a little different. When user presses the enter key through the keyboard, a QKeyEvent WITH text "\r" gets fired to the QWebPage as below,

    QKeyEvent keyEnter(QEvent::KeyPress, Qt::Key_Enter, Qt::NoModifier, "\r");
    page->event(&keyEnter);

Then, everything works fine - a new line gets inserted to the text area.

The problem for the patch is that, on linux, two new lines get inserted when user clicks the enter key since the generated keyevent contains "\r".

bool EventHandler::keyEvent(const PlatformKeyboardEvent& initialKeyEvent){
    ...
    m_frame->editor()->handleInputMethodKeydown(keydown.get()); // insert first new line

    PlatformKeyboardEvent keyPressEvent = initialKeyEvent;
    if (keyPressEvent.text().isEmpty())
        return keydownResult;
    ... 
    node->dispatchEvent(keypress, ec); // insert second new line here!
    ...
}

This also can explain why it causes the regression -- the onsearch event gets fire twice for the same reason.
Comment 55 Yi Shen 2011-04-11 08:03:36 PDT
Again, since this is a symbian only bug. It can be either fixed on Qt-symbian side by sending a QKeyEvent with text info when user presses the enter key on VKB, or fix it on qtwebkit side.

Following is how the qtwebkit patch looks like, I will add the patch for review after I finish the test.

Index: Source/WebKit/qt/Api/qwebpage.cpp
===================================================================
--- Source/WebKit/qt/Api/qwebpage.cpp	(revision 83440)
+++ Source/WebKit/qt/Api/qwebpage.cpp	(working copy)
@@ -929,6 +929,9 @@ void QWebPagePrivate::keyPressEvent(QKey
                 else
                     q->triggerAction(QWebPage::Back);
                 break;
+            case Qt::Key_Enter:
+                q->triggerAction(editorActionForKeyEvent(ev));
+                break;
             default:
                 handled = false;
                 break;
Comment 56 Yi Shen 2011-04-11 08:48:44 PDT
Created attachment 89012 [details]
Allows QtWebKit to insert a new line for a enter key event without key text.

I think it makes sense that qtwebkit can insert a new line for a enter key event which has no text.
Comment 57 Eric Seidel 2011-04-18 09:16:22 PDT
Comment on attachment 89012 [details]
Allows QtWebKit to insert a new line for a enter key event without key text.

Cleared review? from attachment 89012 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 58 Yi Shen 2011-04-22 08:17:08 PDT
The patch has introduced a new bug in Linux (two newlines get inserted when press enter key each time), which needs to be addressed ASAP.
Comment 59 Yi Shen 2011-04-22 08:18:03 PDT
Created attachment 90708 [details]
proposed fix
Comment 60 Yi Shen 2011-04-22 08:35:58 PDT
Created attachment 90709 [details]
proposed fix
Comment 61 Antonio Gomes 2011-04-22 14:10:45 PDT
Comment on attachment 90709 [details]
proposed fix

No test?
Comment 62 Yi Shen 2011-04-25 11:44:50 PDT
(In reply to comment #61)
> (From update of attachment 90709 [details])
> No test?

Just reuse the test added by Janne's patch. See tst_QWebPage::inputMethods() in https://bug-33179-attachments.webkit.org/attachment.cgi?id=85308
Comment 63 Antonio Gomes 2011-04-25 17:24:26 PDT
(In reply to comment #62)
> (In reply to comment #61)
> > (From update of attachment 90709 [details] [details])
> > No test?
> 
> Just reuse the test added by Janne's patch. See tst_QWebPage::inputMethods() in https://bug-33179-attachments.webkit.org/attachment.cgi?id=85308

Well, it seems these tests did not cover your problem. We should improve it, or it can happen again in the future. Or make it explicit in the changelog why no tests are being added.
Comment 64 Yi Shen 2011-04-26 07:02:14 PDT
(In reply to comment #63)
> (In reply to comment #62)
> > (In reply to comment #61)
> > > (From update of attachment 90709 [details] [details] [details])
> > > No test?
> > 
> > Just reuse the test added by Janne's patch. See tst_QWebPage::inputMethods() in https://bug-33179-attachments.webkit.org/attachment.cgi?id=85308
> 
> Well, it seems these tests did not cover your problem. We should improve it, or it can happen again in the future. Or make it explicit in the changelog why no tests are being added.

You are right, Antonio:) I will add a new test for it.
Comment 65 Yi Shen 2011-04-26 10:24:08 PDT
Created attachment 91124 [details]
add more tests
Comment 66 Chang Shu 2011-05-16 07:20:51 PDT
I have two comments here:
1. The missing key text can be filled in function keyTextForKeyEvent in file WebCore/platform/qt/PlatformKeyboardEventQt.cpp. Then we don't need the code in qwebpage.cpp and EditorClientQt.cpp.
2. There should be some existing layout tests that are skipped for Qt due to this bug. It would be good to find them out and unskip them.
Comment 67 Yi Shen 2011-05-16 08:50:33 PDT
Created attachment 93648 [details]
updated with Chang's comments
Comment 68 Chang Shu 2011-05-16 08:57:16 PDT
LGTM. You need to find a reviewer then. :)
Comment 69 Kenneth Rohde Christiansen 2011-05-16 09:49:32 PDT
Comment on attachment 93648 [details]
updated with Chang's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=93648&action=review

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:-549
> -void EditorClientQt::handleInputMethodKeydown(KeyboardEvent* event)
> +void EditorClientQt::handleInputMethodKeydown(KeyboardEvent*)
>  {
> -#ifndef QT_NO_SHORTCUT
> -    const PlatformKeyboardEvent* kevent = event->keyEvent();
> -    if (kevent->type() == PlatformKeyboardEvent::RawKeyDown) {
> -        QWebPage::WebAction action = QWebPagePrivate::editorActionForKeyEvent(kevent->qtEvent());
> -        switch (action) {
> -        case QWebPage::InsertParagraphSeparator:
> -        case QWebPage::InsertLineSeparator:
> -            m_page->triggerAction(action);
> -            break;
> -        default:
> -            break;
> -        }
> -    }
> -#endif

The changelog doesnt explain why this was done.
Comment 70 Yi Shen 2011-05-16 10:19:20 PDT
Created attachment 93664 [details]
updated Changelog
Comment 71 Andreas Kling 2011-05-18 10:51:16 PDT
Comment on attachment 93664 [details]
updated Changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=93664&action=review

LGTM, apart from...

> Source/WebCore/platform/qt/PlatformKeyboardEventQt.cpp:590
>      case Qt::Key_Backtab:
>          if (event->text().isNull())
>              return "\t";
> +    case Qt::Key_Enter:
> +        if (event->text().isNull())
> +            return "\r";

Missing break statement before the Qt::Key_Enter case.
Comment 72 Chang Shu 2011-05-18 10:53:46 PDT
(In reply to comment #71)
> (From update of attachment 93664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93664&action=review
> 
> LGTM, apart from...
> 
> > Source/WebCore/platform/qt/PlatformKeyboardEventQt.cpp:590
> >      case Qt::Key_Backtab:
> >          if (event->text().isNull())
> >              return "\t";
> > +    case Qt::Key_Enter:
> > +        if (event->text().isNull())
> > +            return "\r";
> 
> Missing break statement before the Qt::Key_Enter case.

Gah, the return statement tricked me, too.
Comment 73 Yi Shen 2011-05-18 12:20:02 PDT
Created attachment 93963 [details]
fix the issue found by kling
Comment 74 WebKit Commit Bot 2011-05-18 15:11:07 PDT
Comment on attachment 93963 [details]
fix the issue found by kling

Clearing flags on attachment: 93963

Committed r86798: <http://trac.webkit.org/changeset/86798>
Comment 75 Ademar Reis 2011-05-19 13:20:26 PDT
I'm a bit lost between bug 57472 and this one... What's still missing to have them closed?
Comment 76 Yi Shen 2011-05-19 14:35:03 PDT
I closed it :-)
Comment 77 Ademar Reis 2011-05-20 05:46:14 PDT
Revision r86798 cherry-picked into qtwebkit-2.2 with commit fb119b5 <http://gitorious.org/webkit/qtwebkit/commit/fb119b5>