Bug 31401 - [Qt] Input method support remains enabled on QWebView/QGraphicsWebView after loading a new page
Summary: [Qt] Input method support remains enabled on QWebView/QGraphicsWebView after ...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware Other
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-11-12 03:23 PST by Janne Koskinen
Modified: 2014-02-03 03:13 PST (History)
8 users (show)

See Also:


Attachments
screenshot of text entering (84.08 KB, image/png)
2009-11-12 03:23 PST, Janne Koskinen
no flags Details
proposed fix for focuscontroller.cpp (1.33 KB, patch)
2009-11-12 04:17 PST, Janne Koskinen
eric: review-
Details | Formatted Diff | Diff
proposed fix for FrameLoaderClientQt.cpp (2.87 KB, patch)
2009-11-17 05:51 PST, Janne Koskinen
kenneth: review-
Details | Formatted Diff | Diff
proposed fix for FrameLoaderClientQt.cpp (2.90 KB, patch)
2009-11-17 07:02 PST, Janne Koskinen
no flags Details | Formatted Diff | Diff
Patch (2.54 KB, patch)
2009-11-28 02:33 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Janne Koskinen 2009-11-12 03:23:01 PST
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.
Comment 1 Janne Koskinen 2009-11-12 03:31:12 PST
Bug can be reproduced using QWebView and QGraphicsWebView. fix to follow.
Comment 2 Janne Koskinen 2009-11-12 04:17:59 PST
Created attachment 43056 [details]
proposed fix for focuscontroller.cpp
Comment 3 Antti Koivisto 2009-11-12 10:11:51 PST
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?
Comment 4 Janne Koskinen 2009-11-13 07:10:06 PST
(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 5 Eric Seidel (no email) 2009-11-13 13:42:02 PST
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.
Comment 6 Janne Koskinen 2009-11-17 05:51:37 PST
Created attachment 43358 [details]
proposed fix for FrameLoaderClientQt.cpp

This patch tries address the issue by resetting the state at page load.
Comment 7 Kenneth Rohde Christiansen 2009-11-17 06:00:43 PST
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?
Comment 8 Janne Koskinen 2009-11-17 07:02:10 PST
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.
Comment 9 Kenneth Rohde Christiansen 2009-11-17 07:20:02 PST
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?
Comment 10 Simon Hausmann 2009-11-17 07:39:36 PST
(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.
Comment 11 Janne Koskinen 2009-11-17 07:53:11 PST
(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 12 Kenneth Rohde Christiansen 2009-11-17 09:36:45 PST
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 13 Eric Seidel (no email) 2009-11-17 18:14:50 PST
Comment on attachment 43361 [details]
proposed fix for FrameLoaderClientQt.cpp

janne isn't a committer, so marking cq+
Comment 14 Janne Koskinen 2009-11-18 04:55:05 PST
Set - for commit queue , this breaks node focusing for first time FEP launching.
Comment 15 Simon Hausmann 2009-11-28 02:32:07 PST
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.
Comment 16 Simon Hausmann 2009-11-28 02:33:36 PST
Created attachment 43981 [details]
Patch
Comment 17 WebKit Commit Bot 2009-11-28 06:27:50 PST
Comment on attachment 43981 [details]
Patch

Clearing flags on attachment: 43981

Committed r51458: <http://trac.webkit.org/changeset/51458>
Comment 18 WebKit Commit Bot 2009-11-28 06:27:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Janne Koskinen 2009-12-02 01:07:35 PST
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.
Comment 20 Janne Koskinen 2009-12-02 01:16:00 PST
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.
Comment 21 Tor Arne Vestbø 2010-03-10 06:37:51 PST
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
Comment 22 Jocelyn Turcotte 2014-02-03 03:13:09 PST
=== 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.