WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
29681
[Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods are incomplete
https://bugs.webkit.org/show_bug.cgi?id=29681
Summary
[Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods...
Joseph Ligman
Reported
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.
Attachments
patch to describe the changes need to sync up the s60 input context
(5.27 KB, patch)
2009-09-23 08:11 PDT
,
Joseph Ligman
no flags
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 PDT
,
Joseph Ligman
hausmann
: review-
Details
Formatted Diff
Diff
Proposed patch, input method handling
(11.83 KB, patch)
2009-09-30 10:18 PDT
,
Joseph Ligman
hausmann
: review-
Details
Formatted Diff
Diff
Update patch with more fixes
(13.59 KB, patch)
2009-10-01 05:20 PDT
,
Kristian Amlie
hausmann
: review+
Details
Formatted Diff
Diff
Patch which fixes software input panel
(3.45 KB, patch)
2009-10-01 05:33 PDT
,
Kristian Amlie
hausmann
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Tor Arne Vestbø
Comment 1
2009-09-23 07:02:23 PDT
Please describe the missing cases and how you would change impl/api to cover those
Joseph Ligman
Comment 2
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
Joseph Ligman
Comment 3
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.
Joseph Ligman
Comment 4
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.
Simon Hausmann
Comment 5
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?
Joseph Ligman
Comment 6
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
Joseph Ligman
Comment 7
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
Simon Hausmann
Comment 8
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 :-)
Kristian Amlie
Comment 9
2009-10-01 05:20:19 PDT
Created
attachment 40433
[details]
Update patch with more fixes
Kristian Amlie
Comment 10
2009-10-01 05:33:26 PDT
Created
attachment 40434
[details]
Patch which fixes software input panel
Simon Hausmann
Comment 11
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.
Simon Hausmann
Comment 12
2009-10-01 05:43:59 PDT
Landed in
r48967
and
r48968
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug