Bug 29681 - [Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods are incomplete
: [Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods...
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Qt
:
:
  Show dependency treegraph
 
Reported: 2009-09-23 06:57 PST by
Modified: 2009-10-01 05:43 PST (History)


Attachments
patch to describe the changes need to sync up the s60 input context (5.27 KB, patch)
2009-09-23 08:11 PST, Joseph Ligman
no flags Review Patch | Details | Formatted Diff | Diff
patch to describe the changes need to sync up the s60 input context (6.79 KB, patch)
2009-09-24 11:16 PST, Joseph Ligman
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch, input method handling (11.83 KB, patch)
2009-09-30 10:18 PST, Joseph Ligman
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Update patch with more fixes (13.59 KB, patch)
2009-10-01 05:20 PST, Kristian Amlie
hausmann: review+
Review Patch | Details | Formatted Diff | Diff
Patch which fixes software input panel (3.45 KB, patch)
2009-10-01 05:33 PST, Kristian Amlie
hausmann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-23 06:57:00 PST
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 From 2009-09-23 07:02:23 PST -------
Please describe the missing cases and how you would change impl/api to cover those
------- Comment #2 From 2009-09-23 08:11:42 PST -------
Created an attachment (id=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 From 2009-09-23 08:21:49 PST -------
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 From 2009-09-24 11:16:03 PST -------
Created an attachment (id=40075) [details]
patch to describe the changes need to sync up the s60 input context

Added a change log to the patch.
------- Comment #5 From 2009-09-25 04:06:54 PST -------
(From update of attachment 40075 [details])

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 From 2009-09-25 12:28:19 PST -------
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 From 2009-09-30 10:18:40 PST -------
Created an attachment (id=40382) [details]
Proposed patch,  input method handling

This patch attempts to address some of the missing cases in the input methods
------- Comment #8 From 2009-10-01 03:38:31 PST -------
(From update of attachment 40382 [details])
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 From 2009-10-01 05:20:19 PST -------
Created an attachment (id=40433) [details]
Update patch with more fixes
------- Comment #10 From 2009-10-01 05:33:26 PST -------
Created an attachment (id=40434) [details]
Patch which fixes software input panel
------- Comment #11 From 2009-10-01 05:42:03 PST -------
(From update of attachment 40433 [details])
There's tabs in the ChangeLog, but I'll fix those before landing.
------- Comment #12 From 2009-10-01 05:43:59 PST -------
Landed in r48967 and r48968