Bug 53239 – Wrapping text overlaps floated element(s) when there are 2 or more adjacent floated elements

• Component: Layout and Rendering
• Status: ASSIGNED

[Dan]

Created attachment 80332 [details]
HTML demonstrating the problem

When paragraph text wraps around left-floated elements, if there are 2 or more adjacent floated elements of increasing width, the paragraph text incorrectly overlaps the floated elements for one line before correctly wrapping around it.

For example:

        <p>text...</p> 

        <div class="floatedLeftWidth100">
            <p>Any old content</p>
        </div>
        <div class="floatedLeftWidth150"> 
            <p>Any old content</p>
        </div>
        <div class="floatedLeftWidth200"> 
            <p>Any old content</p>
        </div>
        
        <p>text... The text in here will overlap the second and third floated elements for one line before wrapping correctly.</p> 
        
        <p>text...</p> 

See attached example test page.

[Jacky Jiang]

Created attachment 99633 [details]
Safari screenshot BBC news

Text in paragraph overlaps image on BBC news site http://www.bbc.co.uk/news/world-europe-13930073. 
Reproducible on latest Safari and QT build, floating elements are on the right side in this case, attach srceenshots.
Try to zoom page or change text font size if this page is not reproducible to you.

[Jacky Jiang]

Created attachment 99634 [details]
QT screenshot BBC news

[Jacky Jiang]

Created attachment 99721 [details]
test case

[Simon Fraser (smfr)]

This seems to be QT-specific.

[Jacky Jiang]

Created attachment 99725 [details]
Patch

[Simon Fraser (smfr)]

Why change RenderBlock.cpp when this only affects Qt?

[Antonio Gomes]

(In reply to comment #6)
> Why change RenderBlock.cpp when this only affects Qt?

See attachment 99633 [details]. It is not Qt specific.

[WebKit Review Bot]

Comment on attachment 99725 [details]
Patch

Attachment 99725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8983704

New failing tests:
css1/formatting_model/floating_elements.html

[WebKit Review Bot]

Created attachment 99730 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jacky Jiang]

(In reply to comment #8)
> (From update of attachment 99725 [details])
> Attachment 99725 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8983704
> 
> New failing tests:
> css1/formatting_model/floating_elements.html
In this test case, there are 4 places mismatched after my patch applied.
The bottom of the text line overlaps the float below in these 4 places, search "In addition, they should not overlap each other in any way" and see the top right text in the block. 
One of the original intention of the test case described on the page is:
"In addition, they should not overlap each other in any way, nor should the floated elements be overwritten by the DIV text."  
My patch actually avoids the overlap and improves it. I will update those expected results of this test case and upload updated patch.

[Jacky Jiang]

Created attachment 99832 [details]
Patch

[Jacky Jiang]

Update expected results for floating_elements.html on QT as the overlaps in this test case are avoided and the results improved with this patch. I don't have the other ports, the expected results on the other ports should also be updated:
LayoutTests/platform/mac/css1/formatting_model/floating_elements-expected.png
LayoutTests/platform/mac/css1/formatting_model/floating_elements-expected.txt
LayoutTests/platform/gtk/css1/formatting_model/floating_elements-expected.txt
LayoutTests/platform/chromium-linux/css1/formatting_model/floating_elements-expected.png
LayoutTests/platform/chromium-win/css1/formatting_model/floating_elements-expected.png
LayoutTests/platform/chromium-win/css1/formatting_model/floating_elements-expected.txt
LayoutTests/platform/mac-leopard/css1/formatting_model/floating_elements-expected.png

[WebKit Review Bot]

Comment on attachment 99832 [details]
Patch

Attachment 99832 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8995036

New failing tests:
css1/formatting_model/floating_elements.html

[WebKit Review Bot]

Created attachment 99834 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 99832 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99832&action=review

It's hard to judge about this patch, without a more detailed description - can you please come up with more details? :-)

> LayoutTests/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line belongs before your description.

> LayoutTests/ChangeLog:15
> +        * fast/block/float/text-in-paragraph-overlapping-floats.html: Added.
> +        * platform/qt/fast/block/float/text-in-paragraph-overlapping-floats-expected.png: Added.
> +        * platform/qt/fast/block/float/text-in-paragraph-overlapping-floats-expected.txt: Added.
> +        * platform/qt/css1/formatting_model/floating_elements-expected.txt

You need to generate results with a mac and include them as well.

> Source/WebCore/ChangeLog:8
> +        Text in paragraph overlapping adjacent floats.
> +        Shrink logical offset for line to avoid adjacent floats when the tops of floats are just
> +        between the top and bottom of the line.

This should be more verbose. Tell us what's the problem first and then how you fixed it, currently you're just describing what you've changed.
I'm not understanding it at present.

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line belongs before your description.

[Rob Buis]

Created attachment 99841 [details]
mac results for new test and changed test result

[Jacky Jiang]

Created attachment 99853 [details]
Patch

[Jacky Jiang]

Updated the change log and added mac results for the patch according to the comments of Nikolas.

[WebKit Review Bot]

Comment on attachment 99853 [details]
Patch

Attachment 99853 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8991272

New failing tests:
fast/block/float/text-in-paragraph-overlapping-floats.html
css1/formatting_model/floating_elements.html

[WebKit Review Bot]

Created attachment 99857 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Note that we think there may be an additional problem regarding css1/formatting_model/floating_elements.html. The text result is fine, but the png seems to only test the first page (see the scrollbar). From the text results, the difference starts around height 1100 px in the page. So it is hard to judge what the exact pixel difference is and whether it it an improvement. But for someone familiar with that test it may be easier. Just thought I'd point that one out.
Cheers,

Rob.

[Dave Hyatt]

Comment on attachment 99853 [details]
Patch

I think it's wrong to patch logicalLeftOffsetForLine and logicalRightOffsetForLine. Those methods are behaving correctly in the sense that they just look at a particular y-coordinate and act accordingly.

Really the issue is that we only sample at the top of the line in the line layout code. The reason we do this is that we don't actually know how tall the line is going to be. Your patch is trying to just use the line-height of the block, but of course that isn't necessarily how tall the line of text will end up being. In quirks mode it can be shorter than that, and if there are taller items on the line, it can be larger.

This has been a known issue for years... I just haven't come up with a great way to address it without having to "be wrong" and do another pass (which can itself be cyclic if you keep changing your mind).

I think using the line-height of the block is somewhat reasonable, but it's tricky. The two issues I can think of with using the line-height of the block as a guide are that in quirks mode the descent may go away, and then you have to consider -webkit-line-box-contain also, since if the block isn't being allowed to contribute to line-height, it definitely seems wrong to use it.

This is a really challenging bug to fix.

[Jacky Jiang]

(In reply to comment #22)
> (From update of attachment 99853 [details])
> I think it's wrong to patch logicalLeftOffsetForLine and logicalRightOffsetForLine. Those methods are behaving correctly in the sense that they just look at a particular y-coordinate and act accordingly.
> 
> Really the issue is that we only sample at the top of the line in the line layout code. The reason we do this is that we don't actually know how tall the line is going to be. Your patch is trying to just use the line-height of the block, but of course that isn't necessarily how tall the line of text will end up being. In quirks mode it can be shorter than that, and if there are taller items on the line, it can be larger.
> 
> This has been a known issue for years... I just haven't come up with a great way to address it without having to "be wrong" and do another pass (which can itself be cyclic if you keep changing your mind).
> 
> I think using the line-height of the block is somewhat reasonable, but it's tricky. The two issues I can think of with using the line-height of the block as a guide are that in quirks mode the descent may go away, and then you have to consider -webkit-line-box-contain also, since if the block isn't being allowed to contribute to line-height, it definitely seems wrong to use it.
> 
> This is a really challenging bug to fix.
Thanks for your review, I agree with you. If putting text on the line will cause line height different with computed line height from the style, it sure has problem and it is really hard to figure our before putting text.  
But it seems only Firefox works well, may be they know exactly line height when they find next line break and calculate available line width or have a estimate work around.
If we pass the line object except line top to the calculation of the available line width, wouldn't that be better? May be the same if we haven't put text.

[Alexandru Chiculita]

(In reply to comment #22)
> (From update of attachment 99853 [details])

What about patching RenderBlock::LineBreaker::nextLineBreak to keep track of the maximum height that is not changing the left/right offsets. Every time it commits something to the LineWidth, it should also check that it doesn't pass beyond that logical height limit. If it does, it can calculate the left/right offset again and see if it can fit the line between the new limits or just bail to what was committed before that. If nothing was added on the line, it can just move the logical height and continue.

I think the most complex part is figuring out the logical height of the box/text to be committed, but considering that there's no check at all now, we can at least fix the text in this bug and continue to improve with other bugs.

[Dave Hyatt]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 99853 [details] [details])
> 
> What about patching RenderBlock::LineBreaker::nextLineBreak to keep track of the maximum height that is not changing the left/right offsets. Every time it commits something to the LineWidth, it should also check that it doesn't pass beyond that logical height limit. If it does, it can calculate the left/right offset again and see if it can fit the line between the new limits or just bail to what was committed before that. If nothing was added on the line, it can just move the logical height and continue.
> 
> I think the most complex part is figuring out the logical height of the box/text to be committed, but considering that there's no check at all now, we can at least fix the text in this bug and continue to improve with other bugs.

I'd just build up the entire line, determine its final placement, have it be completely built, and then if it turns out that it is incorrect, scrap it and try again (within reason, we want to avoid infinite yo-yo effects).

The idea of redoing the line is going to come up for text-alignment and exclusions as well, since I assume for text-align:center you'll have to experiment with putting the text on the line into the available segments in the center of the line, and you may end up having to do more than one pass to get it right.

[Shezan Baig]

*** Bug 78948 has been marked as a duplicate of this bug. ***