Bug 81136 – Need implement isWordTextBreak for QT, and WinCE for visual word movement functionality

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Xiaomei Ji]

visual word movement relies isWordTextBreak.
isWordTextBreak returns true if the word break iterator moving across a word, and it returns false if the word break iterator moving across a punctuation/space.
In platforms using ICU, isWordTextBreak uses ICU break iterator functionality.

[Martin Robinson]

I cannot find isWordTextBreak anywhere in the WebKit source code. Is this something you're working on now? For the record GTK+ uses ICU by default, so there shouldn't be much in the way of implementation.

[Xiaomei Ji]

(In reply to comment #1)
> I cannot find isWordTextBreak anywhere in the WebKit source code. Is this something you're working on now? For the record GTK+ uses ICU by default, so there shouldn't be much in the way of implementation.

It is added in r110965
under Source/WebCore/platform/text/TextBreakIterator.h.

If you look at its implementation under TextBreakIteratorICU.cpp,
basically, it is checking the current word break iterator just pass over a word.
For example, "abc def", both positions "abc| |def" are word break positions,
but isWordTextBreak returns true for "abc| def" since the break iterator just pass over "abc". And it returns false for "abc |def" since the break iterator just pass over a space.

If GTK+ uses ICU, that should be easy to implemented.

[Martin Robinson]

(In reply to comment #2)

> If GTK+ uses ICU, that should be easy to implemented.

Look like this just needs to be implemented for the GLib unicode backend, which we don't have complete support for anyway. By default GTK+ uses Source/WebCore/platform/text/TextBreakIteratorICU.cpp.

[Xiaomei Ji]

(In reply to comment #3)
> (In reply to comment #2)
> 
> > If GTK+ uses ICU, that should be easy to implemented.
> 
> Look like this just needs to be implemented for the GLib unicode backend, which we don't have complete support for anyway. By default GTK+ uses Source/WebCore/platform/text/TextBreakIteratorICU.cpp.

Ah, then we can turn it on for GTK.
I thought GTK uses TextBreakIteratorGtk.cpp.

[José Dapena Paz]

(In reply to comment #2)
> It is added in r110965
> under Source/WebCore/platform/text/TextBreakIterator.h.
> 
> If you look at its implementation under TextBreakIteratorICU.cpp,
> basically, it is checking the current word break iterator just pass over a word.
> For example, "abc def", both positions "abc| |def" are word break positions,
> but isWordTextBreak returns true for "abc| def" since the break iterator just pass over "abc". And it returns false for "abc |def" since the break iterator just pass over a space.
> 
> If GTK+ uses ICU, that should be easy to implemented.

I've tried running the Skipped tests, and all but one run without problems in webkitgtk, as it uses ICU backend.

The only test failing is editing/selection/move-by-word-visually-multi-line, with this kind of failure:

 Test 1, LTR:
 Move right by one word
-"abc def ghi jkl mn "[0, 4, 8, 12, 16, 19], "opq rst uvw xyz"[0, 4, 8, 12, 15]
+"abc def ghi jkl mn "[0, 4, 8, 12, 16], "opq rst uvw xyz"[0, 4, 8, 12, 15]
 Move left by one word
 "opq rst uvw xyz"[15, 12, 8, 4, 0], "abc def ghi jkl mn "[16, 12, 8, 4, 0]


 Test 7, RTL:
 Move left by one word
-"abc def ghi jkl mn "[0, 3, 8, 11, 16, 19], "opq rst uvw xyz"[0, 3, 8, 11, 15]
+"abc def ghi jkl mn "[0, 3, 8, 11, 16], "opq rst uvw xyz"[0, 3, 8, 11, 15]
 Move right by one word
 "opq rst uvw xyz"[15, 11, 8, 3, 0], "abc def ghi jkl mn "[16, 11, 8, 3, 0]

Seems that the ICU implementation is not considering the blank space at the end of lines as a word break. Is it expected?

[Xiaomei Ji]

Sorry for replying late.
I think gtk's result is more reasonable (in terms of consistent with other output in other tests).

Since both gtk and chromium uses ICU, I will need to check why chromium's result is different.

(In reply to comment #5)
> (In reply to comment #2)
> > It is added in r110965
> > under Source/WebCore/platform/text/TextBreakIterator.h.
> > 
> > If you look at its implementation under TextBreakIteratorICU.cpp,
> > basically, it is checking the current word break iterator just pass over a word.
> > For example, "abc def", both positions "abc| |def" are word break positions,
> > but isWordTextBreak returns true for "abc| def" since the break iterator just pass over "abc". And it returns false for "abc |def" since the break iterator just pass over a space.
> > 
> > If GTK+ uses ICU, that should be easy to implemented.
> 
> I've tried running the Skipped tests, and all but one run without problems in webkitgtk, as it uses ICU backend.
> 
> The only test failing is editing/selection/move-by-word-visually-multi-line, with this kind of failure:
> 
>  Test 1, LTR:
>  Move right by one word
> -"abc def ghi jkl mn "[0, 4, 8, 12, 16, 19], "opq rst uvw xyz"[0, 4, 8, 12, 15]
> +"abc def ghi jkl mn "[0, 4, 8, 12, 16], "opq rst uvw xyz"[0, 4, 8, 12, 15]
>  Move left by one word
>  "opq rst uvw xyz"[15, 12, 8, 4, 0], "abc def ghi jkl mn "[16, 12, 8, 4, 0]
> 
> 
>  Test 7, RTL:
>  Move left by one word
> -"abc def ghi jkl mn "[0, 3, 8, 11, 16, 19], "opq rst uvw xyz"[0, 3, 8, 11, 15]
> +"abc def ghi jkl mn "[0, 3, 8, 11, 16], "opq rst uvw xyz"[0, 3, 8, 11, 15]
>  Move right by one word
>  "opq rst uvw xyz"[15, 11, 8, 3, 0], "abc def ghi jkl mn "[16, 11, 8, 3, 0]
> 
> Seems that the ICU implementation is not considering the blank space at the end of lines as a word break. Is it expected?

[Tullio Lucena]

Created attachment 165437 [details]
Unskipping passing tests

[Csaba Osztrogonác]

Comment on attachment 165437 [details]
Unskipping passing tests

I tested it and works, so r=me.

[WebKit Review Bot]

Comment on attachment 165437 [details]
Unskipping passing tests

Rejecting attachment 165437 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14038354

[Tullio Lucena]

Created attachment 165827 [details]
Fi

[Tullio Lucena]

Comment on attachment 165827 [details]
Fi

fixed changelog

[WebKit Review Bot]

Comment on attachment 165827 [details]
Fi

Clearing flags on attachment: 165827

Committed r129671: <http://trac.webkit.org/changeset/129671>

[Csaba Osztrogonác]

Comment on attachment 165827 [details]
Fi

Ooops, it was already landed.

[Csaba Osztrogonác]

Yay. Is the bug fixed?

[Xiaomei Ji]

If it is fixed for QT, how about change the bug to WinCE only and keep it open?

[Rafael Brandao]

Ossy, Tullio is investigating one of the two remaining tests:
editing/selection/move-by-word-visually-multi-line.html

I think we can keep this bug open for instance. I believe this came after Qt started to use ICU. :-)

[Tullio Lucena]

Created attachment 166937 [details]
Unskipping last tests

The problem in this last tests was at the Hebrew fonts.
The changes at testfonts [1] made this tests pass now.
If this patch pass at bots, i think that this bug is fixed.

[1] https://gitorious.org/qtwebkit/testfonts/merge_requests/1

[WebKit Review Bot]

Comment on attachment 166937 [details]
Unskipping last tests

Clearing flags on attachment: 166937

Committed r130788: <http://trac.webkit.org/changeset/130788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.