Bug 36292 – [Qt] The numbers are not displayed properly when entering through VKB in number mode

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Kristian Amlie]

Created attachment 51020 [details]
Patch

Steps to reproduce: 

1. Compile this program:
int main(int argc, char **argv)
{
    QApplication app(argc, argv);
    QWidget w;

    QLayout *layout = new QVBoxLayout;

    QGraphicsScene scene;
    QGraphicsView view(&scene);
    QGraphicsWebView *wv = new QGraphicsWebView;
    wv->load(QUrl("http://www.google.com/"));
    scene.addItem(wv);
    layout->addWidget(&view);

    w.setLayout(layout);
    w.showMaximized();
    return app.exec();
}

2. Tap the lineedit so the input panel opens.
3. Enter number mode.
4. Try to enter numbers.

The result is that the numbers lag behind, that is, when entering "23", the first character will produce nothing, and the 3 will produce "2". Must be a QInputContext::update issue, since the content is correct in the widget itself.

[Kristian Amlie]

Comment on attachment 51020 [details]
Patch

> diff --git a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp
> index ceb5ee1..5786965 100644
> --- a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp
> +++ b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp
> @@ -30,6 +30,7 @@
>  #include <QtGui/qapplication.h>
>  #include <QtGui/qgraphicssceneevent.h>
>  #include <QtGui/qstyleoption.h>
> +#include <QtGui/qinputcontext.h>
>  #if defined(Q_WS_X11)
>  #include <QX11Info>
>  #endif
> @@ -63,6 +64,8 @@ public:
>  
>      void _q_doLoadFinished(bool success);
>  
> +    void _q_updateMicroFocus();
> +
>      QGraphicsWebView* q;
>      QWebPage* page;
>  };
> @@ -80,6 +83,18 @@ void QGraphicsWebViewPrivate::_q_doLoadFinished(bool success)
>      emit q->loadFinished(success);
>  }
>  
> +void QGraphicsWebViewPrivate::_q_updateMicroFocus()
> +{
> +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> +    QList<QGraphicsView *> views = q->scene()->views();
> +    for (int c = 0; c < views.size(); ++c) {
> +        QInputContext *ic = views[c]->inputContext();
> +        if (ic)
> +            ic->update();
> +    }
> +#endif
> +}
> +
>  void QGraphicsWebViewPrivate::scroll(int dx, int dy, const QRect& rectToScroll)
>  {
>      q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> @@ -435,6 +450,8 @@ void QGraphicsWebView::setPage(QWebPage* page)
>              this, SIGNAL(statusBarMessage(QString)));
>      connect(d->page, SIGNAL(linkClicked(QUrl)),
>              this, SIGNAL(linkClicked(QUrl)));
> +    connect(d->page, SIGNAL(microFocusChanged()),
> +            this, SLOT(_q_updateMicroFocus()));
>  }
>  
>  /*!
> diff --git a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h
> index f3afb4c..f983ae4 100644
> --- a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h
> +++ b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h
> @@ -134,6 +134,7 @@ protected:
>  
>  private:
>      Q_PRIVATE_SLOT(d, void _q_doLoadFinished(bool success))
> +    Q_PRIVATE_SLOT(d, void _q_updateMicroFocus())
>  
>      QGraphicsWebViewPrivate* const d;
>      friend class QGraphicsWebViewPrivate;

[Simon Hausmann]

Comment on attachment 51020 [details]
Patch

I think in principle this looks good, but I can see two things missing:

* A ChangeLog entry
* A comment indicating that this is a missing feature in QGraphicsItem, ideally with a link to an entry in the Qt JIRA tracking this missing feature. Then we can remove this code again once it's fixed in Qt.

> +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> +    QList<QGraphicsView *> views = q->scene()->views();

Coding style, no space before the '*'

> +    for (int c = 0; c < views.size(); ++c) {
> +        QInputContext *ic = views[c]->inputContext();

I think you may want to use views.at(c) instead of [], to avoid detaching the QList that the scene returns.

> +        if (ic)
> +            ic->update();

Shouldn't this be a call to views.at(c)->updateMicroFocus() instead?

[Kristian Amlie]

(In reply to comment #2)
> * A comment indicating that this is a missing feature in QGraphicsItem, ideally
> with a link to an entry in the Qt JIRA tracking this missing feature. Then we
> can remove this code again once it's fixed in Qt.

So what you are talking about is basically to have QGraphicsItem::updateMicroFocus()? I guess that makes sense, although it can't be a slot, because QGraphicsItem is not a QObject, so we would still need the extra slot in QGraphicsWebView.

QGraphicsTextItem currently also solves this by calling ic->update(), although it looks at qApp->focusWidget() only, which might be smarter.

> > +    for (int c = 0; c < views.size(); ++c) {
> > +        QInputContext *ic = views[c]->inputContext();
> 
> I think you may want to use views.at(c) instead of [], to avoid detaching the
> QList that the scene returns.

I don't think it detaches as long as the called function is also const, but in the sake of clarity, you're right. :-)

> > +        if (ic)
> > +            ic->update();
> 
> Shouldn't this be a call to views.at(c)->updateMicroFocus() instead?

Would be nice, but updateMicroFocus() is protected.

[Simon Hausmann]

(In reply to comment #3)
> (In reply to comment #2)
> > * A comment indicating that this is a missing feature in QGraphicsItem, ideally
> > with a link to an entry in the Qt JIRA tracking this missing feature. Then we
> > can remove this code again once it's fixed in Qt.
> 
> So what you are talking about is basically to have
> QGraphicsItem::updateMicroFocus()? I guess that makes sense, although it can't
> be a slot, because QGraphicsItem is not a QObject, so we would still need the
> extra slot in QGraphicsWebView.

Right. It could be turned into a slot in QGraphicsObject, btw. Then we could also use it. But either way I think this should be filed in the Qt bug tracker and we should reference it here.

> QGraphicsTextItem currently also solves this by calling ic->update(), although
> it looks at qApp->focusWidget() only, which might be smarter.

That's a good argument in favour of fixing it really in QGraphicsItem :-)
 
> > > +    for (int c = 0; c < views.size(); ++c) {
> > > +        QInputContext *ic = views[c]->inputContext();
> > 
> > I think you may want to use views.at(c) instead of [], to avoid detaching the
> > QList that the scene returns.
> 
> I don't think it detaches as long as the called function is also const, but in
> the sake of clarity, you're right. :-)

The variable is local, so why would the compiler choose the non-const [] overload? The constness of the currect function doesn't change that.
 
> > > +        if (ic)
> > > +            ic->update();
> > 
> > Shouldn't this be a call to views.at(c)->updateMicroFocus() instead?
> 
> Would be nice, but updateMicroFocus() is protected.

Dang! :)

[Simon Hausmann]

This is tracked upstream at http://bugreports.qt.nokia.com/browse/QTBUG-7578 . Kristian, can you add a link to that as a comment in the code?

[Simon Hausmann]

*** Bug 33836 has been marked as a duplicate of this bug. ***

[Kristian Amlie]

(In reply to comment #4)
> (In reply to comment #3)
> > QGraphicsTextItem currently also solves this by calling ic->update(), although
> > it looks at qApp->focusWidget() only, which might be smarter.
> 
> That's a good argument in favour of fixing it really in QGraphicsItem :-)

Indeed. I'll leave that in the task you mentioned for now, since this needs to go into 4.6 as well.

> > > > +    for (int c = 0; c < views.size(); ++c) {
> > > > +        QInputContext *ic = views[c]->inputContext();
> > > 
> > > I think you may want to use views.at(c) instead of [], to avoid detaching the
> > > QList that the scene returns.
> > 
> > I don't think it detaches as long as the called function is also const, but in
> > the sake of clarity, you're right. :-)
> 
> The variable is local, so why would the compiler choose the non-const []
> overload? The constness of the currect function doesn't change that.

You're right, I was assuming that inputContext() was a const function, but it isn't, so the compiler would indeed choose the non-const operator[].

I'll get a modified patch up shortly.

[Kristian Amlie]

Created attachment 51142 [details]
Patch v2

[Simon Hausmann]

Comment on attachment 51142 [details]
Patch v2


> +void QGraphicsWebViewPrivate::_q_updateMicroFocus()
> +{
> +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> +    // Ideally, this should be handled by a common call to an updateMicroFocus function
> +    // in QGraphicsItem. See http://bugreports.qt.nokia.com/browse/QTBUG-7578.

Why is this only enabled for X11/QWS/Symbian?

[Kristian Amlie]

(In reply to comment #9)
> (From update of attachment 51142 [details])
> 
> > +void QGraphicsWebViewPrivate::_q_updateMicroFocus()
> > +{
> > +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> > +    // Ideally, this should be handled by a common call to an updateMicroFocus function
> > +    // in QGraphicsItem. See http://bugreports.qt.nokia.com/browse/QTBUG-7578.
> 
> Why is this only enabled for X11/QWS/Symbian?

I copied it from QWidget::updateMicroFocus().

[WebKit Commit Bot]

Comment on attachment 51142 [details]
Patch v2

Clearing flags on attachment: 51142

Committed r56322: <http://trac.webkit.org/changeset/56322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Cherry-picked into qtwebkit-4.6 with commit aa40cdb9595eb15a68e7be03322f973aa613a8f9