Bug 35702 – [Qt] Add more support for InputTextController

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Robert Hogan]

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

[Robert Hogan]

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!

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 49946 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/331053

[Robert Hogan]

Created attachment 50150 [details]
Patch

OK, I've got everything working now I think.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 50150 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/343162

[Robert Hogan]

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?

[Csaba Osztrogonác]

(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. ;)

[Simon Hausmann]

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 :)

[Robert Hogan]

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.

[Csaba Osztrogonác]

(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.)

[Eric Seidel (no email)]

Comment on attachment 50670 [details]
Updated Patch

This patch does not seem to apply.  Looks like we'll need another update.

[Robert Hogan]

Created attachment 51824 [details]
Updated Patch

[Simon Hausmann]

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.

[Simon Hausmann]

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).

[Eric Seidel (no email)]

Comment on attachment 51824 [details]
Updated Patch

[e-mail] 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-.

[Robert Hogan]

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!

[Shen Yi]

I am OK with your changes.

[Robert Hogan]

(In reply to comment #18)
> I am OK with your changes.

Thanks Shen. Simon, OK to commit?

[Eric Seidel (no email)]

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.

[Simon Hausmann]

Comment on attachment 52196 [details]
Updated Patch - Needs a Look!

> From ccf816858c724e4235fc5b359b1b175d321ef77b Mon Sep 17 00:00:00 2001
> From: Robert Hogan <[e-mail]>
> Date: Mon, 29 Mar 2010 18:57:59 +0100
> Subject: [PATCH] 2010-03-29  Robert Hogan  <[e-mail]>
> 
>         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.

[Robert Hogan]

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.

[Simon Hausmann]

Comment on attachment 53889 [details]
Updated Patch


> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,24 @@
> [phone]  Robert Hogan  <[e-mail]>
> +
> +        [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  <[e-mail]>
>  
>          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 :)

[Eric Seidel (no email)]

Attachment 53889 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.

[Robert Hogan]

Landed as r52818

[Ademar Reis]

(In reply to comment #25)
> Landed as r52818

Typo correction:
Landed in r58218