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

• Component: CSS
• Status: RESOLVED FIXED

[Stijn de Witt]

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.

[Stijn de Witt]

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]

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]

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]

(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]

Created attachment 89343 [details]
Simple test case

[ChangSeok Oh]

(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]

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]

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.]

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]

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]

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]

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]

Created attachment 90159 [details]
updated patch using ahem font

[Levi Weintraub]

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]

(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]

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

[ChangSeok Oh]

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

[Joseph Pecoraro]

(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]

(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]

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]

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

[Dave Hyatt]

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]

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

[Ryosuke Niwa]

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:1569
> +    bool floatFitsOnLine(float extra) const { return currentWidth() - additionalWidth() + extra <= m_availableWidth; }

Nit: I don't see why additional width should be associated with floats.

[Joseph Pecoraro]

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]

(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 on attachment 91643 [details]
merged patch with Joseph's test cases

Minusing until comments addressed.

[ChangSeok Oh]

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]

Created attachment 94349 [details]
updated patch

Addressed dave hyatt's comment and revise patch to latest trunk.

[ChangSeok Oh]

(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]

Review this please?

[Ryosuke Niwa]

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]

(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]

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

[Martijn T.]

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

[Ryosuke Niwa]

(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]

Created attachment 110647 [details]
Updated patch

[ChangSeok Oh]

(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 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]

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

[ChangSeok Oh]

(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]

(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]

ChangSeok,

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

[ChangSeok Oh]

(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]

Created attachment 145505 [details]
Updated patch

[WebKit Review Bot]

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]

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 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]

(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]

(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]

Created attachment 151024 [details]
Updated patch

[WebKit Review Bot]

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]

Created attachment 151085 [details]
Patch

[Robert Hogan]

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 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]

Created attachment 151457 [details]
Patch

[ChangSeok Oh]

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]

@Ryosuke Could you review this again?

[Ryosuke Niwa]

(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]

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

[Robert Hogan]

(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]

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

[Robert Hogan]

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]

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

[Levi Weintraub]

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 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]

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

[ChangSeok Oh]

Created attachment 187351 [details]
Patch

[ChangSeok Oh]

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 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]

Created attachment 187455 [details]
Patch

[WebKit Review Bot]

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 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 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]

Created attachment 187783 [details]
Patch

[WebKit Review Bot]

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 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 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]

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]

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]

Created attachment 198351 [details]
Patch

[Robert Hogan]

Created attachment 198391 [details]
Patch

[Dave Hyatt]

Comment on attachment 198391 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 198391 [details]
Patch

Clearing flags on attachment: 198391

Committed r148622: <http://trac.webkit.org/changeset/148622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.