Bug 40351 - [chromium] Can't handle deletion of characters beyond U+10000.
Summary: [chromium] Can't handle deletion of characters beyond U+10000.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-08 22:13 PDT by James Su
Modified: 2011-09-12 00:13 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.29 KB, patch)
2011-09-05 06:45 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2011-09-05 21:00 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2011-09-05 21:28 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2011-09-11 23:12 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Su 2010-06-08 22:13:25 PDT
When deleting a unicode character beyond U+10000, which occupies 2 utf16 chars, webkit can only delete one utf16 char each time.

You may use following steps to reproduce this issue:
1. open www.google.com
2. copy&paste character "𠀾" (U+2003E) into the search box
3. press backspace to delete it, you may notice that the character is disappeared, but actually there is one illegal utf16 char remained.
4. press enter, then the search result page will show with "�" in the search box.

I tested it on Mac OSX 10.6.3
Comment 1 James Su 2010-06-08 22:15:30 PDT
I reproduced it with the latest Chrome. But seems that Safari 4.0 doesn't have this problem.
Comment 2 Alexey Proskuryakov 2010-06-09 13:46:06 PDT
Yes, I also cannot reproduce this in Safari.
Comment 3 Shinya Kawanaka 2011-09-05 06:45:53 PDT
Created attachment 106334 [details]
Patch
Comment 4 Alexey Proskuryakov 2011-09-05 13:10:28 PDT
Comment on attachment 106334 [details]
Patch

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

Looks good.

> Source/WebCore/rendering/RenderText.cpp:1671
> +#if PLATFORM(MAC) || defined(OS_MACOSX)

I don't know if all other ports want this behavior when running on OS X, although I would certainly encourage them to.

> Source/WebCore/rendering/RenderText.cpp:1761
> +    StringImpl& text = *m_text.impl();
> +    if (U16_IS_TRAIL(text[--current]))
> +        --current;

Why did you store StringImpl in a local variable? Just accessing the text through m_text seems like it would be better for readability:

    if (U16_IS_TRAIL(m_text[--current]))
Comment 5 Kent Tamura 2011-09-05 18:16:00 PDT
Comment on attachment 106334 [details]
Patch

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

>> Source/WebCore/rendering/RenderText.cpp:1671
>> +#if PLATFORM(MAC) || defined(OS_MACOSX)
> 
> I don't know if all other ports want this behavior when running on OS X, although I would certainly encourage them to.

OS_MACOSX is a macro used only in Chromium, and we should not use it here. We should write

#if PLATFORM(MAC) || PLATFORM(CHROMIUM) && OS(MAC_OS_X)

or

#if OS(DARWIN)
if you'd like to include iOS and Qt/Mac.
Comment 6 Kent Tamura 2011-09-05 18:49:57 PDT
Comment on attachment 106334 [details]
Patch

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

> LayoutTests/editing/deleting/delete-surrogatepair.html:34
> +    if (!window.layoutTestController)
> +        continue;

This check should be at the outside of the 'for' loop.
We had better show a message like "This test requires layoutTestController" or a manual test instruction if there is no layoutTestController.

> LayoutTests/editing/deleting/delete-surrogatepair.html:40
> +        layoutTestController.execCommand("MoveForward");
> +
> +    layoutTestController.execCommand("DeleteBackward");

Do you need layoutTestController.execCommand()? Doesn't document.execCommand() work?
Comment 7 Shinya Kawanaka 2011-09-05 21:00:26 PDT
Created attachment 106382 [details]
Patch
Comment 8 Shinya Kawanaka 2011-09-05 21:10:49 PDT
(In reply to comment #6)
> (From update of attachment 106334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106334&action=review
> 
> > LayoutTests/editing/deleting/delete-surrogatepair.html:34
> > +    if (!window.layoutTestController)
> > +        continue;
> 
> This check should be at the outside of the 'for' loop.
> We had better show a message like "This test requires layoutTestController" or a manual test instruction if there is no layoutTestController.
> 
> > LayoutTests/editing/deleting/delete-surrogatepair.html:40
> > +        layoutTestController.execCommand("MoveForward");
> > +
> > +    layoutTestController.execCommand("DeleteBackward");
> 
> Do you need layoutTestController.execCommand()? Doesn't document.execCommand() work?

Unfortunately, document.execCommand() didn't work in my environment.
Comment 9 Kent Tamura 2011-09-05 21:14:24 PDT
Comment on attachment 106382 [details]
Patch

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

> LayoutTests/editing/deleting/delete-surrogatepair.html:10
> +<script src="../editing.js" language="JavaScript" type="text/JavaScript" ></script>

nit: language and type attributes are not needed.

> LayoutTests/editing/deleting/delete-surrogatepair.html:38
> +             //adocument.execCommand("MoveForward");

Unused code should be removed.
Comment 10 Shinya Kawanaka 2011-09-05 21:28:15 PDT
Created attachment 106386 [details]
Patch
Comment 11 Shinya Kawanaka 2011-09-05 21:28:47 PDT
(In reply to comment #9)
> (From update of attachment 106382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106382&action=review
> 
> > LayoutTests/editing/deleting/delete-surrogatepair.html:10
> > +<script src="../editing.js" language="JavaScript" type="text/JavaScript" ></script>
> 
> nit: language and type attributes are not needed.
> 
> > LayoutTests/editing/deleting/delete-surrogatepair.html:38
> > +             //adocument.execCommand("MoveForward");
> 
> Unused code should be removed.

Sorry, fixed them.
Comment 12 Kent Tamura 2011-09-11 21:29:43 PDT
Comment on attachment 106386 [details]
Patch

ok
Comment 13 WebKit Review Bot 2011-09-11 22:46:34 PDT
Comment on attachment 106386 [details]
Patch

Rejecting attachment 106386 [details] from commit-queue.

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

Last 500 characters of output:
pp
	M	Source/WebCore/bindings/v8/OptionsObject.h
	M	Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp
r94946 = 2009367ae0a0b4f627ac5aecb39e8cab82b59735 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9631958
Comment 14 Kent Tamura 2011-09-11 22:52:03 PDT
Comment on attachment 106386 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=40351
> +
> +        If a character is the trail part of unicode surrogate pair, the lead part of it

There is no "Reviewed by NOBODY (OOPS!)" line.
Comment 15 Shinya Kawanaka 2011-09-11 22:59:56 PDT
(In reply to comment #14)
> (From update of attachment 106386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106386&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=40351
> > +
> > +        If a character is the trail part of unicode surrogate pair, the lead part of it
> 
> There is no "Reviewed by NOBODY (OOPS!)" line.

!! Sorry, I will fix it soon.
Comment 16 Shinya Kawanaka 2011-09-11 23:12:26 PDT
Created attachment 107024 [details]
Patch
Comment 17 WebKit Review Bot 2011-09-12 00:13:44 PDT
Comment on attachment 107024 [details]
Patch

Clearing flags on attachment: 107024

Committed r94947: <http://trac.webkit.org/changeset/94947>
Comment 18 WebKit Review Bot 2011-09-12 00:13:49 PDT
All reviewed patches have been landed.  Closing bug.