Bug 35702 - [Qt] Add more support for InputTextController
Summary: [Qt] Add more support for InputTextController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-03-03 14:00 PST by Robert Hogan
Modified: 2010-10-22 11:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.18 KB, patch)
2010-03-03 14:05 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (15.22 KB, patch)
2010-03-06 05:35 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (15.30 KB, patch)
2010-03-06 06:25 PST, Robert Hogan
hausmann: review-
Details | Formatted Diff | Diff
Updated Patch (15.91 KB, patch)
2010-03-14 04:19 PDT, Robert Hogan
eric: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (16.18 KB, patch)
2010-03-27 05:03 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated Patch - Needs a Look! (16.36 KB, patch)
2010-03-31 12:40 PDT, Robert Hogan
hausmann: review-
Details | Formatted Diff | Diff
Updated Patch (17.56 KB, patch)
2010-04-20 15:10 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-03-03 14:00:10 PST
Add support for selectedRange(), setMarkedText(), insertText(),
and partial support for firstRectForCharacterRange().

Unskip tests:

fast/forms/input-maxlength-ime-preedit.html
fast/forms/input-maxlength-ime-completed.html
fast/text/international/thai-cursor-position.html
fast/events/ime-composition-events-001.html
Comment 1 Robert Hogan 2010-03-03 14:05:57 PST
Created attachment 49946 [details]
Patch

I'm not entirely sure of some of the changes I've made here: the changes in inputMethodEvent in qwebpage may have side-effects I'm not aware of, also I'd appreciate it if someone has any pointers the FIXME in firstRectForCharacterRange!
Comment 2 WebKit Review Bot 2010-03-03 14:08:54 PST
Attachment 49946 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:159:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:161:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:162:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 7 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2010-03-03 15:59:12 PST
Attachment 49946 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/331053
Comment 4 Robert Hogan 2010-03-06 05:35:37 PST
Created attachment 50150 [details]
Patch

OK, I've got everything working now I think.
Comment 5 WebKit Review Bot 2010-03-06 05:39:36 PST
Attachment 50150 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/TextInputControllerQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 WebKit Review Bot 2010-03-06 06:01:26 PST
Attachment 50150 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/343162
Comment 7 Robert Hogan 2010-03-06 06:25:12 PST
Created attachment 50152 [details]
Patch

Fix style and < 4.6 build.

The bot is running 4.5 or less, and the tests won't pass on less than 4.6 due to the need to QInputMethodQuery::Selection. So maybe this patch is blocked until the bot upgrades? How does this sort of thing usually work, when a fix is Qt version dependent?
Comment 8 Csaba Osztrogonác 2010-03-09 03:38:29 PST
(In reply to comment #7)
> Created an attachment (id=50152) [details]
> Patch

On buidbots we run Qt 4.6.0, so it shouldn't be blocked. Only Qt-EWS bot runs Qt 4.5, but it is only a builder and not a tester. Additionally I tested your patch on buildbot, it works without regressions. ;)
Comment 9 Simon Hausmann 2010-03-10 14:03:52 PST
Comment on attachment 50152 [details]
Patch



> +QVariantList QWEBKIT_EXPORT qt_drt_selectedRange(QWebPage* page)
> +{
> +    WebCore::Frame *frame = page->handle()->page->focusController()->focusedOrMainFrame();

Coding style nitpick, * placement :)

> +    WebCore::Frame *frame = page->handle()->page->focusController()->focusedOrMainFrame();

Same here.

> +    int max = InputElement::s_maximumLength;
> +    if (frame->document() && frame->document()->focusedNode()) {
> +        if (frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +            HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +            max = inputElement->maxLength();
> +        }
> +    }
> +
>      if (!ev->commitString().isEmpty())
>          editor->confirmComposition(ev->commitString());
> -    else if (!ev->preeditString().isEmpty()) {
> +    else {
>          QString preedit = ev->preeditString();
> -        editor->setComposition(preedit, underlines, preedit.length(), 0);
> +        editor->setComposition(preedit, underlines, (preedit.length() > max) ? max : preedit.length(), 0);
>      }

Instead of this logic it might be better to use InputElement::sanitizeValue instead, which AFAICS
would provide a similar cut-off with the extra advantage of handling grapheme clusters, i.e.
cutting off more characters than needed to avoid invalid character combinations.

The cut-off sounds like a good idea in general, but I think this should be also tested
with a unit-test. Sorry, I know this is asking for more than you intented to actually
fix, but the input method handling is unfortunately fragile :(. On the upside there are already
a few unit tests in tst_qwebpage.cpp.

> +#if QT_VERSION >= 0x040600
> +    if (renderTextControl && (selection.length > 0)) {

Probably more of a style nitpick, but I'd leave out the extra pair of parentheses :)
Comment 10 Robert Hogan 2010-03-14 04:19:06 PDT
Created attachment 50670 [details]
Updated Patch

The previous patch didn't pass tst_qwebpage - now it does. I've removed the maxlength check for now - will do it as a separate patch (promise!).

Ossy - I've added a couple of Qt-specific results. The cursor position is certainly correct from the point of view of the test, though it might be that the actual number reported is specific to my system so would appreciate if you, or anyone else, could check whether you match the existing test results or mine when running with this patch.
Comment 11 Csaba Osztrogonác 2010-03-16 08:44:57 PDT
(In reply to comment #10)
> Ossy - I've added a couple of Qt-specific results. The cursor position is
> certainly correct from the point of view of the test, though it might be that
> the actual number reported is specific to my system so would appreciate if you,
> or anyone else, could check whether you match the existing test results or mine
> when running with this patch.

I tested it, works correctly for me. But you should land it manually, because WebKit/qt/Api/qwebpages.cpp conflicts. (Only the context changed in this file.)
Comment 12 Eric Seidel (no email) 2010-03-24 23:59:02 PDT
Comment on attachment 50670 [details]
Updated Patch

This patch does not seem to apply.  Looks like we'll need another update.
Comment 13 Robert Hogan 2010-03-27 05:03:00 PDT
Created attachment 51824 [details]
Updated Patch
Comment 14 Simon Hausmann 2010-03-28 14:06:21 PDT
I'd like to include this in the release branch to make it easier to cherry-pick test fixes in the future, due to the DRT changes.
Comment 15 Simon Hausmann 2010-03-28 14:10:24 PDT
Comment on attachment 51824 [details]
Updated Patch


> -            if (renderTextControl) {
> -                renderTextControl->setSelectionStart(qMin(a.start, (a.start + a.length)));
> -                renderTextControl->setSelectionEnd(qMax(a.start, (a.start + a.length)));
> -            }
> +            selection = a;
> +            hasSelection = true;
>              break;
>          }
>  #endif
> @@ -1334,10 +1382,23 @@ void QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)

> +#if QT_VERSION >= 0x040600
> +        if (hasSelection) {
> +            QString text = (renderTextControl) ? QString(renderTextControl->text()) : QString();
> +            if (preedit.isEmpty() && selection.start + selection.length > 0)
> +                preedit = text;
> +            editor->setComposition(preedit, underlines, selection.start, selection.start + selection.length);
> +        } else
> +#endif
> +            editor->setComposition(preedit, underlines, preedit.length(), 0);

Looks good to me, but before landing please verify that the above two hunks don't break the auto-test added in http://trac.webkit.org/changeset/56334 , which verifies that selection start/end are swapped correctly if they are reversed (see the qMin/qMax usage).
Comment 16 Eric Seidel (no email) 2010-03-29 11:41:34 PDT
Comment on attachment 51824 [details]
Updated Patch

robert@webkit.org is not in http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py so I would cq+, however it sounds like the reviewer has comments left to address, so cq-.
Comment 17 Robert Hogan 2010-03-31 12:40:02 PDT
Created attachment 52196 [details]
Updated Patch - Needs a Look!

I've made a couple of modifications to cope with the test added to tst_qwepage by Yi Shen. The first is in qwebpage itself, but I have also had to add a step to tst_qwebpage where it cancels the current composition before performing the negative selection. I believe this is correct but would like someone more conversant with the expected behaviour of input methods (Yi Shen?) to take a look first!
Comment 18 Shen Yi 2010-03-31 13:56:25 PDT
I am OK with your changes.
Comment 19 Robert Hogan 2010-04-01 13:38:16 PDT
(In reply to comment #18)
> I am OK with your changes.

Thanks Shen. Simon, OK to commit?
Comment 20 Eric Seidel (no email) 2010-04-06 23:43:54 PDT
Comment on attachment 51824 [details]
Updated Patch

Cleared Simon Hausmann's review+ from obsolete attachment 51824 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 21 Simon Hausmann 2010-04-19 17:35:03 PDT
Comment on attachment 52196 [details]
Updated Patch - Needs a Look!

> From ccf816858c724e4235fc5b359b1b175d321ef77b Mon Sep 17 00:00:00 2001
> From: Robert Hogan <robert@webkit.org>
> Date: Mon, 29 Mar 2010 18:57:59 +0100
> Subject: [PATCH] 2010-03-29  Robert Hogan  <robert@webkit.org>
> 
>         Reviewed by Simon Hausmann.

I have r+ed an earlier patch, but not reviewed this one.

In general I still agree with this patch, but it doesn't apply anymore after the recent refactorings.
Comment 22 Robert Hogan 2010-04-20 15:10:46 PDT
Created attachment 53889 [details]
Updated Patch

Sorry Simon, that was a waste of review! Updated patch attached. I'll land this manually at the weekend in case any buildbot/my build differences crop up in the tests.
Comment 23 Simon Hausmann 2010-04-20 17:06:57 PDT
Comment on attachment 53889 [details]
Updated Patch


> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,24 @@
> +2010-04-20  Robert Hogan  <robert@webkit.org>
> +
> +        [Qt] Add more support for textInputController
> +

This part of the ChangeLog is missing the reviewed by bit ;-)

> +        Add support for selectedRange(), setMarkedText(), insertText(),
> +        and firstRectForCharacterRange().
> +
> +        Unskip tests:
> +
> +        fast/forms/input-maxlength-ime-preedit.html
> +        fast/forms/input-maxlength-ime-completed.html
> +        fast/text/international/thai-cursor-position.html
> +        fast/events/ime-composition-events-001.html
> +        editing/selection/5825350-1.html
> +        editing/selection/5825350-2.html
> +        editing/selection/mixed-editability-10.html
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=35702
> +
> +        * platform/qt/Skipped:
> +
>  2010-04-20  James Robinson  <jamesr@chromium.org>
>  
>          Unreviewed.  Update chromium test expectations to reflect changes

>      WebCore::Frame *frame = page->focusController()->focusedOrMainFrame();
>      WebCore::Editor *editor = frame->editor();
> +#if QT_VERSION >= 0x040600
> +    QInputMethodEvent::Attribute selection(QInputMethodEvent::Selection, 0, 0, QVariant());
> +#endif
>  
>      if (!editor->canEdit()) {
>          ev->ignore();
> @@ -1264,6 +1267,7 @@ void QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)
>          renderTextControl = toRenderTextControl(renderer);
>  
>      Vector<CompositionUnderline> underlines;
> +    bool hasSelection = false;

I suggest to move the boolean next to the definition of the "selection" variable, for clarity.

> @@ -1299,10 +1301,25 @@ void QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)
>  
>      if (!ev->commitString().isEmpty())
>          editor->confirmComposition(ev->commitString());
> -    else if (!ev->preeditString().isEmpty()) {
> +    else {
> +        // 1. empty preedit with a selection attribute, and start/end of 0 cancels composition
> +        // 2. empty preedit with a selection attribute, and start/end of non-0 updates selection of current preedit text
> +        // 3. populated preedit with a selection attribute, and start/end of 0 or non-0 updates selection of supplied preedit text
> +        // 4. otherwise event is updating supplied pre-edit text
>          QString preedit = ev->preeditString();
> -        editor->setComposition(preedit, underlines, preedit.length(), 0);
> +#if QT_VERSION >= 0x040600
> +        if (hasSelection) {
> +            QString text = (renderTextControl) ? QString(renderTextControl->text()) : QString();

I think you can leave out the parenthese around renderTextControl.


r=me. Feel free to fix my nitpicks when landing :)
Comment 24 Eric Seidel (no email) 2010-04-21 18:15:07 PDT
Attachment 53889 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.
Comment 25 Robert Hogan 2010-04-25 00:27:15 PDT
Landed as r52818
Comment 26 Ademar Reis 2010-10-22 11:16:46 PDT
(In reply to comment #25)
> Landed as r52818

Typo correction:
Landed in r58218