Bug 81136 - Need implement isWordTextBreak for QT, and WinCE for visual word movement functionality
: Need implement isWordTextBreak for QT, and WinCE for visual word movement fun...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Tullio Lucena
:
Depends on:
Blocks: 25298
  Show dependency treegraph
 
Reported: 2012-03-14 11:17 PDT by Xiaomei Ji
Modified: 2012-10-09 12:14 PDT (History)
6 users (show)

See Also:


Attachments
Unskipping passing tests (1.98 KB, patch)
2012-09-24 13:27 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Fi (2.03 KB, patch)
2012-09-26 10:19 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Unskipping last tests (1.40 KB, patch)
2012-10-03 12:20 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2012-03-14 11:17:03 PDT
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.
Comment 1 Martin Robinson 2012-03-24 09:07:01 PDT
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.
Comment 2 Xiaomei Ji 2012-03-26 12:56:20 PDT
(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.
Comment 3 Martin Robinson 2012-03-26 13:13:20 PDT
(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.
Comment 4 Xiaomei Ji 2012-03-26 13:40:48 PDT
(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.
Comment 5 José Dapena Paz 2012-03-27 07:25:27 PDT
(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?
Comment 6 Xiaomei Ji 2012-04-11 13:38:10 PDT
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?
Comment 7 Tullio Lucena 2012-09-24 13:27:07 PDT
Created attachment 165437 [details]
Unskipping passing tests
Comment 8 Csaba Osztrogonác 2012-09-26 09:36:17 PDT
Comment on attachment 165437 [details]
Unskipping passing tests

I tested it and works, so r=me.
Comment 9 WebKit Review Bot 2012-09-26 09:38:29 PDT
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
Comment 10 Tullio Lucena 2012-09-26 10:19:40 PDT
Created attachment 165827 [details]
Fi
Comment 11 Tullio Lucena 2012-09-26 10:20:12 PDT
Comment on attachment 165827 [details]
Fi

fixed changelog
Comment 12 WebKit Review Bot 2012-09-26 11:00:59 PDT
Comment on attachment 165827 [details]
Fi

Clearing flags on attachment: 165827

Committed r129671: <http://trac.webkit.org/changeset/129671>
Comment 13 Csaba Osztrogonác 2012-09-27 09:05:39 PDT
Comment on attachment 165827 [details]
Fi

Ooops, it was already landed.
Comment 14 Csaba Osztrogonác 2012-09-27 09:06:35 PDT
Yay. Is the bug fixed?
Comment 15 Xiaomei Ji 2012-09-27 10:24:29 PDT
If it is fixed for QT, how about change the bug to WinCE only and keep it open?
Comment 16 Rafael Brandao 2012-09-28 07:38:02 PDT
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. :-)
Comment 17 Tullio Lucena 2012-10-03 12:20:25 PDT
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
Comment 18 WebKit Review Bot 2012-10-09 12:14:25 PDT
Comment on attachment 166937 [details]
Unskipping last tests

Clearing flags on attachment: 166937

Committed r130788: <http://trac.webkit.org/changeset/130788>
Comment 19 WebKit Review Bot 2012-10-09 12:14:29 PDT
All reviewed patches have been landed.  Closing bug.