[webkit-reviews] review denied: [Bug 60161] [Qt] Virtual Keyboard is not hidden when you 'unfocus' from input field / QGraphicsWebview : [Attachment 112688] "Close software input panel on element blur" patch.

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Bruno Abinader
<bruno.de_oliveira at basyskom.de>'s request for review:
Bug 60161: [Qt] Virtual Keyboard is not hidden when you 'unfocus' from input
field / QGraphicsWebview
https://bugs.webkit.org/show_bug.cgi?id=60161

Attachment 112688: "Close software input panel on element blur" patch.
https://bugs.webkit.org/attachment.cgi?id=112688&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112688&action=review


I'm going to say r- because there are a few open questions here. Support for
QStyle::RSIP_OnMouseClick was removed and it's not clear why
QApplication::focusWidget() should be preferred over more specific item/widget
hasFocus() functions.

Also I don't see how this patch handles the case where you just "unblur" an
element within the page. AFAICS this patch only calls updateSoftwareInputPanel
from QWebPage::focusInEvent, which is certainly not called when a focusable
node within the document looses focus.

In the N9 we solved this through EditorClient::setInputMethodState, which
through a lengthy chain of calls should end up sending the RSIP _or_ the CSIP
event.

> Source/WebKit/qt/Api/qwebpage.cpp:-813
> -	   QStyle::RequestSoftwareInputPanel behavior =
QStyle::RequestSoftwareInputPanel(
> -	      
client->ownerWidget()->style()->styleHint(QStyle::SH_RequestSoftwareInputPanel)
);
> -	   if (!clickCausedFocus || behavior == QStyle::RSIP_OnMouseClick) {

So _this_ is the part that your patch removes entirely, the support for
QStyle::RSIP_OnMouseClick. Why did you remove it?

> Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:196
> +    QWidget* focusedWidget = QApplication::focusWidget();
> +
> +    if (!focusedWidget)
> +	   return;
> +

That doesn't look quite right. Wouldn't it be cleaner to check for
view->hasFocus() and also send the RSIP/CSIP events to view?

> Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:413
> +    QGraphicsView* focusedGraphicsView =
qobject_cast<QGraphicsView*>(QApplication::focusWidget());
> +
> +    if (focusedGraphicsView
> +	   && view->hasFocus()

Similarly here I think it would be better to use QGraphicsItem::hasFocus()
instead of using QApplication::focusWidget().

> Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:435
> +    if (enable)
> +	   view->setFlag(QGraphicsItem::ItemAcceptsInputMethod, true);
> +
> +    updateSoftwareInputPanel(enable);
> +
> +    if (!enable)
> +	   view->setFlag(QGraphicsItem::ItemAcceptsInputMethod, false);

A comment here (as well as in the other method) would probably help to
understand why
you enable before calling updateSoftwareInputPanel and disable afterwards.