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
Created attachment 25149 [details] reduction
Confirmed with WebKit-r38860
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(-)
Created attachment 30859 [details] single-line-expected.png
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(-)
Created attachment 30861 [details] single-line-expected.png
Sorry, I updated the patch because I put the URL of chromium bug instead of this bug in layout test of the first patch.
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(-)
Hyatt would know most about this support.
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 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 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.
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!
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.
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 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 on attachment 32700 [details] Patch v5 Clearing review flags, since above comments suggest this patch has been withdrawn.
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(-)
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 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 on attachment 35028 [details] Patch v6 r- for above mentioned nits.
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(-)
Created attachment 39041 [details] single-line-expected.png (revised^2)
Another try. I fixed the typos and changed the code slightly for clarification.
This looks fine to me. I'll email hyatt before I r+ since he's been silent for the last 3 months...
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.
Created attachment 39321 [details] Prevent text inside a multi-column block from being split into columns. (fixed comment)
Thanks Dave. I've fixed the comment mentioned above. Could you give another look at this?
Hi hyatt, could you please take a time and review this patch?
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.
Created attachment 41716 [details] Prevent text inside a multi-column block from being split into columns. (Moved tests)
Comment on attachment 41716 [details] Prevent text inside a multi-column block from being split into columns. (Moved tests) Looks OK.
Committed r50869: <http://trac.webkit.org/changeset/50869>
(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.
> Ah, I think this will make case test fails in several environments. I'll fix > this. Done. http://trac.webkit.org/changeset/50870
(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!