[webkit-reviews] review granted: [Bug 115070] Moving word boundaries backwards fails when there is a text node starting with an apostrophe : [Attachment 199343] Fixes the bug

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 115070: Moving word boundaries backwards fails when there is a text node
starting with an apostrophe
https://bugs.webkit.org/show_bug.cgi?id=115070

Attachment 199343: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=199343&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=199343&action=review


r=me

> Source/WebCore/ChangeLog:12
> +	   text node is seen by ICU on its own even though there is no word
boundary between the two when the entire
> +	   "I'll" is given to ICU.

I think that it would be more appropriate to talk about the expected behavior
per Unicode spec here, not about ICU, which is an implementation detail (and
not even used on Mac in this case, at least not directly).

> Source/WebCore/ChangeLog:15
> +	   work around this bug. In the long term, we probably need to give
ICU/AppKit the entire context since in

More precisely, Foundation - I don't think that this part of NSString is in
AppKit.

> Source/WebCore/ChangeLog:16
> +	   langugaes like Hebrew and some of European languages, there could be
many accents and combining characters

typo: langugaes

> Source/WebCore/editing/VisibleUnits.cpp:502
> +	   if (next > 1) // FIXME: This is a work around for
https://webkit.org/b/115070. We need to give more contexts in general case.

I'd say "provide more context", not "give".

>
LayoutTests/editing/selection/previous-word-boundary-across-text-nodes.html:39
> +    shouldBeEqualToString("selectWordBackward(container); /* " +
container.innerHTML + " */ getSelection().toString();", container.textContent);


Does the commented out code help here?