Bug 40351 – [chromium] Can't handle deletion of characters beyond U+10000.

• Component: Text
• Status: RESOLVED FIXED

[James Su]

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

[James Su]

I reproduced it with the latest Chrome. But seems that Safari 4.0 doesn't have this problem.

[Alexey Proskuryakov]

Yes, I also cannot reproduce this in Safari.

[Shinya Kawanaka]

Created attachment 106334 [details]
Patch

[Alexey Proskuryakov]

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]))

[Kent Tamura]

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.

[Kent Tamura]

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?

[Shinya Kawanaka]

Created attachment 106382 [details]
Patch

[Shinya Kawanaka]

(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.

[Kent Tamura]

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.

[Shinya Kawanaka]

Created attachment 106386 [details]
Patch

[Shinya Kawanaka]

(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.

[Kent Tamura]

Comment on attachment 106386 [details]
Patch

ok

[WebKit Review Bot]

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

[Kent Tamura]

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.

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 107024 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107024 [details]
Patch

Clearing flags on attachment: 107024

Committed r94947: <http://trac.webkit.org/changeset/94947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.