When editor is in composition mode, the Qt::ImCursorPosition, Qt::ImAnchorPosition, Qt::ImCurrentSelection seems return wrong value, which produce some bugs on S60 VKB, e.g. entering a character which gets highlighted for a second (in composition mode), then the highlight disappeared. Also, in my tests cases, the value of ImCursorPosition & ImAnchorostion can be negative sometimes, which is incorrect. So, I modified QWebPage::inputMethodEvent & inputMethodQuery() based on qcoefepinputcontext_s60.cpp and now it works as following when in composition mode, 1. anchor position and cursor position are the same and always >= 0 2. current selection is always Null when in composition mode
Created attachment 73271 [details] tested on both linux & s60
Comment on attachment 73271 [details] tested on both linux & s60 View in context: https://bugs.webkit.org/attachment.cgi?id=73271&action=review Very nice test coverage here! Unfortunately I have to r- because of lacking ChangeLog. > WebKit/qt/ChangeLog:7 > + Insufficient ChangeLog- please explain what's being changed and why. > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1695 > + // clear selection, also cancel the ongoing composition if there is one Comments should be capitalized and end with a period. > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1704 > + //ImCurrentSelection Style violation, space after //
Created attachment 73374 [details] fix style and changelog
Comment on attachment 73374 [details] fix style and changelog View in context: https://bugs.webkit.org/attachment.cgi?id=73374&action=review Oops, > WebKitTools/ChangeLog:6 > + [Qt][QtTestBrowser] Don't erase incorrect url in the Url Bar > + https://bugs.webkit.org/show_bug.cgi?id=49047 This stuff belongs to another bug report :) > WebKitTools/QtTestBrowser/mainwindow.cpp:119 > - urlEdit->setText(url.toString(QUrl::RemoveUserInfo)); > + setAddressUrl(url.toString(QUrl::RemoveUserInfo)); Ditto. > WebKitTools/QtTestBrowser/mainwindow.cpp:125 > - urlEdit->setText(url); > + if (!url.contains("about:")) > + urlEdit->setText(url); Ditto.
Created attachment 73375 [details] try again
(In reply to comment #4) > (From update of attachment 73374 [details]) > This stuff belongs to another bug report :) Weird, I updated both? Anyway, please review it again, thanks :)
Created attachment 73891 [details] fix style
Comment on attachment 73891 [details] fix style View in context: https://bugs.webkit.org/attachment.cgi?id=73891&action=review In general the change looks good to me, but I'd like to hear Robert's opinion on the patch, too. Nice test coverage, btw :) > WebKit/qt/Api/qwebpage.cpp:1086 > - editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length()); > + editor->setComposition(ev->preeditString(), underlines, 0, 0); That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before? > WebKit/qt/Api/qwebpage.cpp:1372 > - if (editor->hasComposition()) { > - RefPtr<Range> range = editor->compositionRange(); > - return QVariant(frame->selection()->end().offsetInContainerNode() - TextIterator::rangeLength(range.get())); > - } > + if (editor->hasComposition()) > + return QVariant(frame->selection()->end().offsetInContainerNode()); Ah, does this fix the negative positioning you mentioned in the bugreport? > WebKit/qt/Api/qwebpage.cpp:1386 > - if (renderTextControl) { > + if (!editor->hasComposition() && renderTextControl) { Why is this check needed? When you enter composition, you clear the selection anyway, don't you? > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1755 > + // Send temporary text, which makes the editor has composition 'l'. > + { > + QList<QInputMethodEvent::Attribute> attributes; > + QInputMethodEvent event("n", attributes); > + page->event(&event); Shouldn't the comment say "has composition 'n'" ? > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1810 > + // Send temporary text, which makes the editor has composition 'm'. > + { > + QList<QInputMethodEvent::Attribute> attributes; > + QInputMethodEvent event("d", attributes); Same here, isn't the comment incorrect and it should say 'd' instead? > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1869 > + // Send temporary text, which makes the editor has composition 'm'. > + { > + QList<QInputMethodEvent::Attribute> attributes; > + QInputMethodEvent event("t", attributes); > + page->event(&event); Same here...
(In reply to comment #8) > (From update of attachment 73891 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73891&action=review > > In general the change looks good to me, but I'd like to hear Robert's opinion on the patch, too. Nice test coverage, btw :) > > > WebKit/qt/Api/qwebpage.cpp:1086 > > - editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length()); > > + editor->setComposition(ev->preeditString(), underlines, 0, 0); > > That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before? > > > WebKit/qt/Api/qwebpage.cpp:1372 > > - if (editor->hasComposition()) { > > - RefPtr<Range> range = editor->compositionRange(); > > - return QVariant(frame->selection()->end().offsetInContainerNode() - TextIterator::rangeLength(range.get())); > > - } > > + if (editor->hasComposition()) > > + return QVariant(frame->selection()->end().offsetInContainerNode()); > > Ah, does this fix the negative positioning you mentioned in the bugreport? Yes, it does. > > > WebKit/qt/Api/qwebpage.cpp:1386 > > - if (renderTextControl) { > > + if (!editor->hasComposition() && renderTextControl) { > > Why is this check needed? When you enter composition, you clear the selection anyway, don't you? No, the editor doesn't clear the selection when enter composition. So I add this check to make sure it returns empty string when in composition. > > > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1755 > > + // Send temporary text, which makes the editor has composition 'l'. > > + { > > + QList<QInputMethodEvent::Attribute> attributes; > > + QInputMethodEvent event("n", attributes); > > + page->event(&event); > > Shouldn't the comment say "has composition 'n'" ? I will fix it, thanks > > > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1810 > > + // Send temporary text, which makes the editor has composition 'm'. > > + { > > + QList<QInputMethodEvent::Attribute> attributes; > > + QInputMethodEvent event("d", attributes); > > Same here, isn't the comment incorrect and it should say 'd' instead? > > > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1869 > > + // Send temporary text, which makes the editor has composition 'm'. > > + { > > + QList<QInputMethodEvent::Attribute> attributes; > > + QInputMethodEvent event("t", attributes); > > + page->event(&event); > > Same here...
Created attachment 74112 [details] fix typo
(In reply to comment #8) > > > WebKit/qt/Api/qwebpage.cpp:1086 > > - editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length()); > > + editor->setComposition(ev->preeditString(), underlines, 0, 0); > > That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before? That looks like a regression from http://trac.webkit.org/changeset/70259 that wasn't caught by the unit tests. Yi, could you make sure the changes don't cause any regressions on : 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
(In reply to comment #11) > (In reply to comment #8) > > > > > WebKit/qt/Api/qwebpage.cpp:1086 > > > - editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length()); > > > + editor->setComposition(ev->preeditString(), underlines, 0, 0); > > > > That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before? > > That looks like a regression from http://trac.webkit.org/changeset/70259 that wasn't caught by the unit tests. > > Yi, could you make sure the changes don't cause any regressions on : > > 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, I run the test on linux and all above tests passed.
(In reply to comment #12) > > Robert Hogan, I run the test on linux and all above tests passed. Great stuff. Looks good to me so!
Comment on attachment 74112 [details] fix typo LGTM, too. Thanks Simon & Robert for weighing in!
(In reply to comment #14) > (From update of attachment 74112 [details]) > LGTM, too. Thanks Simon & Robert for weighing in! Thanks all you guys :)
Comment on attachment 74112 [details] fix typo View in context: https://bugs.webkit.org/attachment.cgi?id=74112&action=review > WebKit/qt/ChangeLog:10 > + 2. current selection is always NULL We normally write "null" as we do not use a define for 0.
Created attachment 74150 [details] fix changelog
Comment on attachment 74112 [details] fix typo Cleared Andreas Kling's review+ from obsolete attachment 74112 [details] so that this bug does not appear in http://webkit.org/pending-commit.
The commit-queue encountered the following flaky tests while processing attachment 74150 [details]: compositing/iframes/overlapped-nested-iframes.html fast/frames/flattening/frameset-flattening-advanced.html Please file bugs against the tests. These tests were authored by kenneth@webkit.org, ossy@webkit.org, and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
(In reply to comment #19) > The commit-queue encountered the following flaky tests while processing attachment 74150 [details]: > > compositing/iframes/overlapped-nested-iframes.html > fast/frames/flattening/frameset-flattening-advanced.html > > Please file bugs against the tests. These tests were authored by kenneth@webkit.org, ossy@webkit.org, and simon.fraser@apple.com. The commit-queue is continuing to process your patch. Tested on Linux with r72289, compositing/iframes/overlapped-nested-iframes.html failed without/with my patch. fast/frames/flattening/frameset-flattening-advanced.html passed without/with my patch.
The commit-queue encountered the following flaky tests while processing attachment 74150 [details]: compositing/iframes/overlapped-nested-iframes.html fast/dom/Document/CaretRangeFromPoint/hittest-relative-to-viewport.html Please file bugs against the tests. These tests were authored by sam@webkit.org, simon.fraser@apple.com, and xji@chromium.org. The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 74150 [details]: compositing/iframes/overlapped-nested-iframes.html fast/workers/storage/use-same-database-in-page-and-workers.html Please file bugs against the tests. These tests were authored by dumi@chromium.org and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
Comment on attachment 74150 [details] fix changelog Clearing flags on attachment: 74150 Committed r72370: <http://trac.webkit.org/changeset/72370>
All reviewed patches have been landed. Closing bug.
The commit-queue encountered the following flaky tests while processing attachment 74150 [details]: compositing/iframes/overlapped-nested-iframes.html fast/preloader/script.html Please file bugs against the tests. These tests were authored by abarth@webkit.org and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
(In reply to comment #25) > The commit-queue encountered the following flaky tests while processing attachment 74150 [details]: > > compositing/iframes/overlapped-nested-iframes.html > fast/preloader/script.html > > Please file bugs against the tests. These tests were authored by abarth@webkit.org and simon.fraser@apple.com. The commit-queue is continuing to process your patch. Tested on Linux with r72337, fast/dom/Document/CaretRangeFromPoint/hittest-relative-to-viewport.html fast/workers/storage/use-same-database-in-page-and-workers.html fast/preloader/script.html all passed with this patch.