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.
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.
Created attachment 99634 [details] QT screenshot BBC news
Created attachment 99721 [details] test case
This seems to be QT-specific.
Created attachment 99725 [details] Patch
Why change RenderBlock.cpp when this only affects Qt?
(In reply to comment #6) > Why change RenderBlock.cpp when this only affects Qt? See attachment 99633 [details]. It is not Qt specific.
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
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
(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.
Created attachment 99832 [details] Patch
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
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
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
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.
Created attachment 99841 [details] mac results for new test and changed test result
Created attachment 99853 [details] Patch
Updated the change log and added mac results for the patch according to the comments of Nikolas.
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
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
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.
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.
(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.
(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.
(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.
*** Bug 78948 has been marked as a duplicate of this bug. ***