Bug 10842

Summary: REGRESSION (r15418): contenteditable div truncates rightmost Japanese character
Product: WebKit Reporter: Dan Wood <dwood>
Component: HTML EditingAssignee: Graham Dennis <Graham.Dennis>
Status: RESOLVED FIXED    
Severity: Major CC: ap, Graham.Dennis, mitz
Priority: P1 Keywords: EasyFix, HasReduction, Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
test case
none
OK, it looks like I can't type Japanese into Bugzilla. I've attached my report as a UTF16 text file.
none
patch
none
patch 2 hyatt: review+

Description Dan Wood 2006-09-13 11:10:42 PDT
When a div containing Japanese text is content-editable, the rightmost character does not display; it's truncated.

My test source text in Japanese (understanding the words it not needed; just look at the glyphs) is:

???????? ??????????????????????????????????Edgies?????US$9.99???????????????????????????????????

I've inserted hard returns in the text to see how the text wraps in the current released version of Safari/WebKit:

???????? ??
???????????
???????????
??????????
Edgies?????
US$9.99??????
???????????
???????????
???????

In TOT Webkit, if contentediting is not true, then the above is how it wraps -- that's correct.

However, if contentediting is true (focused or not), most of the rightmost characters (the ones near the right margin) are missing!  This is what you see:

???????? ?             missing: ?
??????????           missing:?
??????????           missing:?
??????????
Edgies?????
US$9.99?????              missing: ?
??????????           missing:?
??????????           missing:?
???????
Comment 1 Dan Wood 2006-09-13 11:11:45 PDT
Created attachment 10538 [details]
test case
Comment 2 Dan Wood 2006-09-13 11:14:43 PDT
Created attachment 10539 [details]
OK, it looks like I can't type Japanese into Bugzilla.  I've attached my report as a UTF16 text file.
Comment 3 Alexey Proskuryakov 2006-09-13 21:30:39 PDT
Regression->P1
Comment 4 mitz 2006-09-14 01:00:27 PDT
Oops, this is a regression from r15418 http://trac.webkit.org/projects/webkit/changeset/15418 (fix for bug 9670).
Comment 5 mitz 2006-09-14 23:35:55 PDT
Looks like an easy fix: there are two places in RenderBlock::findNextLineBreak() that use the condition "o->style()->breakOnlyAfterWhiteSpace() && !midWordBreak": when setting charWidth and in the second if statement after that. You should add "currentCharacterIsWS && " in front of the condition (it is really only for checking whether to add a run of whitespace to the line even though it makes it too wide).
Comment 6 Graham Dennis 2006-09-17 02:04:35 PDT
Created attachment 10598 [details]
patch

Patch using Mitz's suggesstion.

I also did a logic shuffle at Mitz's request to prevent re-testing the same expression twice in quick succession in two if statements (currentCharacterIsWS && o->style()->breakOnlyAfterWhiteSpace() && !midWordBreak)
Comment 7 David Kilzer (:ddkilzer) 2006-09-17 08:29:03 PDT
Comment on attachment 10598 [details]
patch

>Index: LayoutTests/fast/text/line-breaks-after-white-space.html
>===================================================================
>Cannot display: file marked as a binary type.
>svn:mime-type = application/octet-stream
>
>Property changes on: LayoutTests/fast/text/line-breaks-after-white-space.html
>___________________________________________________________________
>Name: svn:mime-type
>   + application/octet-stream

The layout test should not have its svn mime type set to application/octet-stream, should it?  Did this happen automatically because of the unicode characters in the file?
Comment 8 Graham Dennis 2006-09-17 15:24:28 PDT
Created attachment 10609 [details]
patch 2

Originally the layout test had Japanese characters in UTF16, but I ended up changing the patch to only use latin characters. Anyway, the mime type shouldn't have been application/octet-stream, and that's fixed now.
Comment 9 Dave Hyatt 2006-09-18 23:17:14 PDT
Comment on attachment 10609 [details]
patch 2

Make sure all other layout tests pass.  r=me.
Comment 10 Graham Dennis 2006-09-24 06:11:50 PDT
(In reply to comment #9)
> (From update of attachment 10609 [details] [edit])
> Make sure all other layout tests pass.  r=me.
> 

It doesn't break any layout tests.
Comment 11 Alexey Proskuryakov 2006-10-01 02:09:26 PDT
Committed revision 16696.