Bug 22249 - CSS Multicolumn text is split awkwardly
Summary: CSS Multicolumn text is split awkwardly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-13 17:29 PST by Anantha Keesara
Modified: 2009-11-12 00:41 PST (History)
4 users (show)

See Also:


Attachments
reduction (142 bytes, text/html)
2008-11-13 17:30 PST, Anantha Keesara
no flags Details
patch v1 (4.59 KB, patch)
2009-06-02 03:38 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
single-line-expected.png (18.52 KB, image/png)
2009-06-02 03:40 PDT, Shinichiro Hamaji
no flags Details
patch v2 (4.81 KB, patch)
2009-06-02 03:49 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
single-line-expected.png (18.14 KB, image/png)
2009-06-02 03:49 PDT, Shinichiro Hamaji
no flags Details
patch v3 (4.82 KB, patch)
2009-06-02 04:30 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
patch v4 (4.86 KB, patch)
2009-06-04 21:57 PDT, Shinichiro Hamaji
hyatt: review-
Details | Formatted Diff | Diff
Patch v5 (8.02 KB, patch)
2009-07-14 01:51 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v6 (11.41 KB, patch)
2009-08-18 01:36 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
single-line-expected.png (revised) (33.19 KB, image/png)
2009-08-18 01:43 PDT, Yuta Kitamura
no flags Details
Prevent text inside a multi-column block from being split into columns. (12.06 KB, patch)
2009-09-04 00:03 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
single-line-expected.png (revised^2) (33.15 KB, image/png)
2009-09-04 00:05 PDT, Yuta Kitamura
no flags Details
Prevent text inside a multi-column block from being split into columns. (fixed comment) (12.05 KB, patch)
2009-09-09 21:27 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Prevent text inside a multi-column block from being split into columns. (Moved tests) (11.95 KB, patch)
2009-10-23 00:10 PDT, Yuta Kitamura
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anantha Keesara 2008-11-13 17:29:20 PST
Steps:
Launch attached testcase

What is the expected result?
Should produce the text:
Testing multicolumn css support

What happens instead?
Text is split across two lines

Nightly tested:WebKit-r38192

Other Browsers: 
    Safari 3: FAIL
    Firefox 3: OK
         IE 7: OK (no multicolumn support)

Chromium Bug: 
http://code.google.com/p/chromium/issues/detail?id=4407
Comment 1 Anantha Keesara 2008-11-13 17:30:38 PST
Created attachment 25149 [details]
reduction
Comment 2 Stéphane Marguet 2008-12-03 10:02:08 PST
Confirmed with WebKit-r38860
Comment 3 Shinichiro Hamaji 2009-06-02 03:38:36 PDT
Created attachment 30858 [details]
patch v1

 LayoutTests/ChangeLog                              |   11 +++++++++++
 LayoutTests/fast/multicol/single-line.html         |    9 +++++++++
 .../fast/multicol/single-line-expected.checksum    |    1 +
 .../mac/fast/multicol/single-line-expected.png     |  Bin 0 -> 18969 bytes
 .../mac/fast/multicol/single-line-expected.txt     |   13 +++++++++++++
 WebCore/ChangeLog                                  |   11 +++++++++++
 WebCore/rendering/RenderBlock.cpp                  |    4 ++--
 7 files changed, 47 insertions(+), 2 deletions(-)
Comment 4 Shinichiro Hamaji 2009-06-02 03:40:23 PDT
Created attachment 30859 [details]
single-line-expected.png
Comment 5 Shinichiro Hamaji 2009-06-02 03:49:43 PDT
Created attachment 30860 [details]
patch v2

 LayoutTests/ChangeLog                              |   11 +++++++++++
 LayoutTests/fast/multicol/single-line.html         |    9 +++++++++
 .../fast/multicol/single-line-expected.checksum    |    1 +
 .../mac/fast/multicol/single-line-expected.png     |  Bin 0 -> 18575 bytes
 .../mac/fast/multicol/single-line-expected.txt     |   18 ++++++++++++++++++
 WebCore/ChangeLog                                  |   11 +++++++++++
 WebCore/rendering/RenderBlock.cpp                  |    4 ++--
 7 files changed, 52 insertions(+), 2 deletions(-)
Comment 6 Shinichiro Hamaji 2009-06-02 03:49:55 PDT
Created attachment 30861 [details]
single-line-expected.png
Comment 7 Shinichiro Hamaji 2009-06-02 03:52:01 PDT
Sorry, I updated the patch because I put the URL of chromium bug instead of this bug in layout test of the first patch.
Comment 8 Shinichiro Hamaji 2009-06-02 04:30:10 PDT
Created attachment 30863 [details]
patch v3

 LayoutTests/ChangeLog                              |   11 +++++++++++
 LayoutTests/fast/multicol/single-line.html         |    9 +++++++++
 .../fast/multicol/single-line-expected.checksum    |    1 +
 .../mac/fast/multicol/single-line-expected.png     |  Bin 0 -> 18575 bytes
 .../mac/fast/multicol/single-line-expected.txt     |   18 ++++++++++++++++++
 WebCore/ChangeLog                                  |   11 +++++++++++
 WebCore/rendering/RenderBlock.cpp                  |    4 ++--
 7 files changed, 52 insertions(+), 2 deletions(-)
Comment 9 Eric Seidel (no email) 2009-06-02 13:20:55 PDT
Hyatt would know most about this support.
Comment 10 Shinichiro Hamaji 2009-06-04 21:57:15 PDT
Created attachment 30991 [details]
patch v4

 LayoutTests/ChangeLog                              |   13 +++++++++++++
 LayoutTests/fast/multicol/single-line.html         |    9 +++++++++
 .../fast/multicol/single-line-expected.checksum    |    1 +
 .../mac/fast/multicol/single-line-expected.png     |  Bin 0 -> 18575 bytes
 .../mac/fast/multicol/single-line-expected.txt     |   18 ++++++++++++++++++
 WebCore/ChangeLog                                  |   13 +++++++++++++
 WebCore/rendering/RenderBlock.cpp                  |    4 ++--
 7 files changed, 56 insertions(+), 2 deletions(-)
Comment 11 Shinichiro Hamaji 2009-06-04 21:58:20 PDT
Comment on attachment 30991 [details]
patch v4

Inserted some line breaks in ChangeLog as I was advised in another review that we should keep lines in ChangeLog short.
Comment 12 Dave Hyatt 2009-06-06 15:10:35 PDT
Comment on attachment 30991 [details]
patch v4

I don't think this is right.  It seems like it will result in truncation simply not happening in cases where it needs to.
Comment 13 Shinichiro Hamaji 2009-06-06 16:08:02 PDT
Thanks for the review!
> I don't think this is right.  It seems like it will result in truncation simply
> not happening in cases where it needs to.
For which case this change doesn't truncate when it should truncate? This change sets too big value (currY+availableHeight) into m_truncatedAt and m_bestTruncatedAt, but I think that m_bestTruncatedAt will be updated to correct value in latter paintObject() call. If I understand correctly, m_truncatedAt acts as the maximum value of m_bestTruncatedAt in RenderLineBoxList::paint(). This bug happens because the maximum value m_truncatedAt is too small. If we set currY+availableHeight into m_truncatedAt, RenderLineBoxList::paint() seems to set the maximum value which is allowed by RenderView::printRect, whose height is still colHeight.
FYI, this change passed all existing layout tests including fast/multicol on Leopard.
Honestly saying, I'm still unfamiliar with code around this. Any kind of advices will be really appreciated.
Thanks!
Comment 14 Yuta Kitamura 2009-07-14 01:51:13 PDT
Created attachment 32700 [details]
Patch v5

I take over this bug from Shinichiro. Please review this patch.

This patch adds code that checks if a multi-column element has the enough height that can hold the text inside.
Comment 15 Yuta Kitamura 2009-07-14 01:53:25 PDT
 LayoutTests/ChangeLog                              |   17 ++++++++++++++
 LayoutTests/fast/multicol/single-line.html         |    9 +++++++
 .../fast/multicol/single-line-expected.checksum    |    1 +
 .../mac/fast/multicol/single-line-expected.png     |  Bin 0 -> 18575 bytes
 .../mac/fast/multicol/single-line-expected.txt     |   18 +++++++++++++++
 WebCore/ChangeLog                                  |   24 ++++++++++++++++++++
 WebCore/rendering/RenderBlock.cpp                  |   12 +++++++--
 WebCore/rendering/RenderBlock.h                    |    2 +-
 WebCore/rendering/RenderLineBoxList.cpp            |    2 +
 WebCore/rendering/RenderView.h                     |    5 +++-
 10 files changed, 85 insertions(+), 5 deletions(-)
Comment 16 Shinichiro Hamaji 2009-07-14 02:11:39 PDT
Comment on attachment 30991 [details]
patch v4

I withdraw this patch as Yuta showed me the cases where my patch didn't work. Thanks Yuta for working on this!
Comment 17 Eric Seidel (no email) 2009-07-23 23:29:40 PDT
Comment on attachment 32700 [details]
Patch v5

Clearing review flags, since above comments suggest this patch has been withdrawn.
Comment 18 Yuta Kitamura 2009-08-18 01:36:50 PDT
Created attachment 35028 [details]
Patch v6

If the tentative height of a multi-column block was too small, we need to
expand the block height and try to layout again, in order to prevent text
from being splitted into different columns.
---
 10 files changed, 155 insertions(+), 7 deletions(-)
Comment 19 Yuta Kitamura 2009-08-18 01:43:59 PDT
Created attachment 35029 [details]
single-line-expected.png (revised)

Layout test "single-line.html" has been updated. Please use this image as the expected result of pixel-tests.
Comment 20 Eric Seidel (no email) 2009-08-21 09:52:07 PDT
Comment on attachment 35028 [details]
Patch v6

"split" not "splitted":
+        from being splitted into different columns.

(unless that's british english that I don't know...)

I'm confused why this talks about original and uses minimum:
41     if (minimumColumnHeight >= 0) {
 3842         // If originalColHeight was too small, we need to try to layout again.
 3843         return layoutColumns(endOfContent, minimumColumnHeight);
 3844     }

Seems like this could use a comment:
3756     if (computeIntrinsicHeight) {
 3757         if (requestedColumnHeight < 0)
 3758             colHeight += columnSlop;
 3759         else
 3760             colHeight = requestedColumnHeight;
 3761     }
 3762     int originalColHeight = colHeight;
Also, it's strange to see "original" being set after modification.  I think I'm just not familiar with this code.

I think you should try to track dhyatt down on irc and ask him nicely to review this small patch.
Comment 21 Eric Seidel (no email) 2009-09-03 00:28:17 PDT
Comment on attachment 35028 [details]
Patch v6

r- for above mentioned nits.
Comment 22 Yuta Kitamura 2009-09-04 00:03:39 PDT
Created attachment 39040 [details]
Prevent text inside a multi-column block from being split into columns.

If the tentative height of a multi-column block was too small, we need to
expand the block height and try to layout again, in order to prevent text
from being split into different columns.
---
 10 files changed, 157 insertions(+), 11 deletions(-)
Comment 23 Yuta Kitamura 2009-09-04 00:05:24 PDT
Created attachment 39041 [details]
single-line-expected.png (revised^2)
Comment 24 Yuta Kitamura 2009-09-04 00:09:02 PDT
Another try.

I fixed the typos and changed the code slightly for clarification.
Comment 25 Eric Seidel (no email) 2009-09-09 11:06:17 PDT
This looks fine to me.  I'll email hyatt before I r+ since he's been silent for the last 3 months...
Comment 26 Dave Hyatt 2009-09-09 14:24:46 PDT
Comment on attachment 39040 [details]
Prevent text inside a multi-column block from being split into columns.

"// Initial column height was too small to put some line of text."

Change to:

"// The initial column height was too small to contain one line of text."

Patch looks fine aside from that.
Comment 27 Yuta Kitamura 2009-09-09 21:27:34 PDT
Created attachment 39321 [details]
Prevent text inside a multi-column block from being split into columns. (fixed comment)
Comment 28 Yuta Kitamura 2009-09-09 21:29:44 PDT
Thanks Dave. I've fixed the comment mentioned above. Could you give another look at this?
Comment 29 Yuta Kitamura 2009-09-23 13:16:18 PDT
Hi hyatt, could you please take a time and review this patch?
Comment 30 Eric Seidel (no email) 2009-10-21 12:45:20 PDT
Comment on attachment 39321 [details]
Prevent text inside a multi-column block from being split into columns. (fixed comment)

As far as I can tell, this looks fine to me.

Yuta, this will cause failure on other platforms since you're only adding Mac results.

We have 3 options:
1.  Move the results next to the tests instead of in platform/mac.
2.  Add results for other platforms (only necessary if they differ, otherwise #1 is better).
3.  Make these tests dumpAsText (possible by using getComputedStyle).

If the tests were OK, I would r+ this.
Comment 31 Yuta Kitamura 2009-10-23 00:10:34 PDT
Created attachment 41716 [details]
Prevent text inside a multi-column block from being split into columns. (Moved tests)
Comment 32 Eric Seidel (no email) 2009-11-10 14:32:40 PST
Comment on attachment 41716 [details]
Prevent text inside a multi-column block from being split into columns. (Moved tests)

Looks OK.
Comment 33 Shinichiro Hamaji 2009-11-12 00:16:32 PST
Committed r50869: <http://trac.webkit.org/changeset/50869>
Comment 34 Shinichiro Hamaji 2009-11-12 00:28:25 PST
(In reply to comment #33)
> Committed r50869: <http://trac.webkit.org/changeset/50869>

Ah, I think this will make case test fails in several environments. I'll fix this.
Comment 35 Shinichiro Hamaji 2009-11-12 00:38:24 PST
> Ah, I think this will make case test fails in several environments. I'll fix
> this.

Done.

http://trac.webkit.org/changeset/50870
Comment 36 Yuta Kitamura 2009-11-12 00:41:34 PST
(In reply to comment #35)
> > Ah, I think this will make case test fails in several environments. I'll fix
> > this.
> 
> Done.
> 
> http://trac.webkit.org/changeset/50870

Thank you for taking care of this!