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
I reproduced it with the latest Chrome. But seems that Safari 4.0 doesn't have this problem.
Yes, I also cannot reproduce this in Safari.
Created attachment 106334 [details] Patch
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 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 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?
Created attachment 106382 [details] Patch
(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 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.
Created attachment 106386 [details] Patch
(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 on attachment 106386 [details] Patch ok
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 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.
(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.
Created attachment 107024 [details] Patch
Comment on attachment 107024 [details] Patch Clearing flags on attachment: 107024 Committed r94947: <http://trac.webkit.org/changeset/94947>
All reviewed patches have been landed. Closing bug.