Bug 22249 – CSS Multicolumn text is split awkwardly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Anantha Keesara]

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

[Anantha Keesara]

Created attachment 25149 [details]
reduction

[Stéphane Marguet]

Confirmed with WebKit-r38860

[Shinichiro Hamaji]

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(-)

[Shinichiro Hamaji]

Created attachment 30859 [details]
single-line-expected.png

[Shinichiro Hamaji]

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(-)

[Shinichiro Hamaji]

Created attachment 30861 [details]
single-line-expected.png

[Shinichiro Hamaji]

Sorry, I updated the patch because I put the URL of chromium bug instead of this bug in layout test of the first patch.

[Shinichiro Hamaji]

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(-)

[Eric Seidel (no email)]

Hyatt would know most about this support.

[Shinichiro Hamaji]

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(-)

[Shinichiro Hamaji]

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.

[Dave Hyatt]

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.

[Shinichiro Hamaji]

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!

[Yuta Kitamura]

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.

[Yuta Kitamura]

 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(-)

[Shinichiro Hamaji]

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!

[Eric Seidel (no email)]

Comment on attachment 32700 [details]
Patch v5

Clearing review flags, since above comments suggest this patch has been withdrawn.

[Yuta Kitamura]

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(-)

[Yuta Kitamura]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 35028 [details]
Patch v6

r- for above mentioned nits.

[Yuta Kitamura]

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(-)

[Yuta Kitamura]

Created attachment 39041 [details]
single-line-expected.png (revised^2)

[Yuta Kitamura]

Another try.

I fixed the typos and changed the code slightly for clarification.

[Eric Seidel (no email)]

This looks fine to me.  I'll email hyatt before I r+ since he's been silent for the last 3 months...

[Dave Hyatt]

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.

[Yuta Kitamura]

Created attachment 39321 [details]
Prevent text inside a multi-column block from being split into columns. (fixed comment)

[Yuta Kitamura]

Thanks Dave. I've fixed the comment mentioned above. Could you give another look at this?

[Yuta Kitamura]

Hi hyatt, could you please take a time and review this patch?

[Eric Seidel (no email)]

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.

[Yuta Kitamura]

Created attachment 41716 [details]
Prevent text inside a multi-column block from being split into columns. (Moved tests)

[Eric Seidel (no email)]

Comment on attachment 41716 [details]
Prevent text inside a multi-column block from being split into columns. (Moved tests)

Looks OK.

[Shinichiro Hamaji]

Committed r50869: <http://trac.webkit.org/changeset/50869>

[Shinichiro Hamaji]

(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.

[Shinichiro Hamaji]

> Ah, I think this will make case test fails in several environments. I'll fix
> this.

Done.

http://trac.webkit.org/changeset/50870

[Yuta Kitamura]

(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!