45274 – Breaking Float: floated block level element following inline element in floated container breaks to next line

RESOLVED FIXED 45274

Breaking Float: floated block level element following inline element in floated container breaks to next line

https://bugs.webkit.org/show_bug.cgi?id=45274

Summary Breaking Float: floated block level element following inline element in float...

Stijn de Witt

Reported 2010-09-06 13:17:15 PDT

When a block-level element such as a div is floated to the left and follows an inline element inside a parent container that is also foated to the left, the inner div breaks (wraps) to the next line even though this is not expected. In this sample HTML: <div class="outer"> <span>Breaking</span> <div class="inner">Float</div> </div> If this stylesheet is applied: .outer { float: left; } .inner { float: left; } ...the word 'Float' unexpectedly breaks to the next line in WebKit-based browsers such as Safari and Chrome, but stays on the same line as expected in other browsers such as internet Explorer, Firefox and Opera.

Attachments
Reproduction page (1.67 KB, text/html) 2010-09-06 13:23 PDT, Stijn de Witt	no flags	Details
Simple test case (385 bytes, text/html) 2011-04-12 23:02 PDT, ChangSeok Oh	no flags	Details
Updated version of test page, validates as HTML 4.01 strict (1.69 KB, text/html) 2011-04-13 09:44 PDT, Stijn de Witt	no flags	Details
Screenshot of IE9 results (89.46 KB, image/png) 2011-04-13 09:58 PDT, Stijn de Witt	no flags	Details
Proposed patch (9.38 KB, patch) 2011-04-18 23:46 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
updated patch using ahem font (440 bytes, text/html) 2011-04-19 00:29 PDT, ChangSeok Oh	no flags	Details
Updated patch to meet Levi's comment (9.74 KB, patch) 2011-04-20 00:26 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
merged patch with Joseph's test cases (101.50 KB, patch) 2011-04-28 23:06 PDT, ChangSeok Oh	hyatt: review- hyatt: commit-queue-	Details Formatted Diff Diff
updated patch (101.35 KB, patch) 2011-05-22 10:40 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
Revied patch to meet current version (101.37 KB, patch) 2011-06-20 09:43 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
Updated patch (101.38 KB, patch) 2011-10-12 00:06 PDT, ChangSeok Oh	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Updated patch (27.20 KB, patch) 2012-06-03 19:37 PDT, ChangSeok Oh	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (557.11 KB, application/zip) 2012-06-03 20:18 PDT, WebKit Review Bot	no flags	Details
Updated patch (27.77 KB, patch) 2012-07-05 23:59 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (28.37 KB, patch) 2012-07-06 08:13 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (37.64 KB, patch) 2012-07-10 07:21 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (39.13 KB, patch) 2013-02-08 12:59 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (39.12 KB, patch) 2013-02-09 19:42 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (39.12 KB, patch) 2013-02-11 23:46 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (39.01 KB, patch) 2013-04-16 11:23 PDT, Robert Hogan	no flags	Details Formatted Diff Diff
Patch (40.17 KB, patch) 2013-04-16 12:58 PDT, Robert Hogan	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Stijn de Witt

Comment 1 2010-09-06 13:23:35 PDT

Created attachment 66667 [details] Reproduction page Attached the reproduction page for this bug. This is the same page this bug also links to but because that's on my personal webspace I can't guarantee it will stay there forever. I might switch providers in the future etc.

Stijn de Witt

Comment 2 2011-02-14 15:18:15 PST

Still unconfirmed? It has now been over 5 months since I reported this issue. I think the scenario to reproduce is pretty simple and obvious? If there is more information I should add to have someone be able to confirm this issue, please let me know.

Stijn de Witt

Comment 3 2011-04-12 12:41:52 PDT

Wow it is a bit disappointing to see a bug reported not being looked at for months and months, even though Chrome and Safari have major releases in the mean time. Should I report this somewhere else?

ChangSeok Oh

Comment 4 2011-04-12 22:15:40 PDT

(In reply to comment #3) > Wow it is a bit disappointing to see a bug reported not being looked at for months and months, even though Chrome and Safari have major releases in the mean time. Should I report this somewhere else? I'm interested this issue. I'll have a look.

ChangSeok Oh

Comment 5 2011-04-12 23:02:53 PDT

Created attachment 89343 [details] Simple test case

ChangSeok Oh

Comment 6 2011-04-12 23:25:30 PDT

(In reply to comment #2) > Still unconfirmed? It has now been over 5 months since I reported this issue. I think the scenario to reproduce is pretty simple and obvious? If there is more information I should add to have someone be able to confirm this issue, please let me know. Well, I could reproduce this as you mentioned. In my case, Chrome, Safari, & IE8,9 show the tc in two lines. And the others(FF & Opera) show it in one line. I'm not convinced this is a real issue. Could you let me know where you faced this issue? I want to know the real site which is broken on webkit based browser. Or is there any standard spec you refer?

Stijn de Witt

Comment 7 2011-04-13 09:44:59 PDT

Created attachment 89392 [details] Updated version of test page, validates as HTML 4.01 strict My original test page contained a couple of small errors that made it not pass W3C validation, so I fixed those. New file attached. I also updates the version on my local web space and you can validate it here: http://validator.w3.org/check?uri=http%3A%2F%2Fmembers.chello.nl%2F~sgm.jansen%2FWebkitBreakingFloat%2F

Stijn de Witt

Comment 8 2011-04-13 09:58:07 PDT

Created attachment 89393 [details] Screenshot of IE9 results I would have to search for the exact wording in the standard that dictates how these floats should behave but I'm pretty confident that what WebKit is doing is violating the standard. I can only directly test this issue in IE9. There the words Breaking and Float render on one line as expected. See the screenshot I attached. However if I use the NetRenderer tool to check IE7 and 8 ( http://ipinfo.info/netrenderer/index.php ) I can confirm that indeed, the words render on two lines in IE7. IE8 renders them on one line as expected though. So to summarize: IE7 and before behave the same as WebKit: Words render on two lines. IE8, IE9, Firefox 3 and 4 and Opera (latest versions) render as I would expect: On one line. I can't point you to a site that demonstrates this issue in real life as I discovered it during development and changed it to something that does work... as I guess most developers will have done.

Martijn T.

Comment 9 2011-04-13 13:02:00 PDT

I can confirm this issue in the very latest Chrome, Safari and also in IE8 in compatibility mode. The 'normal' IE8 (without compatibility mode) displays just a single line here. @ChangSeok Oh: strange that you are seeing two lines on IE8 and IE9, perhaps you where using compatibility mode by accident? I've experienced this when I was creating a new website where this construction seemed logical. I then immediately submitted this bug to the Webkit bugzilla but without response so far: https://bugs.webkit.org/show_bug.cgi?id=57441 Since submitting the bug, I did several testcases to determine if surrounding objects where causing any problems, and came to this extract of my project: http://jsfiddle.net/gFevt/10/ Stijn then strongly reduced the code to demonstrate the problem: http://jsfiddle.net/gFevt/21/

Stijn de Witt

Comment 10 2011-04-13 14:20:35 PDT

I looked through the W3C specs (mostly CSS2's section of Float: http://www.w3.org/TR/CSS2/visuren.html#floats ) but that basically states that floated boxes: * Stay on the same line * Break to the next line if there is not enough space left for them to fit "A float is a box that is shifted to the left or right on the current line." "If there is not enough horizontal room for the float, it is shifted downward until either it fits or there are no more floats present." The wording there is pretty complicated, but I don't see anything explaining WebKit's behaviour, so given that, plus the fact that all other standards-compliant browsers (when running in standards mode) render them on one line I think it's save to say that WebKit is breaking the standard here. Also given that this issue has now been confirmed by two people already and reproduction scenarios are available it may be time to move this bug to status 'NEW'?

ChangSeok Oh

Comment 11 2011-04-18 23:33:27 PDT

I've spent some time for this and found two strange things which make me confirm an issue. First, if we remove all spaces between "Breaking" and "Float", they are placed on a same line. Second, the width of floating element "Float" is exactrly same with the right empty spaces of first line. With these symptoms, I think empty spaces make inner Floating element is placed at next line.

ChangSeok Oh

Comment 12 2011-04-18 23:46:14 PDT

Created attachment 90155 [details] Proposed patch This patch affects following Layout test cases. fast/multicol/float-truncation.html fast/multicol/vertical-lr/float-truncation.html fast/multicol/vertical-rl/float-truncation.html Because this patch makes inner floating element is placed on a same line with outer floating element. I think expected results of them are needed to change, but I'm not sure. This issue may be related with bug46421.

ChangSeok Oh

Comment 13 2011-04-19 00:29:06 PDT

Created attachment 90159 [details] updated patch using ahem font

Levi Weintraub

Comment 14 2011-04-19 12:04:09 PDT

View in context: https://bugs.webkit.org/attachment.cgi?id=90155&action=review I'm having trouble determining how exactly uncommitedWidth differs from additionalWidth. It seems to me that additionalWidth is really more like uncommittedTextWidth (at least as implemented in this patch), and while I have my theories about why this is the right fix, I'd like a better description as to why in the changelog. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1779 > + if (floatsFitOnLine && width.fitsOnLine(logicalWidthForFloat(f) - width.additionalWidth())) { Could we instead have a different version of fitsOnLine that wraps this up for us? Perhaps fitsOnLineOverridingText? Or floatFitsOnLine? This change doesn't self-document well. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2129 > + if (!ignoringSpaces && additionalTmpW) At least the !ignoringSpaces part is redundant since additionalTmpW is only nonzero when !ignoringSpaces.

ChangSeok Oh

Comment 15 2011-04-20 00:23:45 PDT

(In reply to comment #14) > I'm having trouble determining how exactly uncommitedWidth differs from additionalWidth. It seems to me that additionalWidth is really more like uncommittedTextWidth (at least as implemented in this patch), and while I have my theories about why this is the right fix, I'd like a better description as to why in the changelog. If there are more than 2 spaces between "Float" and "Breaking" in TC, uncommittedWidth would be different from additionalWidth. uncommittedWidth and additionalWidth might be same for first empty space, but uncommittedWidth is commited and we set ignoringSpaces=true from second spaces. In this situation, uncommittedWidth would be zero, when checking float fits on line. I've tried to update ChangLog in detail. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1779 > > + if (floatsFitOnLine && width.fitsOnLine(logicalWidthForFloat(f) - width.additionalWidth())) { > > Could we instead have a different version of fitsOnLine that wraps this up for us? Perhaps fitsOnLineOverridingText? Or floatFitsOnLine? This change doesn't self-document well. Yeh. floatFitsOnLine seems proper to me. :) > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2129 > > + if (!ignoringSpaces && additionalTmpW) > > At least the !ignoringSpaces part is redundant since additionalTmpW is only nonzero when !ignoringSpaces. I was confused. :p My intention is currentCharacterIsSpace. Thanks for your comment! :)

ChangSeok Oh

Comment 16 2011-04-20 00:26:46 PDT

Created attachment 90320 [details] Updated patch to meet Levi's comment

ChangSeok Oh

Comment 17 2011-04-22 08:51:43 PDT

This issue seems to be solved by a patch for bug58806. :|

Joseph Pecoraro

Comment 18 2011-04-22 10:16:27 PDT

(In reply to comment #17) > This issue seems to be solved by a patch for bug58806. :| I'm sorry I didn't find this bug first. This patch is slightly cleaner, and might be the one to go with. Does it pass all of the test cases I had on mine? My only concern with this patch is that it doesn't check if there is a trailing space when it tries to position the float ignoring additional space. I wonder if this could overlap a character or if it works out.

ChangSeok Oh

Comment 19 2011-04-22 20:27:52 PDT

(In reply to comment #18) > (In reply to comment #17) > > This issue seems to be solved by a patch for bug58806. :| > > Does it pass all of the test cases I had on mine? I tested that my patch passed your test cases included in your patch. Yes. it passed all. Of course, it passed all layout tests except three things I mentioned above. :) > My only concern with this patch is that it doesn't > check if there is a trailing space when it tries to position the float > ignoring additional space. In my opinion, no problem without additional checking if there is a trailing space. Because it already verified with following line, > if (currentCharacterIsSpace && additionalTmpW) > width.setAdditionalWidth(additionalTmpW); m_additionalWidth is initialized with 0. And we set only non-zero value when current Character is a "space". This means that if m_additionalWidth isn't zero, there is a trailing space. Otherwise m_additialalWidth doesn't affect anything. > I wonder if this could overlap a character or if it works out. I've tested some kinds of situation like, multiple floats, no space between floats, multiple spaces between float, multiple text strings between float, multiple spaces in float and so on... But I've found nothing special. Thank you for your kind comment, and concern. :)

ChangSeok Oh

Comment 20 2011-04-28 23:06:55 PDT

Created attachment 91643 [details] merged patch with Joseph's test cases This patch is merged with Joseph's test cases. See https://bugs.webkit.org/show_bug.cgi?id=58806.

Joseph Pecoraro

Comment 21 2011-04-29 09:45:32 PDT

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

Dave Hyatt

Comment 22 2011-05-03 15:01:34 PDT

Comment on attachment 91643 [details] merged patch with Joseph's test cases View in context: https://bugs.webkit.org/attachment.cgi?id=91643&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1568 > + bool floatFitsOnLine(float extra) const { return currentWidth() - additionalWidth() + extra <= m_availableWidth; } Is it not sufficient to just ignore the uncommitted width here? It seems like you're basically doing committed + uncommitted - uncommitted + extra, which works out to just being committed + extra. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2119 > + if (currentCharacterIsSpace && additionalTmpW) > + width.setAdditionalWidth(additionalTmpW); It seems wrong not to include inlineLogicalWidth here. You might want to make a test for this case. You should be able to just add it to additionalTmpW I would think.

Dave Hyatt

Comment 23 2011-05-04 09:04:31 PDT

Also, checking currentCharacterIsSpace seems suspicious to me without also checking the whitespace mode.

Ryosuke Niwa

Comment 24 2011-05-04 09:34:39 PDT

Joseph Pecoraro

Comment 25 2011-05-12 16:23:33 PDT

ChangSeok, are you going to address the comments? Especially with the recent refactoring going on within this file, getting a patch to land would be great, otherwise it will go stale quickly. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2119 > > + if (currentCharacterIsSpace && additionalTmpW) > > + width.setAdditionalWidth(additionalTmpW); > > It seems wrong not to include inlineLogicalWidth here. You might > want to make a test for this case. You should be able to just add > it to additionalTmpW I would think. > Also, checking currentCharacterIsSpace seems suspicious to me > without also checking the whitespace mode. Hyatt, did you have example tests in mind for these?

ChangSeok Oh

Comment 26 2011-05-13 07:34:52 PDT

(In reply to comment #25) > ChangSeok, are you going to address the comments? > > Especially with the recent refactoring going on within this file, getting > a patch to land would be great, otherwise it will go stale quickly. > > > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2119 > > > + if (currentCharacterIsSpace && additionalTmpW) > > > + width.setAdditionalWidth(additionalTmpW); > > > > It seems wrong not to include inlineLogicalWidth here. You might > > want to make a test for this case. You should be able to just add > > it to additionalTmpW I would think. > > > Also, checking currentCharacterIsSpace seems suspicious to me > > without also checking the whitespace mode. > > Hyatt, did you have example tests in mind for these? Sorry for late response. I'm a little busy recently. I'll have a look again and give you a proper response as soon as possible. Thank you :)

Dave Hyatt

Comment 27 2011-05-13 10:17:04 PDT

Comment on attachment 91643 [details] merged patch with Joseph's test cases Minusing until comments addressed.

ChangSeok Oh

Comment 28 2011-05-22 10:37:26 PDT

Comment on attachment 91643 [details] merged patch with Joseph's test cases View in context: https://bugs.webkit.org/attachment.cgi?id=91643&action=review Thank for review. And I so sorry for late response. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1568 >> + bool floatFitsOnLine(float extra) const { return currentWidth() - additionalWidth() + extra <= m_availableWidth; } > > Is it not sufficient to just ignore the uncommitted width here? It seems like you're basically doing committed + uncommitted - uncommitted + extra, which works out to just being committed + extra. No. Actually it could be a little different if there are two more white spaces between blocks. When we encounter first white space, (let's suppose committed = 70, uncommitted = 5.) the final result is 70 + extra (70 + 5 - 5 + extra). But when we encounter second white space, the final result will be 75 + extra. Because ucommitted '5' is submitted & included in committed. (75 + 0 - 0 + extra) After this we can't get what was last additionalWidth(uncommitted), so that I want that m_additionalWidth holds this value. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1569 >> float currentWidth() const { return m_committedWidth + m_uncommittedWidth; } > > Nit: I don't see why additional width should be associated with floats. Becuase extra space(additional width) cause that float doesn't fit to line. Even though Float width is exactly same with remained line width. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2119 >> + width.setAdditionalWidth(additionalTmpW); > > It seems wrong not to include inlineLogicalWidth here. You might want to make a test for this case. You should be able to just add it to additionalTmpW I would think. Done. But I haven't found problem case. inlineLogicalWidth is almost 0.

ChangSeok Oh

Comment 29 2011-05-22 10:40:27 PDT

Created attachment 94349 [details] updated patch Addressed dave hyatt's comment and revise patch to latest trunk.

ChangSeok Oh

Comment 30 2011-05-22 10:46:49 PDT

(In reply to comment #23) > Also, checking currentCharacterIsSpace seems suspicious to me without also checking the whitespace mode. Do you mean whitespace mode is to check collapseWhiteSpace? I added it, but it didn't seem to affect anything to me.

ChangSeok Oh

Comment 31 2011-06-12 22:21:19 PDT

Review this please?

Ryosuke Niwa

Comment 32 2011-06-12 22:49:17 PDT

Comment on attachment 94349 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=94349&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1677 > + float m_additionalWidth; What does "additional" width mean here? It should be widths of something, right? What are they? I can't tell whether this patch is correct or not but adding this member variable scares me to no end because I can't tell what it is.

ChangSeok Oh

Comment 33 2011-06-14 02:53:45 PDT

(In reply to comment #32) > (From update of attachment 94349 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94349&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1677 > > + float m_additionalWidth; > > What does "additional" width mean here? It should be widths of something, right? What are they? I can't tell whether this patch is correct or not but adding this member variable scares me to no end because I can't tell what it is. Actually, "additional" means "extra". As you know, if there are 2 more spaces between elements, there are ignored except one. The width of float element which has white space at next includes a width of this "one" white space. So the width of float is bigger than actual one. (This is why fitsOnLine failed.) m_additionalWidth saves the width of an white space, and is used to subtract later when float is checked whether it fits on line.

ChangSeok Oh

Comment 34 2011-06-20 09:43:01 PDT

Created attachment 97813 [details] Revied patch to meet current version

Martijn T.

Comment 35 2011-10-09 02:15:09 PDT

Hey guys, what the status on this? It's been a while since the last update.

Ryosuke Niwa

Comment 36 2011-10-09 11:42:48 PDT

(In reply to comment #33) > Actually, "additional" means "extra". It doesn't explain anything still. additional and extra are synonymous. > As you know, if there are 2 more spaces between elements, there are ignored except one. The width of float element which has white space at next includes a width of this "one" white space. So the width of float is bigger than actual one. > (This is why fitsOnLine failed.) I don't follow. > m_additionalWidth saves the width of an white space, and is used to subtract later when float is checked whether it fits on line. Then it should be named as m_whitespaceWidth. There's no reason we should be using vague adjectives like additional and extra.

ChangSeok Oh

Comment 37 2011-10-12 00:06:47 PDT

Created attachment 110647 [details] Updated patch

ChangSeok Oh

Comment 38 2011-10-12 00:12:14 PDT

(In reply to comment #36) > (In reply to comment #33) > > m_additionalWidth saves the width of an white space, and is used to subtract later when float is checked whether it fits on line. > > Then it should be named as m_whitespaceWidth. There's no reason we should be using vague adjectives like additional and extra. I've updated the patch to reflect your opinion.

WebKit Review Bot

Comment 39 2011-10-12 03:00:14 PDT

Comment on attachment 110647 [details] Updated patch Attachment 110647 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10031733 New failing tests: fast/block/float/overhanging-float-remove-from-fixed-position-block2.html fast/block/float/overhanging-float-remove-from-fixed-position-block.html

Levi Weintraub

Comment 40 2011-11-02 21:46:07 PDT

Can we try this patch again? I'd love to see this fixed :)

ChangSeok Oh

Comment 41 2011-11-02 23:41:04 PDT

(In reply to comment #40) > Can we try this patch again? I'd love to see this fixed :) Sure. Let me revise it. :)

Robert Hogan

Comment 42 2012-04-28 03:46:18 PDT

(In reply to comment #41) > (In reply to comment #40) > > Can we try this patch again? I'd love to see this fixed :) > > Sure. Let me revise it. :) The problem as I understand is that in: <div class="container"> <div class="first"></div> <div class="second"></div> </div> the spaces before the second div add some width to the line - the width of a single white space. Unless there's actual text we don't want this to happen do we? So isn't the correct solution to back out that width when no text is encountered rather than account for it when trying to fit subsequent floats on the line? Or is there a reason we need to hold on to it?

Robert Hogan

Comment 43 2012-05-21 10:34:11 PDT

ChangSeok, dhyatt on IRC was OK with this approach - so you willing to fix it up for final review?

ChangSeok Oh

Comment 44 2012-05-23 02:42:31 PDT

(In reply to comment #43) > ChangSeok, > > dhyatt on IRC was OK with this approach - so you willing to fix it up for final review? Sure. Thanks :)

ChangSeok Oh

Comment 45 2012-06-03 19:37:52 PDT

Created attachment 145505 [details] Updated patch

WebKit Review Bot

Comment 46 2012-06-03 20:18:26 PDT

Comment on attachment 145505 [details] Updated patch Attachment 145505 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12890288 New failing tests: fast/block/float/overhanging-float-remove-from-fixed-position-block2.html fast/block/float/overhanging-float-remove-from-fixed-position-block.html

WebKit Review Bot

Comment 47 2012-06-03 20:18:31 PDT

Created attachment 145509 [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: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Robert Hogan

Comment 48 2012-06-26 12:29:57 PDT

Comment on attachment 145505 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=145505&action=review > LayoutTests/ChangeLog:27 > + * fast/inline-block/multiple-floats-with-whitespace.html: Added. > + This also fixes floats-001.htm and floats-102.htm from the CSS test suite. You'll find them at http://test.csswg.org/suites/css2.1/nightly-unstable/html4/

Robert Hogan

Comment 49 2012-06-26 12:33:39 PDT

(In reply to comment #46) > (From update of attachment 145505 [details]) > Attachment 145505 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12890288 > > New failing tests: > fast/block/float/overhanging-float-remove-from-fixed-position-block2.html > fast/block/float/overhanging-float-remove-from-fixed-position-block.html These look like they could be genuine regressions. Can you see what's going on with them locally?

ChangSeok Oh

Comment 50 2012-06-26 22:13:53 PDT

(In reply to comment #49) > (In reply to comment #46) > > (From update of attachment 145505 [details] [details]) > > Attachment 145505 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/12890288 > > > > New failing tests: > > fast/block/float/overhanging-float-remove-from-fixed-position-block2.html > > fast/block/float/overhanging-float-remove-from-fixed-position-block.html > > These look like they could be genuine regressions. Can you see what's going on with them locally? Yeh. I'm digging it. I think I need to understand more deeply to meet them..

ChangSeok Oh

Comment 51 2012-07-05 23:59:03 PDT

Created attachment 151024 [details] Updated patch

WebKit Review Bot

Comment 52 2012-07-06 00:03:29 PDT

Attachment 151024 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderBlockLineLayout.cpp:2435: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

ChangSeok Oh

Comment 53 2012-07-06 08:13:31 PDT

Created attachment 151085 [details] Patch

Robert Hogan

Comment 54 2012-07-06 12:13:04 PDT

Comment on attachment 151085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151085&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2571 > + if (collapseWhiteSpace && currentCharacterIsSpace && additionalTmpW) > + width.setWhitespaceWidth(additionalTmpW + inlineLogicalW); Why don't you care about previousCharacterIsSpace here? I also think what's happening here is sufficiently non-obvious to deserve a comment. Nit: I think the 'W' thing is a bad idea - maybe just call it inlineLogicalWidth. > LayoutTests/ChangeLog:26 > + * fast/inline-block/multiple-floats-with-whitespace.html: Added. Would you mind adding floats-001.htm and floats-102.htm from the CSS test suite - this patch fixes them. See http://test.csswg.org/suites/css2.1/nightly-unstable/html4/

Ryosuke Niwa

Comment 55 2012-07-06 12:21:52 PDT

Comment on attachment 151085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151085&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:100 > + void setWhitespaceWidth(float w) { m_whitespaceWidth = w; } Please don't abbreviate width as w. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2567 > + float inlineLogicalW = inlineLogicalWidth(current.m_obj, !appliedStartWidth, includeEndWidth); Ditto.

ChangSeok Oh

Comment 56 2012-07-10 07:21:31 PDT

Created attachment 151457 [details] Patch

ChangSeok Oh

Comment 57 2012-07-10 07:30:55 PDT

Comment on attachment 151085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151085&action=review Thank you for your review! >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:100 >> + void setWhitespaceWidth(float w) { m_whitespaceWidth = w; } > > Please don't abbreviate width as w. O.K. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2567 >> + float inlineLogicalW = inlineLogicalWidth(current.m_obj, !appliedStartWidth, includeEndWidth); > > Ditto. Done. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2571 >> + width.setWhitespaceWidth(additionalTmpW + inlineLogicalW); > > Why don't you care about previousCharacterIsSpace here? > > I also think what's happening here is sufficiently non-obvious to deserve a comment. > > Nit: I think the 'W' thing is a bad idea - maybe just call it inlineLogicalWidth. previsousCharacterIsSpace is out of scope here, and it seems no problem though we don't take care of it here. Done. inlineLogicalW -> inlineLogicalTempWidth >> LayoutTests/ChangeLog:26 >> + * fast/inline-block/multiple-floats-with-whitespace.html: Added. > > Would you mind adding floats-001.htm and floats-102.htm from the CSS test suite - this patch fixes them. See http://test.csswg.org/suites/css2.1/nightly-unstable/html4/ Done.

ChangSeok Oh

Comment 58 2012-07-18 09:57:32 PDT

@Ryosuke Could you review this again?

Ryosuke Niwa

Comment 59 2012-07-18 11:29:41 PDT

(In reply to comment #58) > @Ryosuke Could you review this again? I don't think I'm qualified to review this patch. I was just pointing out some nits.

Stijn de Witt

Comment 60 2012-07-19 07:18:59 PDT

Does it pass all tests? Does it fix the issue? Land it! :)

Robert Hogan

Comment 61 2012-07-19 11:53:16 PDT

(In reply to comment #58) > @Ryosuke Could you review this again? You will need to get on IRC and ask dhyatt to review this one.

Dave Hyatt

Comment 62 2012-09-04 14:16:40 PDT

What's the reasoning behind adding exclamation points to the multicol tests?

Robert Hogan

Comment 63 2012-09-04 14:27:48 PDT

Comment on attachment 151457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151457&action=review > LayoutTests/ChangeLog:39 > + > + Update the following tests so that wrapping happens as it > + did before. With this change the float:left div progressed > + and could fit on an earlier line when we position after > + collapsing whitespace. This moved the float a line earlier > + and changed the results of the test. By adding a character > + to the line, the float won't fit and goes back on to the > + line that it was on before this change. dhyatt: See above!

Robert Hogan

Comment 64 2012-12-15 04:01:30 PST

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

Levi Weintraub

Comment 65 2012-12-17 15:08:11 PST

Comment on attachment 151457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151457&action=review > Source/WebCore/ChangeLog:8 > + Fix position issue of floating div element in floating div element. Nit: "div" doesn't actually add any information here. This would apply to any tag :) > Source/WebCore/ChangeLog:10 > + Inner floating div element has placed at next line when outer floating div element has text, > + even though previous line has spaces enough to fit it. I can't follow this sentence.

Dave Hyatt

Comment 66 2013-02-07 12:09:06 PST

Comment on attachment 151457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151457&action=review r- > Source/WebCore/rendering/RenderBlockLineLayout.cpp:81 > bool fitsOnLine(float extra) const { return currentWidth() + extra <= m_availableWidth; } > + bool floatFitsOnLine(float extra) const { return currentWidth() - whitespaceWidth() + extra <= m_availableWidth; } I think I would prefer this: bool fitsOnLine(float extra, LineFittingCheck includeWhitespace = IncludeTrailingWhitespace) const { return currentWidth() - (includeWhitespace ? trailingWhitespaceWidth() : 0) + extra <= m_availableWidth; } > Source/WebCore/rendering/RenderBlockLineLayout.cpp:88 > + float whitespaceWidth() const { return m_whitespaceWidth; } float trailingWhitespaceWidth() const { return m_trailingWhitespaceWidth; } > Source/WebCore/rendering/RenderBlockLineLayout.cpp:100 > + void setWhitespaceWidth(float width) { m_whitespaceWidth = width; } void setTrailingWhitespaceWidth(float width) { m_trailingWhitespaceWidth = width; } > Source/WebCore/rendering/RenderBlockLineLayout.cpp:113 > + float m_whitespaceWidth; float m_trailingWhitespaceWidth; > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2248 > + if (floatsFitOnLine && width.floatFitsOnLine(m_block->logicalWidthForFloat(f))) { if (floatsFitOnLine && width.floatFitsOnLine(m_block->logicalWidthForFloat(f), ExcludeTrailingWhitespace)) { > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2423 > + width.setWhitespaceWidth(additionalTempWidth); width.setTrailingWhitespaceWidth(additionalTempWidth);

Dave Hyatt

Comment 67 2013-02-07 12:09:59 PST

Oops, my includeTrailingWhitespace check is backwards... anyway you get the idea.

ChangSeok Oh

Comment 68 2013-02-08 12:59:31 PST

Created attachment 187351 [details] Patch

ChangSeok Oh

Comment 69 2013-02-08 13:06:31 PST

Comment on attachment 151457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151457&action=review Thank you for your kind review! I hope this bug would be fixed in this iteration :) >> Source/WebCore/ChangeLog:8 >> + Fix position issue of floating div element in floating div element. > > Nit: "div" doesn't actually add any information here. This would apply to any tag :) Removed 'div' >> Source/WebCore/ChangeLog:10 >> + even though previous line has spaces enough to fit it. > > I can't follow this sentence. I believe the simple test attached in this bug will help you. :) >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:81 >> + bool floatFitsOnLine(float extra) const { return currentWidth() - whitespaceWidth() + extra <= m_availableWidth; } > > I think I would prefer this: > > bool fitsOnLine(float extra, LineFittingCheck includeWhitespace = IncludeTrailingWhitespace) const { return currentWidth() - (includeWhitespace ? trailingWhitespaceWidth() : 0) + extra <= m_availableWidth; } Done. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:88 >> + float whitespaceWidth() const { return m_whitespaceWidth; } > > float trailingWhitespaceWidth() const { return m_trailingWhitespaceWidth; } Done. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:100 >> + void setWhitespaceWidth(float width) { m_whitespaceWidth = width; } > > void setTrailingWhitespaceWidth(float width) { m_trailingWhitespaceWidth = width; } Done. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:113 >> + float m_whitespaceWidth; > > float m_trailingWhitespaceWidth; Done. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2248 >> + if (floatsFitOnLine && width.floatFitsOnLine(m_block->logicalWidthForFloat(f))) { > > if (floatsFitOnLine && width.floatFitsOnLine(m_block->logicalWidthForFloat(f), ExcludeTrailingWhitespace)) { Done. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2423 >> + width.setWhitespaceWidth(additionalTempWidth); > > width.setTrailingWhitespaceWidth(additionalTempWidth); Done.

WebKit Review Bot

Comment 70 2013-02-09 11:36:54 PST

Comment on attachment 187351 [details] Patch Attachment 187351 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16474454 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

ChangSeok Oh

Comment 71 2013-02-09 19:42:54 PST

Created attachment 187455 [details] Patch

WebKit Review Bot

Comment 72 2013-02-09 21:27:20 PST

Comment on attachment 187455 [details] Patch Attachment 187455 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16466712 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

Build Bot

Comment 73 2013-02-09 22:03:09 PST

Comment on attachment 187455 [details] Patch Attachment 187455 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16467750 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

Build Bot

Comment 74 2013-02-09 22:57:56 PST

Comment on attachment 187455 [details] Patch Attachment 187455 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16468742 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

ChangSeok Oh

Comment 75 2013-02-11 23:46:47 PST

Created attachment 187783 [details] Patch

WebKit Review Bot

Comment 76 2013-02-12 00:41:19 PST

Comment on attachment 187783 [details] Patch Attachment 187783 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16483025 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

Build Bot

Comment 77 2013-02-12 10:42:02 PST

Comment on attachment 187783 [details] Patch Attachment 187783 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16519340 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

Build Bot

Comment 78 2013-02-12 22:46:43 PST

Comment on attachment 187783 [details] Patch Attachment 187783 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16440648 New failing tests: css2.1/20110323/floats-001.html css2.1/20110323/floats-102.html

Stijn de Witt

Comment 79 2013-04-08 12:09:06 PDT

Seems this one is hard to fix. Not for a lack of work on it though. Many thanks to ChangSeok Oh for his persistence. Still hoping this fundamental rendering issue in a basic HTML construct will continue to receive the attention it deserves and be fixed soon.

Dave Hyatt

Comment 80 2013-04-16 10:12:58 PDT

Just need to figure out the failing tests. The patch seems fine to me now, but nobody has explained the failing tests or rebaselined them.

Robert Hogan

Comment 81 2013-04-16 11:23:39 PDT

Created attachment 198351 [details] Patch

Robert Hogan

Comment 82 2013-04-16 12:58:32 PDT

Created attachment 198391 [details] Patch

Dave Hyatt

Comment 83 2013-04-17 10:57:44 PDT

Comment on attachment 198391 [details] Patch r=me

WebKit Commit Bot

Comment 84 2013-04-17 11:47:39 PDT

Comment on attachment 198391 [details] Patch Clearing flags on attachment: 198391 Committed r148622: <http://trac.webkit.org/changeset/148622>

WebKit Commit Bot

Comment 85 2013-04-17 11:47:46 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Major

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component CSS

Assignee

Robert Hogan

Reported

2010-09-06 13:17 PDT

Modified

2013-04-17 11:47 PDT History

CC List

19 users Show

URL

http://members.chello.nl/~sgm.jansen/WebkitBreakingFloat/

Keywords

Duplicates (2)

18090 58806 View as bug list

Depends on

Blocks

11946 37245 84876

Dependencies

tree graph