Bug 29681 – [Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods are incomplete

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Joseph Ligman]

These methods are used to sync the virtual keyboard on s60 devices through the fep input context. There are a few cases missing in the query and event methods.

[Tor Arne Vestbø]

Please describe the missing cases and how you would change impl/api to cover those

[Joseph Ligman]

Created attachment 39997 [details]
patch to describe the changes need to sync up the s60 input context

patch to describe missing s60 cases from inputMethodQuery, inputMethodEvent

[Joseph Ligman]

The patch attempts to address these issue for s60:

QWebPage::inputMethodQuery
Qt::ImAnchorPosition is not implemented.
Qt::ImCursorPosition returns the wrong position.
Qt::ImSurroundingText returns an empty string.

QWebPagePrivate::inputMethodEvent
The QInputMethodEvent::Selection position is not set for input areas.

[Joseph Ligman]

Created attachment 40075 [details]
patch to describe the changes need to sync up the s60 input context

Added a change log to the patch.

[Simon Hausmann]

Comment on attachment 40075 [details]
patch to describe the changes need to sync up the s60 input context


In principle this looks good, it's great that you're working on fixing the input method handling!

I have a few minor issues (see below) and I'm also wondering if there's anything we can do
to make this testable.

How about unit tests that programmatically set the focus, select text, etc. and then call inputMethodQuery() 
or send synthetic input method events to verify this code?

> +
> +#if PLATFORM(SYMBIAN)
> +    for (int i = 0; i < ev->attributes().size(); ++i) {
> +        const QInputMethodEvent::Attribute &a = ev->attributes().at(i);
> +        if (a.type == QInputMethodEvent::Selection) {
> +            RenderObject* renderer = 0;
> +            if (frame->selection()->rootEditableElement())
> +                renderer = frame->selection()->rootEditableElement()->shadowAncestorNode()->renderer();
> +
> +            if (renderer && renderer->isTextControl()) {
> +                toRenderTextControl(renderer)->setSelectionStart(a.start);
> +                toRenderTextControl(renderer)->setSelectionEnd(a.start + a.length);
> +            }
> +            break;
> +        };
>      }
> +#endif

Why is this code limited to PLATFORM(SYMBIAN)?
  

> @@ -1196,42 +1215,64 @@ bool QWebPagePrivate::handleScrolling(QK
[...]
>
> +    switch (property) { 
> +        case Qt::ImMicroFocus: { 

Looks like the indentation is off here :)

> +             return QVariant(frame->selection()->absoluteCaretBounds()); 
> +         return QVariant(); 
> +     } 
> +     case Qt::ImFont: { 
> +         QWebView *webView = qobject_cast<QWebView *>(d->view); 
> +         if (webView) 
> +             return QVariant(webView->font()); 
> +         return QVariant(); 

This doesn't look correct. font() is a method of QWidget, it should be enough to
simply use return d->view ? d->view->font() : QFont();

> +#if PLATFORM(SYMBIAN)
> +     case Qt::ImAnchorPosition: {
> +        if (renderTextControl) {
> +            if (editor->hasComposition()) {
> +                PassRefPtr<Range> range = editor->compositionRange();
> +                return QVariant(renderTextControl->selectionStart() - TextIterator::rangeLength(range.get()));
> +            }
> +            return QVariant(renderTextControl->selectionStart());
>          }
>          return QVariant();
>      }
> -    case Qt::ImSurroundingText: {
> -        Frame *frame = d->page->focusController()->focusedFrame();
> -        if (frame) {
> -            Document *document = frame->document();
> -            if (document->focusedNode())
> -                return QVariant(document->focusedNode()->nodeValue());
> +#endif

Same as earlier hunk, why is this only enabled for Symbian?

[Joseph Ligman]

Thanks for the review. 
I will add the missing test cases and remove the PLATFORM(SYMBIAN) flag. 

I see if I can cover the cases in the 4.6 documentation:
InputMethodQuery:
Qt::ImMicroFocus, Qt::ImFont, Qt::ImCursorPosition, Qt::ImSurroundingText, Qt::ImCurrentSelection, Qt::ImMaximumTextLength, Qt::ImAnchorPosition

InputMethodEvent:
QInputMethodEvent::TextFormat, QInputMethodEvent::Cursor, QInputMethodEvent::Language, QInputMethodEvent::Ruby, QInputMethodEvent::Selection

[Joseph Ligman]

Created attachment 40382 [details]
Proposed patch,  input method handling

This patch attempts to address some of the missing cases in the input methods

[Simon Hausmann]

Comment on attachment 40382 [details]
Proposed patch,  input method handling

Thanks for the update and a big thanks for adding the good automated tests!

I've taken another look at the patch and I've also tried it out with Kristian (CCed).

There are a few things that need to be fixed as commented below. Kristian is going to upload
a revised version of your patch later with a few small fixes.


> +Q_DECLARE_METATYPE(QTextCharFormat)
[...]
> +            case QInputMethodEvent::TextFormat: {
> +                QTextCharFormat textCharFormat = a.value.value<QTextCharFormat>();
> +                QColor qcolor = textCharFormat.underlineColor();

This sequence can be simplified a bit. The text format stored in the QVariant is _actually_ a QTextFormat. QVariant has native
support for storing these. So the Q_DECLARE_METATYPE can be removed and instead the code should look like this:

    QTextCharFormat textCharFormat = a.value.value<QCharFormat>().toCharFormat();

A coding style issue I noticed: The case portions of the switch() statement should be indented at the same level
as the switch(). I recommend the use of WebKitTools/Scripts/check-webkit-style to verify the coding style after
a change :-)


>  QVariant QWebPage::inputMethodQuery(Qt::InputMethodQuery property) const
[...]
>  {
> +    Frame *frame = d->page->focusController()->focusedFrame();
[...]
> +    WebCore::Editor *editor = frame->editor();

* placement :)


> +        case Qt::ImCursorPosition: {
> +            if (renderTextControl) {
> +                if (editor->hasComposition()) {
> +                    PassRefPtr<Range> range = editor->compositionRange();

This should be a RefPtr, not a PassRefPtr. Same for the other cases below.

> +                    return QVariant(renderTextControl->selectionEnd() - TextIterator::rangeLength(range.get()));
> +                }
> +                return QVariant(renderTextControl->selectionEnd());
> +            }
> +            return QVariant();
> +        }
> +        case Qt::ImSurroundingText: {
> +            if (renderTextControl)
> +                return QVariant(renderTextControl->text());
> +            return QVariant();
> +        }
> +        case Qt::ImCurrentSelection: {
> +            if (renderTextControl) {
> +                int start = renderTextControl->selectionStart();
> +                int end = renderTextControl->selectionEnd();
> +                if (end > start)
> +                    return QVariant(QString(renderTextControl->text()).mid(start,end-start));
> +            }
> +            return QVariant();
> +
> +        }
> +#if QT_VERSION >= 0x040502

This should check for Qt version >= 4.6 instead of 4.5.2, otherwise the compilation
on non-S60 platforms will break.


Another issue that Kristian noticed is that the query for ImSurroundingText should _exclude_ the preedit text.

We'll see about getting that fixed in a revised patch :-)

[Kristian Amlie]

Created attachment 40433 [details]
Update patch with more fixes

[Kristian Amlie]

Created attachment 40434 [details]
Patch which fixes software input panel

[Simon Hausmann]

Comment on attachment 40433 [details]
Update patch with more fixes

There's tabs in the ChangeLog, but I'll fix those before landing.

[Simon Hausmann]

Landed in r48967 and r48968