Bug 31401

Bug can be reproduced using QWebView and QGraphicsWebView. fix to follow.

Created attachment 43056 [details] proposed fix for focuscontroller.cpp

Comment on attachment 43056 [details] proposed fix for focuscontroller.cpp > Node* oldFocusedNode = oldDocument ? oldDocument->focusedNode() : 0; > - if (oldFocusedNode == node) > + if (node && oldFocusedNode == node) > return true; So this affects the case where the focused node was already null. Why didn't inputMethodState get cleared at the time it became null?

(In reply to comment #3) > So this affects the case where the focused node was already null. Why didn't > inputMethodState get cleared at the time it became null? Input focus is not resetted when new page is loaded. I'll try to figure out if there is a way to reset it. I'll get back on this on monday.

Comment on attachment 43056 [details] proposed fix for focuscontroller.cpp Can this be tested? r- for lack of test, or lack of explanation as to why it can't be tested. I don't really understand what this fix is trying to do, and testing would help a reviewer understand the effect of this change.

Created attachment 43358 [details] proposed fix for FrameLoaderClientQt.cpp This patch tries address the issue by resetting the state at page load.

Comment on attachment 43358 [details] proposed fix for FrameLoaderClientQt.cpp Still no tests, nor explaination why it is not testable. > @@ -57,6 +57,7 @@ > #include "ScriptString.h" > #include "Settings.h" > #include "QWebPageClient.h" > +#include "EditorClient.h" Did you run check-webkit-style. These should be ordered. > + if (m_frame) { > + if (m_frame->editor() && m_frame->editor()->client()) > + m_frame->editor()->client()->setInputMethodState(false); > + > + // send a mousemove event to > + // (1) update the cursor to change according to whatever is underneath the mouse cursor right now > + // (2) display the tool tip if the mouse hovers a node which has a tool tip > + if (m_frame->eventHandler() && m_webFrame->page()) { > + QWidget* view = m_webFrame->page()->view(); So this is a fix for QWebView only and not for QGraphicsWebView. Is a similar fix needed, if so, why not fix it right away?

Created attachment 43361 [details] proposed fix for FrameLoaderClientQt.cpp sort order fixed. Test case to follow, it might take a few months to make though if everything is covered. I'll see if could do some shortcuts. For the question of QGraphicsWebView and QWebView. Patch does address both scenarios and FEP works as expected for this case. There are still several FEP related issues with Symbian port which will need closer look. I'll set r? once automated test environment built for this.

I would say that if you are committed to adding test cases, and as this is an important fix, it would be OK for me to review it already. I just need to understand how this fix works for QGraphicsWebView, because in that case the page will have a view() equal to 0 (unless set to something else). Shouldn't you be using the pageClient instead?

(In reply to comment #9) > I would say that if you are committed to adding test cases, and as this is an > important fix, it would be OK for me to review it already. > > I just need to understand how this fix works for QGraphicsWebView, because in > that case the page will have a view() equal to 0 (unless set to something > else). > > Shouldn't you be using the pageClient instead? The use of the EditorClient for setInputMethodState is correct. There's a bit of extra logic before it passes on to the PageClient. But the unrelated code below for the mouse event to update the cursor after a load, that needs to be ported to pageclient ;-) I still wonder how the others port do it though, when/how they reset the state of the input method when loading a new page.

(In reply to comment #9) >I would say that if you are committed to adding test cases, and as this is an >important fix, it would be OK for me to review it already. Without the fix user will be stuck on Google front page on S60 touch devices. I do understand the importance of tests and I do not understand why other platforms with virtual keyboards are not suffering from this issue, so I would like to create a test case for this. With testing on Symbian we have a lot of issues e.g. there is no device with enough memory to run dumprendertree. So if done properly testing has to be started by constructing such a device and baseport Symbian on it :) But as I said it can be shortcutted. > I just need to understand how this fix works for QGraphicsWebView, because in > that case the page will have a view() equal to 0 (unless set to something > else). Ah you might be reading the wrong part. my fix lines are: + if (m_frame->editor() && m_frame->editor()->client()) + m_frame->editor()->client()->setInputMethodState(false); Rest of it is due to new indentation from moving if(m_frame) to cover the whole block.

Comment on attachment 43361 [details] proposed fix for FrameLoaderClientQt.cpp Feel free to commit it, but please open two other bug reports to track the missing test cases, plus the code change to use pageClient.

Comment on attachment 43361 [details] proposed fix for FrameLoaderClientQt.cpp janne isn't a committer, so marking cq+

Set - for commit queue , this breaks node focusing for first time FEP launching.

From the user's perspective the bug is that when using the input method to enter text in one page and then navigating to a new page, any following click in the new page will bring up the software input panel.

Created attachment 43981 [details] Patch

Comment on attachment 43981 [details] Patch Clearing flags on attachment: 43981 Committed r51458: <http://trac.webkit.org/changeset/51458>

All reviewed patches have been landed. Closing bug.

This doesn't fix the issue. It tries to treat the symptoms and fails at that on devices where you have keypad. With the patch inputfocus remains on previous page's control until user clicks on a control that doesn't have inputmethods. With devices that have keypad you can type in text when you have inputfocus so in this case user is able to type in chars to non-existent control and is indicated to do so by UI showin "abc" - indicator on screen. Further the patch introduces a bug where FEP is launched on page change even if the user never started such event but merely used HW keyboard to type in the search query.

Reopening to fix the underlying issues. Patch that has already landed can be left to trunk as this is a good patch in any case. The device to test this is Nokia N97 where you have both touch screen and keypad. Unfortunately I don't own one and I wouldn't want to rely solely on emulator environment.

Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should only 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 - Add the keyword 'Qt' to signal that it's a Qt-related bug http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

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