Bug 33179 – [Qt] Enterkey to go to Newline does not work in the text area(in HTML form)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vikram]

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

[sapeltom]

Created attachment 48471 [details]
patch

[WebKit Review Bot]

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.

[sapeltom]

Created attachment 48477 [details]
patch

[sapeltom]

Added switch / case with two editor handling cases.
WebCoreSupport\EditorClientQt.cpp (EditorClientQt::handleKeyboardEvent)

[WebKit Review Bot]

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.

[sapeltom]

Created attachment 48478 [details]
patch

[sapeltom]

Added switch / case with two editor handling cases.
WebCoreSupport\EditorClientQt.cpp (EditorClientQt::handleKeyboardEvent)

[Simon Hausmann]

This code has been fragile in the past. Could you provide a layout test with it? I'm worried about introducing regressions here...

[Antonio Gomes]

(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

[Kenneth Rohde Christiansen]

"+            // 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?

[Simon Hausmann]

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.

[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

[Arend van Beelen jr.]

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?

[Janne Koskinen]

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.

[Janne Koskinen]

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

[Arend van Beelen jr.]

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.

[Janne Koskinen]

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.

[Arend van Beelen jr.]

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?

[Arend van Beelen jr.]

@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...

[Janne Koskinen]

(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

[Benjamin Poulain]

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.

[Janne Koskinen]

(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.

[Suresh Voruganti]

(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?

[Suresh Voruganti]

Adding to Qtwebkit 2.1 Master bug.

[Ademar Reis]

(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?

[Janne Koskinen]

> 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.

[Suresh Voruganti]

(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?

[Janne Koskinen]

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.

[Suresh Voruganti]

As this issue is getting fixed in Qt, removing the block for Qtwebkit 2.1.

[Janne Koskinen]

(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.

[Suresh Voruganti]

(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

[Janne Koskinen]

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.

[Ademar Reis]

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.

[Janne Koskinen]

(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.

[Joel Parks]

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.

[Janne Koskinen]

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.

[WebKit Review Bot]

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.

[Janne Koskinen]

Created attachment 85305 [details]
Patch for trunk2

Copy/paste fail

[WebKit Review Bot]

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.

[Janne Koskinen]

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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 85303 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8125019

[Janne Koskinen]

Created attachment 85308 [details]
Patch for trunk4

Not my day obviously :)

[Janne Koskinen]

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 :)

[Caio Marcelo de Oliveira Filho]

(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.

[Janne Koskinen]

(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.

[Luiz Agostini]

Patch added to 2.1 as 4c627893fc0270728804e185a15245b59e508de9, merged into 2.1.x as well.

[Andreas Kling]

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.

[Alexis Menard (darktears)]

Committed r82238: <http://trac.webkit.org/changeset/82238>

[Alexis Menard (darktears)]

(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.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal

[Alexis Menard (darktears)]

(In reply to comment #51)
> http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal

Fix is on the way

[Alexis Menard (darktears)]

(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

[Yi Shen]

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.

[Yi Shen]

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;

[Yi Shen]

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.

[Eric Seidel (no email)]

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).

[Yi Shen]

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.

[Yi Shen]

Created attachment 90708 [details]
proposed fix

[Yi Shen]

Created attachment 90709 [details]
proposed fix

[Antonio Gomes]

Comment on attachment 90709 [details]
proposed fix

No test?

[Yi Shen]

(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

[Antonio Gomes]

(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.

[Yi Shen]

(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.

[Yi Shen]

Created attachment 91124 [details]
add more tests

[Chang Shu]

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.

[Yi Shen]

Created attachment 93648 [details]
updated with Chang's comments

[Chang Shu]

LGTM. You need to find a reviewer then. :)

[Kenneth Rohde Christiansen]

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.

[Yi Shen]

Created attachment 93664 [details]
updated Changelog

[Andreas Kling]

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.

[Chang Shu]

(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.

[Yi Shen]

Created attachment 93963 [details]
fix the issue found by kling

[WebKit Commit Bot]

Comment on attachment 93963 [details]
fix the issue found by kling

Clearing flags on attachment: 93963

Committed r86798: <http://trac.webkit.org/changeset/86798>

[Ademar Reis]

I'm a bit lost between bug 57472 and this one... What's still missing to have them closed?

[Yi Shen]

I closed it :-)

[Ademar Reis]

Revision r86798 cherry-picked into qtwebkit-2.2 with commit fb119b5 <http://gitorious.org/webkit/qtwebkit/commit/fb119b5>