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
Created attachment 48471 [details] patch
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.
Created attachment 48477 [details] patch
Added switch / case with two editor handling cases. WebCoreSupport\EditorClientQt.cpp (EditorClientQt::handleKeyboardEvent)
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.
Created attachment 48478 [details] patch
This code has been fragile in the past. Could you provide a layout test with it? I'm worried about introducing regressions here...
(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
"+ // 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 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.
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
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?
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.
*** Bug 52572 has been marked as a duplicate of this bug. ***
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.
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.
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?
@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...
(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
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.
(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.
(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?
Adding to Qtwebkit 2.1 Master bug.
(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?
> 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.
(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?
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.
As this issue is getting fixed in Qt, removing the block for Qtwebkit 2.1.
(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.
(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
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.
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.
(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.
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.
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.
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.
Created attachment 85305 [details] Patch for trunk2 Copy/paste fail
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.
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.
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.
Attachment 85303 [details] did not build on qt: Build output: http://queues.webkit.org/results/8125019
Created attachment 85308 [details] Patch for trunk4 Not my day obviously :)
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 :)
(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.
(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.
Patch added to 2.1 as 4c627893fc0270728804e185a15245b59e508de9, merged into 2.1.x as well.
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.
Committed r82238: <http://trac.webkit.org/changeset/82238>
(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.
http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal
(In reply to comment #51) > http://trac.webkit.org/changeset/82238 might have broken Qt Linux Release minimal Fix is on the way
(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
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.
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;
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 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).
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.
Created attachment 90708 [details] proposed fix
Created attachment 90709 [details] proposed fix
Comment on attachment 90709 [details] proposed fix No test?
(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
(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.
(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.
Created attachment 91124 [details] add more tests
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.
Created attachment 93648 [details] updated with Chang's comments
LGTM. You need to find a reviewer then. :)
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.
Created attachment 93664 [details] updated Changelog
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.
(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.
Created attachment 93963 [details] fix the issue found by kling
Comment on attachment 93963 [details] fix the issue found by kling Clearing flags on attachment: 93963 Committed r86798: <http://trac.webkit.org/changeset/86798>
I'm a bit lost between bug 57472 and this one... What's still missing to have them closed?
I closed it :-)
Revision r86798 cherry-picked into qtwebkit-2.2 with commit fb119b5 <http://gitorious.org/webkit/qtwebkit/commit/fb119b5>