Bug 115070 - Moving word boundaries backwards fails when there is a text node starting with an apostrophe
Summary: Moving word boundaries backwards fails when there is a text node starting wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-23 17:47 PDT by Ryosuke Niwa
Modified: 2013-04-24 12:51 PDT (History)
3 users (show)

See Also:


Attachments
Fixes the bug (8.26 KB, patch)
2013-04-23 18:00 PDT, Ryosuke Niwa
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-04-23 17:47:08 PDT
Moving word boundaries backwards fails when there is a text node starting with an apostrophe
Comment 1 Ryosuke Niwa 2013-04-23 18:00:11 PDT
Created attachment 199343 [details]
Fixes the bug
Comment 2 Alexey Proskuryakov 2013-04-23 21:47:33 PDT
Comment on attachment 199343 [details]
Fixes the bug

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?
Comment 3 Ryosuke Niwa 2013-04-24 12:29:32 PDT
Comment on attachment 199343 [details]
Fixes the bug

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

Thanks for the review.

>> Source/WebCore/ChangeLog:12
>> +        "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).

What specifically did you have in your mind?

> LayoutTests/editing/selection/previous-word-boundary-across-text-nodes-expected.txt:24
> +I'll

Oops, I forgot to hide these nodes after the test. Will do that.

>> 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?

Yeah, it gets printed out in the expected result so that we can see what’s being tested.
Comment 4 Ryosuke Niwa 2013-04-24 12:51:33 PDT
Committed r149058: <http://trac.webkit.org/changeset/149058>