Bug 31401 – [Qt] Input method support remains enabled on QWebView/QGraphicsWebView after loading a new page

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

[Janne Koskinen]

Created attachment 43054 [details]
screenshot of text entering

Entering text into input field in www.google.com via FEP (virtual keyboard) in 5800 Express Music / Nokia N97 the autocompletion is brought up.
When selecting a search phrase from autocompletion field the inputMethoState is not updated correctly and is left enabled.
Any consecutive click to anywhere on screen will automatically start the FEP, not allowing user to interact with the webpage.

In attached emulator screen shot:
after closing the FEP and selecting "webkit bugs" clicking anywhere on the screen will bring FEP back up and can't be gotten rid of.

[Janne Koskinen]

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

[Janne Koskinen]

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

[Antti Koivisto]

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?

[Janne Koskinen]

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

[Eric Seidel (no email)]

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.

[Janne Koskinen]

Created attachment 43358 [details]
proposed fix for FrameLoaderClientQt.cpp

This patch tries address the issue by resetting the state at page load.

[Kenneth Rohde Christiansen]

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?

[Janne Koskinen]

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.

[Kenneth Rohde Christiansen]

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?

[Simon Hausmann]

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

[Janne Koskinen]

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

[Kenneth Rohde Christiansen]

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.

[Eric Seidel (no email)]

Comment on attachment 43361 [details]
proposed fix for FrameLoaderClientQt.cpp

janne isn't a committer, so marking cq+

[Janne Koskinen]

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

[Simon Hausmann]

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.

[Simon Hausmann]

Created attachment 43981 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 43981 [details]
Patch

Clearing flags on attachment: 43981

Committed r51458: <http://trac.webkit.org/changeset/51458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Janne Koskinen]

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.

[Janne Koskinen]

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.

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

[Jocelyn Turcotte]

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