Bug 29681

Summary: [Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods are incomplete
Product: WebKit Reporter: Joseph Ligman <joseph.ligman>
Component: WebKit QtAssignee: Joseph Ligman <joseph.ligman>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, kristian.amlie, laszlo.gombos, vestbo
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch to describe the changes need to sync up the s60 input context
none
patch to describe the changes need to sync up the s60 input context
hausmann: review-
Proposed patch, input method handling
hausmann: review-
Update patch with more fixes
hausmann: review+
Patch which fixes software input panel hausmann: review+

Description Joseph Ligman 2009-09-23 06:57:00 PDT
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.
Comment 1 Tor Arne Vestbø 2009-09-23 07:02:23 PDT
Please describe the missing cases and how you would change impl/api to cover those
Comment 2 Joseph Ligman 2009-09-23 08:11:42 PDT
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
Comment 3 Joseph Ligman 2009-09-23 08:21:49 PDT
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.
Comment 4 Joseph Ligman 2009-09-24 11:16:03 PDT
Created attachment 40075 [details]
patch to describe the changes need to sync up the s60 input context

Added a change log to the patch.
Comment 5 Simon Hausmann 2009-09-25 04:06:54 PDT
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?
Comment 6 Joseph Ligman 2009-09-25 12:28:19 PDT
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
Comment 7 Joseph Ligman 2009-09-30 10:18:40 PDT
Created attachment 40382 [details]
Proposed patch,  input method handling

This patch attempts to address some of the missing cases in the input methods
Comment 8 Simon Hausmann 2009-10-01 03:38:31 PDT
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 :-)
Comment 9 Kristian Amlie 2009-10-01 05:20:19 PDT
Created attachment 40433 [details]
Update patch with more fixes
Comment 10 Kristian Amlie 2009-10-01 05:33:26 PDT
Created attachment 40434 [details]
Patch which fixes software input panel
Comment 11 Simon Hausmann 2009-10-01 05:42:03 PDT
Comment on attachment 40433 [details]
Update patch with more fixes

There's tabs in the ChangeLog, but I'll fix those before landing.
Comment 12 Simon Hausmann 2009-10-01 05:43:59 PDT
Landed in r48967 and r48968