Last Comment Bug 765166 - IDEOGRAPHIC SPACE (U+3000) should cause line break after a white space
: IDEOGRAPHIC SPACE (U+3000) should cause line break after a white space
Status: RESOLVED FIXED
: intl
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
:
Mentors:
http://www.mikariole.com/shop/index.html
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-15 02:42 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-06-16 19:41 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.11 KB, text/html)
2012-06-15 02:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details
Patch (4.73 KB, patch)
2012-06-15 02:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (4.75 KB, patch)
2012-06-16 04:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-15 02:42:46 PDT
Created attachment 633446 [details]
testcase

Reported from fxinput: http://input.mozilla.org/opinion/3001588

It says that this page layout is broken only on Fx.
http://www.mikariole.com/shop/index.html

The page specifies the width of table as 760px but actual width is wider than it because there is a line which has only IDEOGRAPHIC SPACEs (U+3000).

See the attached testcase. On WebKit and Opera, the line is broken between them. I'm not sure for IE9. IE9 doesn't break the page layout but doesn't break lines on the testcase.

Anyway, we should break lines before IDEOGRAPHIC SPACE after a white space.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-15 02:43:21 PDT
Created attachment 633447 [details] [diff] [review]
Patch
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-15 09:11:33 PDT
Comment on attachment 633447 [details] [diff] [review]
Patch

Review of attachment 633447 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsLineBreaker.cpp
@@ +225,5 @@
> +        mBreakHere ||
> +        // U+3000 is IDEOGRAPHIC SPACE.  Japanese authors may use it instead of
> +        // ASCII white space accidentally.  Like WebKit and Opera, we should
> +        // allow to break lines before it.
> +        (mAfterBreakableSpace && (!isBreakableSpace || ch == 0x3000)) ||

This is a bit complicated.

This code is trying not to break inside runs of breakable spaces, only at the end of a run of breakable spaces. For normal spaces that usually can't produce long lines since runs normal spaces are compressed by default. But 0x3000 isn't compressed by nsTextFrameUtils::TransformText (and they're not compressed by any other browser, so we shouldn't start doing that.

Opera and Webkit allow breaking both before and after 0x3000, which your patch doesn't do.

Could we remove 0x3000 from IsSpace and/or NS_IsSpace, and rely on nsILineBreaker to return allowed break positions before and after 0x3000? That might be the cleanest way to fix this.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-16 04:24:44 PDT
Created attachment 633794 [details] [diff] [review]
Patch

Okay, this doesn't hurt other parts calling the method.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-16 04:25:47 PDT
see also https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a41b43d0bea0
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-16 07:50:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebd8e4ceb51
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-06-16 19:41:31 PDT
https://hg.mozilla.org/mozilla-central/rev/6ebd8e4ceb51

Note You need to log in before you can comment on or make changes to this bug.