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
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!
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.
Attachment 49946 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/331053
Created attachment 50150 [details] Patch OK, I've got everything working now I think.
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.
Attachment 50150 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/343162
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?
(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 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 :)
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.
(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 on attachment 50670 [details] Updated Patch This patch does not seem to apply. Looks like we'll need another update.
Created attachment 51824 [details] Updated Patch
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 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 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-.
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!
I am OK with your changes.
(In reply to comment #18) > I am OK with your changes. Thanks Shen. Simon, OK to commit?
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 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.
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 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 :)
Attachment 53889 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.
Landed as r52818
(In reply to comment #25) > Landed as r52818 Typo correction: Landed in r58218