Summary: | CSS Multicolumn text is split awkwardly | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anantha Keesara <anantha> | ||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | eric, hamaji, hyatt, yutak | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||
OS: | Windows XP | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Anantha Keesara
2008-11-13 17:29:20 PST
Created attachment 25149 [details]
reduction
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! |