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
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 25298
  Show dependency treegraph
 
Reported: 2012-03-14 11:17 PST by
Modified: 2012-10-09 12:14 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-14 11:17:03 PST
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 From 2012-03-24 09:07:01 PST -------
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 From 2012-03-26 12:56:20 PST -------
(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 From 2012-03-26 13:13:20 PST -------
(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 From 2012-03-26 13:40:48 PST -------
(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 From 2012-03-27 07:25:27 PST -------
(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 From 2012-04-11 13:38:10 PST -------
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 From 2012-09-24 13:27:07 PST -------
Created an attachment (id=165437) [details]
Unskipping passing tests
------- Comment #8 From 2012-09-26 09:36:17 PST -------
(From update of attachment 165437 [details])
I tested it and works, so r=me.
------- Comment #9 From 2012-09-26 09:38:29 PST -------
(From update of attachment 165437 [details])
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 From 2012-09-26 10:19:40 PST -------
Created an attachment (id=165827) [details]
Fi
------- Comment #11 From 2012-09-26 10:20:12 PST -------
(From update of attachment 165827 [details])
fixed changelog
------- Comment #12 From 2012-09-26 11:00:59 PST -------
(From update of attachment 165827 [details])
Clearing flags on attachment: 165827

Committed r129671: <http://trac.webkit.org/changeset/129671>
------- Comment #13 From 2012-09-27 09:05:39 PST -------
(From update of attachment 165827 [details])
Ooops, it was already landed.
------- Comment #14 From 2012-09-27 09:06:35 PST -------
Yay. Is the bug fixed?
------- Comment #15 From 2012-09-27 10:24:29 PST -------
If it is fixed for QT, how about change the bug to WinCE only and keep it open?
------- Comment #16 From 2012-09-28 07:38:02 PST -------
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 From 2012-10-03 12:20:25 PST -------
Created an attachment (id=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 From 2012-10-09 12:14:25 PST -------
(From update of attachment 166937 [details])
Clearing flags on attachment: 166937

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