Bug 88662 – [Qt][WK2] Only show virtual keyboard as a result of user interaction.

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Yael]

The ASSERT in QtViewportHandler::focusEditableArea is wrong.
An editable element can be focused when JavaScript focuses it.
I hit this ASSERT when loading e.g. www.google.com in MiniBrowser.

It was added in r118493.

[Yael]

Created attachment 146583 [details]
Patch

Remove the ASSERT.
Input elements can become focused via JavaScript, and not only from user interaction.

[Kenneth Rohde Christiansen]

Comment on attachment 146583 [details]
Patch

Just like on iOS and N9 etc we only want to allow bringing up the vkb and zooming in when the element got focused as a result of user interaction. There is code to make sure of that and that is the reason for this assert. If that doesn't work correctly it should be fixed. Also, it would be good with qml tests for this.

[Yael]

(In reply to comment #2)
> (From update of attachment 146583 [details])
> Just like on iOS and N9 etc we only want to allow bringing up the vkb and zooming in when the element got focused as a result of user interaction. There is code to make sure of that and that is the reason for this assert. If that doesn't work correctly it should be fixed. Also, it would be good with qml tests for this.

Where is the code that prevents bringing VKB?
It seems to me very backwards to add ASSERTS without making sure that the code works.

[Chang Shu]

Would "if (!m_hadUserInteraction) return;" help? :)

[Kenneth Rohde Christiansen]

Zalan can you look into this?

[alan]

Some code got moved around recently and now JS could bring up vkb indeed. Previously QtWebPageEventHandler::doneWithGestureEvent() triggered setInputPanelVisible(), now it is in this more generic updateTextInputState() method, which gets called by WebCore when EditorStateChange. 
see the change here: https://bugs.webkit.org/attachment.cgi?id=140006&action=diff

The code was probably mistakenly moved. I'll ask Michael tomorrow and make a patch based on that.

[Kenneth Rohde Christiansen]

Would be great with a qml test as well!

[Michael Brüning]

Created attachment 159400 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 159400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159400&action=review

> Source/WebKit2/ChangeLog:10
> +        [Qt][WK2] Remove wrong ASSERT
> +        https://bugs.webkit.org/show_bug.cgi?id=88662
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Rolled back a change to QtWebPageEventHandler::doneGestureEvent and implemented the correct behaviour.
> +        Also added a QML test to test whether the virtual keyboard is visible.
> +

I think that bug needs to be renamed or you need to create another one

[Michael Brüning]

Created attachment 159435 [details]
Patch

[Michael Brüning]

Update ChangeLog to new title.

[Kenneth Rohde Christiansen]

Comment on attachment 159435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159435&action=review

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_noVirtualKeyboardFromJS.qml:18
> +        function test_virtualKeyboardNotVisible() {
> +            var url = Qt.resolvedUrl("../common/focusInputElementFromJS.html")
> +
> +            webView.url = url

I want another test, like one click on a button and it calls js to focus something else which is editable... that should work

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:437
> +    // Ignore input method requests not due to a tap gesture.
> +    if (!editor.isContentEditable)
> +        setInputPanelVisible(false);

How is it doing that?

[Michael Brüning]

Created attachment 159439 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 159439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159439&action=review

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:437
>      qApp->inputMethod()->update(Qt::ImQueryInput | Qt::ImEnabled | Qt::ImHints);
>  
> -    setInputPanelVisible(editor.isContentEditable);
> +    // Ignore input method requests not due to a tap gesture.
> +    if (!editor.isContentEditable)
> +        setInputPanelVisible(false);

Do we need to update() the input method when we are setting the input panel to invisible?

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:454
> -    updateTextInputState();
> +    qApp->inputMethod()->update(Qt::ImQueryInput | Qt::ImEnabled | Qt::ImHints);
> +
> +    const EditorState& editor = m_webPageProxy->editorState();
> +    bool newVisible = editor.isContentEditable;
> +    setInputPanelVisible(newVisible);

Don't we need to update WebView's flag ItemAcceptsInputMethod?

Could the common code of updateTextInputState() and doneWithGestureEvent() be moved to a function that both call?

[Kenneth Rohde Christiansen]

Comment on attachment 159439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159439&action=review

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:435
> +    // Ignore input method requests not due to a tap gesture.

The comment is not so good because we do hide it.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:453
> +    qApp->inputMethod()->update(Qt::ImQueryInput | Qt::ImEnabled | Qt::ImHints);
> +
> +    const EditorState& editor = m_webPageProxy->editorState();
> +    bool newVisible = editor.isContentEditable;

The difference here from updateTextInputState is minimal, so either it needs a comment or you could give updateTextInputState an argument. Like why is it ok to ignore the postpone? or the hasActiveFocus?

[Michael Brüning]

(In reply to comment #15)
> (From update of attachment 159439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159439&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:435
> > +    // Ignore input method requests not due to a tap gesture.
> 
> The comment is not so good because we do hide it.

Actually, this comment was at that line in exactly the same wording for quite a while until my previous change (the one which caused the trouble with the assert) removed it. But I do agree that it does not really explain why we're hiding the thing, so I'll reword it when I post the refactoring...

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:453
> > +    qApp->inputMethod()->update(Qt::ImQueryInput | Qt::ImEnabled | Qt::ImHints);
> > +
> > +    const EditorState& editor = m_webPageProxy->editorState();
> > +    bool newVisible = editor.isContentEditable;
> 
> The difference here from updateTextInputState is minimal, so either it needs a comment or you could give updateTextInputState an argument. Like why is it ok to ignore the postpone? or the hasActiveFocus?

Hm, afaics, it's not ignoring any of those :). The postpone gets reset to false here because it is set by the single tap gesture and this is the callback / handler that gets called when this single tap gesture is done.

[Michael Brüning]

Comment on attachment 159439 [details]
Patch

Clearing flags, will post updated patch later.

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