Bug 51099 – [Qt] QWebPagePrivate::inputMethodEvent has problem when compositing Korean

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Dongseong Hwang]

QWebPagePrivate::inputMethodEvent has 2 problems when compositing Korean.

1. The caret disappear after compositing Korean.
2. If an user types Korean until end of the text field, the border of the text field hides the compositing text, because of the caret prior to compositing text, not next to that.

[Dongseong Hwang]

Created attachment 76729 [details]
[Qt] QWebPagePrivate::inputMethodEvent has problem when compositing Korean

[WebKit Review Bot]

Attachment 76729 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/Api/qwebpage.cpp', u'WebKit/qt/ChangeLog']" exit_code: 1
WebKit/qt/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongseong Hwang]

I amended two points.

First, added frame->selection()->setCaretVisible(true);
It is because the caret should be visible on and after Korean composition (and the other languages that need composition.).

Second, amend from editor->setComposition(ev->preeditString(), underlines, 0, 0) to editor->setComposition(ev->preeditString(), underlines, 1, 1).
It is because it is more natural for the caret to be next to the composition word, not in front of the composition word.
Moreover, the caret in front of the composition word makes the composition word on right end of the text field hide back text field box.

[Dongseong Hwang]

Created attachment 76730 [details]
[Qt] QWebPagePrivate::inputMethodEvent has problem when compositing Korean

[Eric Seidel (no email)]

Comment on attachment 76730 [details]
[Qt] QWebPagePrivate::inputMethodEvent has problem when compositing Korean

How do we test this?

[Andreas Kling]

Comment on attachment 76730 [details]
[Qt] QWebPagePrivate::inputMethodEvent has problem when compositing Korean

View in context: https://bugs.webkit.org/attachment.cgi?id=76730&action=review

How can we test this?

> WebKit/qt/Api/qwebpage.cpp:1098
> +        // that the caret is vanished. after finishing composition, caret should
> +        // be visible.

Grammar.
When compositing Korean, 'ev' is of type Cursor with length=0. This causes the caret to vanish. After the composition finishes, the caret should be visible.

> WebKit/qt/Api/qwebpage.cpp:1102
> +        // If preeditSting exists, Caret should be visible.

preeditSting -> preeditString
Caret -> the caret

> WebKit/qt/Api/qwebpage.cpp:1105
> +        // next to composition, not in front of composition.

next to composition -> next to the composition
in front of composition -> in front of the composition

[Dongseong Hwang]

Thank you for comment.
It is necessary to check whether this patch is vaild because It is very old patch.

If this is vaild, I'll update the patch with layout test.

[Dongseong Hwang]

Created attachment 91874 [details]
Patch

This patch is about the caret disappearing.
I give up patching about the caret position on composition because when I patch I have no confidence whether the other language have valid caret position.
If I have confidence I will patch again.

Although I find the way how to test, I can not find the feasible test way.
It is because
1. There is no JSObject API whether the caret is visible for layout test.
2. textInputController.setMarkedText's event is different from actual input method event.

In detail about #2, the QInputMethodEvent instance that QWebPagePrivate::inputMethodEvent method receives includes two attribute types such as QInputMethodEvent::TextFormat and QInputMethodEvent::Cursor.
However, textInputController.setMarkedText(,,) only create QInputMethodEvent::TextFormat type QInputMethodEvent instance.
I don't know it is Qt specific or not.

[Dongseong Hwang]

Created attachment 91877 [details]
patch

I made mistake not to include ChangeLog.

[Dongseong Hwang]

Created attachment 91919 [details]
Patch

[Dongseong Hwang]

I am waiting that someone reviews this patch.

[Kenneth Rohde Christiansen]

Comment on attachment 91919 [details]
Patch

I would want a commetn there like "In some languages like Korean you can have a length of 0 after compositing, so the cursor should stay visible" or similar

[Dongseong Hwang]

Why should cursor be invisible if length is 0?
Who does know the reason?

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> Why should cursor be invisible if length is 0?
> Who does know the reason?

I thought that is what you patch does and you needed that for Korean. I do not know Korean, but it shows that you need to explain better why you are making the change then.

[Dongseong Hwang]

Created attachment 95382 [details]
Patch

I changed the approach.

Qt document said QInputMethodEvent::Cursor's zero length means invisible.
http://web.mit.edu/qt-dynamic/www/qinputmethodevent.html
I think it make sense that the caret is invisible when compositing single text that the underline marks compositing here.
If multi-texts are being composited like Arabic, it is necessary for caret to mark where is compositing.

Although the caret became invisible due to composition, the caret should be visible after composition.

[Eric Seidel (no email)]

Comment on attachment 91919 [details]
Patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 91919 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Robert Hogan]

Comment on attachment 95382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95382&action=review

> Source/WebKit/qt/Api/qwebpage.cpp:1133
>              editor->confirmComposition(ev->commitString());
>          else
>              editor->insertText(ev->commitString(), 0);
> +        frame->selection()->setCaretVisible(true);
>      } else if (!hasSelection && !ev->preeditString().isEmpty())
>          editor->setComposition(ev->preeditString(), underlines, 0, 0);
> -    else if (ev->preeditString().isEmpty() && editor->hasComposition())
> +    else if (ev->preeditString().isEmpty() && editor->hasComposition()) {
>          editor->confirmComposition(String());
> +        frame->selection()->setCaretVisible(true);
> +    }
>  

It should be possible to test this in tst_qwebpage. See void tst_QWebFrame::inputFieldFocus() for an example of testing for a blinking caret.

> Source/WebKit/qt/ChangeLog:12
> +        It is nature to composite the text without the caret if Cursor's length is zero.

s/nature/natural

[Eric Seidel (no email)]

Comment on attachment 95382 [details]
Patch

r- for lack of testing per Robert's comment.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.