Bug 87041

Summary: Ideographic comma and full-stops are mishandled in linebreak
Product: WebKit Reporter: Jungshik Shin <jshin>
Component: Layout and RenderingAssignee: mitz
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, eric, mitz, ossy, webkit.review.bot, xji
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 89426, 89436    
Bug Blocks:    
Attachments:
Description Flags
Remove the unconditional line breaking opportunity after ideographic commas and full stops ap: review+

Description Jungshik Shin 2012-05-21 13:17:05 PDT
Copied from our internal investigation: 

----------

The problem is in shouldBreakAfter() of Source/WebCore/rendering/break_lines.cpp.

 static inline bool shouldBreakAfter(UChar ch, UChar nextCh)
 {
   switch (ch) {
   case ideographicComma:
   case ideographicFullStop:
       // FIXME: cases for ideographicComma and ideographicFullStop are a workaround for an issue in Unicode 5.0
       // which is likely to be resolved in Unicode 5.1 <http://bugs.webkit.org/show_bug.cgi?id=17411>.
       // We may want to remove or conditionalize this workaround at some point.

       return true;
   default:
   ....


shouldBreakAfter() hard coded a line break after ideographicComma and ideographicFullStop regardless any other punctuation right after these 2 characters. 
-----------------

The above issue is not present any more in ICU 4.0 or later. Chrome uses ICU 4.6. The other major port that uses ICU is Safari. I'm sure Safari Windows uses ICU 4.0 or later. The question boils down to the version of ICU on the earliest version of Mac OS X that Safari has to support. If it's ICU 4.0 or alter, we can just remove two hard-coded cases above (ideographicComma and ideographicFullStop). 

Otherwise, either we have to detect the ICU version at run-time and do different things or at least we have to fix Chromium and Safari on Windows with #if-def (build-time check). 


darin or ap, can you tell us the version of ICU on the earliest version of Mac OS X Safari has to support?
Comment 1 Darin Adler 2012-05-22 13:31:09 PDT
ICU 4.0 is there on Snow Leopard, so the only issue is Leopard. The Apple folks are not expecting to support Leopard with the current WebKit TOT, so it would be OK to remove this completely from the Apple Mac port point of view.
Comment 2 Jungshik Shin 2012-05-30 15:02:23 PDT
Thank you, Darin, for the reply. (I was OOO for a while). I'll make a patch to remove it.
Comment 3 mitz 2012-06-18 09:56:37 PDT
<rdar://problem/11215797>
Comment 4 mitz 2012-06-18 13:15:44 PDT
Created attachment 148157 [details]
Remove the unconditional line breaking opportunity after ideographic commas and full stops
Comment 5 mitz 2012-06-18 13:40:51 PDT
Fixed in <http://trac.webkit.org/r120624>.
Comment 6 Csaba Osztrogonác 2012-06-18 23:06:34 PDT
(In reply to comment #5)
> Fixed in <http://trac.webkit.org/r120624>.

It made a test fail on Qt. Could you check it, please? I file a new bug report to track this new regression - https://bugs.webkit.org/show_bug.cgi?id=89426
Comment 7 mitz 2012-06-18 23:12:11 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Fixed in <http://trac.webkit.org/r120624>.
> 
> It made a test fail on Qt. Could you check it, please? I file a new bug report to track this new regression - https://bugs.webkit.org/show_bug.cgi?id=89426

That’s an odd way to put it. The test was added as part of this change.
Comment 8 Csaba Osztrogonác 2012-06-18 23:23:26 PDT
(In reply to comment #7)
> That’s an odd way to put it. The test was added as part of this change.
Oh, I overlooked it, I fixed the title of the bug.