Bug 57472

Summary: [Qt]REGRESSION(r82243): fast/events/onsearch-enter.html fails
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, commit-queue, eric, kling, max.hong.shen, menard, tonikitoo, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 58144    
Bug Blocks: 33179    
Attachments:
Description Flags
first try none

Description Csaba Osztrogonác 2011-03-30 10:27:58 PDT
It seems the event fires twice:

--- /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/layout-test-results/fast/events
/onsearch-enter-expected.txt	2011-03-29 10:26:35.556752666 -0700
+++ /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/layout-test-results/fast/events/onsearch-enter-actual.txt	2011-03-29 10:26:35.556752666 -0700
@@ -1,4 +1,5 @@
 This tests that onSearch fires correctly.
 
 Test Passed
+Test Passed
Comment 1 Csaba Osztrogonác 2011-03-30 11:23:51 PDT
I added it to the skipped list: http://trac.webkit.org/changeset/82476
Comment 2 Yi Shen 2011-04-07 14:36:47 PDT
Created attachment 88701 [details]
first try

The problem is that EditorClientQt::handleInputMethodKeydown() is invoked by both keydown event and keypress event, and inserts two new lines (and sends onsearch events twice in this bug). One simple solution is by checking defaultPrevented() for each event before trigger the insertnewline action.
Comment 3 WebKit Commit Bot 2011-04-08 08:24:28 PDT
Comment on attachment 88701 [details]
first try

Clearing flags on attachment: 88701

Committed r83295: <http://trac.webkit.org/changeset/83295>
Comment 4 WebKit Commit Bot 2011-04-08 08:24:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 WebKit Review Bot 2011-04-08 09:18:53 PDT
http://trac.webkit.org/changeset/83295 might have broken Qt Linux Release
The following tests are not passing:
fast/forms/button-enter-click.html
fast/forms/enter-clicks-buttons.html
fast/forms/implicit-submission.html
fast/forms/select-double-onchange.html
Comment 6 Antonio Gomes 2011-04-08 09:46:52 PDT
(In reply to comment #5)
> http://trac.webkit.org/changeset/83295 might have broken Qt Linux Release
> The following tests are not passing:
> fast/forms/button-enter-click.html
> fast/forms/enter-clicks-buttons.html
> fast/forms/implicit-submission.html
> fast/forms/select-double-onchange.html

Rolling it out for offline fix.
Comment 7 Yi Shen 2011-04-08 10:54:38 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > http://trac.webkit.org/changeset/83295 might have broken Qt Linux Release
> > The following tests are not passing:
> > fast/forms/button-enter-click.html
> > fast/forms/enter-clicks-buttons.html
> > fast/forms/implicit-submission.html
> > fast/forms/select-double-onchange.html
> 
> Rolling it out for offline fix.

Sorry for it. I am looking at the issues.
Comment 8 Yi Shen 2011-04-10 07:32:33 PDT
This regression is caused by the patch launched in https://bugs.webkit.org/show_bug.cgi?id=33179, and I doubt that patch is correct.

First, I saw a problem in the unit test for that patch, in which it tries to insert a new line by sending a enter key event

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

This is incorrect in Webkit, in order to adding a new line, we should add a text for the key event as below,

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

Secondly, the issue in bug 33179 is a symbian platform only, which can't be reproduced on Linux. While, the patch impacts all the platforms and causes this regression.

I will take a look at the bug 33179 on symbian to see whether there is another way to fix it.
Comment 9 Antonio Gomes 2011-04-10 09:00:51 PDT
(In reply to comment #8)
> This regression is caused by the patch launched in https://bugs.webkit.org/show_bug.cgi?id=33179, and I doubt that patch is correct.
> 
> First, I saw a problem in the unit test for that patch, in which it tries to insert a new line by sending a enter key event
> 
>     QKeyEvent keyEnter(QEvent::KeyPress, Qt::Key_Enter, Qt::NoModifier);
>     page->event(&keyEnter);
> 
> This is incorrect in Webkit, in order to adding a new line, we should add a text for the key event as below,
> 
>     QKeyEvent keyEnter(QEvent::KeyPress, Qt::Key_Enter, Qt::NoModifier, "\n");
>     page->event(&keyEnter);
> 
> Secondly, the issue in bug 33179 is a symbian platform only, which can't be reproduced on Linux. While, the patch impacts all the platforms and causes this regression.
> 
> I will take a look at the bug 33179 on symbian to see whether there is another way to fix it.

please comment in that bug, if you have not yet. It sounds really bad.
Comment 10 Yi Shen 2011-04-11 07:34:55 PDT
(In reply to comment #2)
> Created an attachment (id=88701) [details]
> first try
> 
> The problem is that EditorClientQt::handleInputMethodKeydown() is invoked by both keydown event and keypress event, and inserts two new lines (and sends onsearch events twice in this bug). One simple solution is by checking defaultPrevented() for each event before trigger the insertnewline action.

Please ignore the comment above, which is not completely right. I will put some comment in https://bugs.webkit.org/show_bug.cgi?id=33179, which is the one that caused this regression.
Comment 11 Ademar Reis 2011-05-19 13:18:20 PDT
(In reply to comment #10)
> Please ignore the comment above, which is not completely right. I will put some comment in https://bugs.webkit.org/show_bug.cgi?id=33179, which is the one that caused this regression.

Should this bug still be open then? (Bug 33179, which depends on this one, is closed already).
Comment 12 Ademar Reis 2011-05-19 13:19:18 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Please ignore the comment above, which is not completely right. I will put some comment in https://bugs.webkit.org/show_bug.cgi?id=33179, which is the one that caused this regression.
> 
> Should this bug still be open then? (Bug 33179, which depends on this one, is closed already).

Duh, actually the bug is still open, but all patches have been landed. /me lost :-)
Comment 13 Yi Shen 2011-05-19 14:35:31 PDT
Close this bug which is fixed in 33179