Bug 45274

Summary: Breaking Float: floated block level element following inline element in floated container breaks to next line
Product: WebKit Reporter: Stijn de Witt <stijndewitt>
Component: CSSAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Major CC: buildbot, commit-queue, ddkilzer, dglazkov, eric, esprehn+autocc, hyatt, jasneet, joepeck, joone, kevin.cs.oh, leviw, mitz, ojan.autocc, rniwa, robert, simon.fraser, webkit_bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://members.chello.nl/~sgm.jansen/WebkitBreakingFloat/
Bug Depends on:    
Bug Blocks: 11946, 37245, 84876    
Attachments:
Description Flags
Reproduction page
none
Simple test case
none
Updated version of test page, validates as HTML 4.01 strict
none
Screenshot of IE9 results
none
Proposed patch
none
updated patch using ahem font
none
Updated patch to meet Levi's comment
none
merged patch with Joseph's test cases
hyatt: review-, hyatt: commit-queue-
updated patch
none
Revied patch to meet current version
none
Updated patch
webkit.review.bot: commit-queue-
Updated patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
Updated patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Stijn de Witt 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.
Comment 1 Stijn de Witt 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.
Comment 2 Stijn de Witt 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.
Comment 3 Stijn de Witt 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?
Comment 4 ChangSeok Oh 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.
Comment 5 ChangSeok Oh 2011-04-12 23:02:53 PDT
Created attachment 89343 [details]
Simple test case
Comment 6 ChangSeok Oh 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?
Comment 7 Stijn de Witt 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
Comment 8 Stijn de Witt 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.
Comment 9 Martijn T. 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/
Comment 10 Stijn de Witt 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'?
Comment 11 ChangSeok Oh 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.
Comment 12 ChangSeok Oh 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.
Comment 13 ChangSeok Oh 2011-04-19 00:29:06 PDT
Created attachment 90159 [details]
updated patch using ahem font
Comment 14 Levi Weintraub 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.
Comment 15 ChangSeok Oh 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! :)
Comment 16 ChangSeok Oh 2011-04-20 00:26:46 PDT
Created attachment 90320 [details]
Updated patch to meet Levi's comment
Comment 17 ChangSeok Oh 2011-04-22 08:51:43 PDT
This issue seems to be solved by a patch for bug58806. :|
Comment 18 Joseph Pecoraro 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.
Comment 19 ChangSeok Oh 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. :)
Comment 20 ChangSeok Oh 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.
Comment 21 Joseph Pecoraro 2011-04-29 09:45:32 PDT
*** Bug 58806 has been marked as a duplicate of this bug. ***
Comment 22 Dave Hyatt 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.
Comment 23 Dave Hyatt 2011-05-04 09:04:31 PDT
Also, checking currentCharacterIsSpace seems suspicious to me without also checking the whitespace mode.
Comment 24 Ryosuke Niwa 2011-05-04 09:34:39 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: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.
Comment 25 Joseph Pecoraro 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?
Comment 26 ChangSeok Oh 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 :)
Comment 27 Dave Hyatt 2011-05-13 10:17:04 PDT
Comment on attachment 91643 [details]
merged patch with Joseph's test cases

Minusing until comments addressed.
Comment 28 ChangSeok Oh 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.
Comment 29 ChangSeok Oh 2011-05-22 10:40:27 PDT
Created attachment 94349 [details]
updated patch

Addressed dave hyatt's comment and revise patch to latest trunk.
Comment 30 ChangSeok Oh 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.
Comment 31 ChangSeok Oh 2011-06-12 22:21:19 PDT
Review this please?
Comment 32 Ryosuke Niwa 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.
Comment 33 ChangSeok Oh 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.
Comment 34 ChangSeok Oh 2011-06-20 09:43:01 PDT
Created attachment 97813 [details]
Revied patch to meet current version
Comment 35 Martijn T. 2011-10-09 02:15:09 PDT
Hey guys, what the status on this? It's been a while since the last update.
Comment 36 Ryosuke Niwa 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.
Comment 37 ChangSeok Oh 2011-10-12 00:06:47 PDT
Created attachment 110647 [details]
Updated patch
Comment 38 ChangSeok Oh 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.
Comment 39 WebKit Review Bot 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
Comment 40 Levi Weintraub 2011-11-02 21:46:07 PDT
Can we try this patch again? I'd love to see this fixed :)
Comment 41 ChangSeok Oh 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. :)
Comment 42 Robert Hogan 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?
Comment 43 Robert Hogan 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?
Comment 44 ChangSeok Oh 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 :)
Comment 45 ChangSeok Oh 2012-06-03 19:37:52 PDT
Created attachment 145505 [details]
Updated patch
Comment 46 WebKit Review Bot 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
Comment 47 WebKit Review Bot 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
Comment 48 Robert Hogan 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/
Comment 49 Robert Hogan 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?
Comment 50 ChangSeok Oh 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..
Comment 51 ChangSeok Oh 2012-07-05 23:59:03 PDT
Created attachment 151024 [details]
Updated patch
Comment 52 WebKit Review Bot 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.
Comment 53 ChangSeok Oh 2012-07-06 08:13:31 PDT
Created attachment 151085 [details]
Patch
Comment 54 Robert Hogan 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/
Comment 55 Ryosuke Niwa 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.
Comment 56 ChangSeok Oh 2012-07-10 07:21:31 PDT
Created attachment 151457 [details]
Patch
Comment 57 ChangSeok Oh 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.
Comment 58 ChangSeok Oh 2012-07-18 09:57:32 PDT
@Ryosuke Could you review this again?
Comment 59 Ryosuke Niwa 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.
Comment 60 Stijn de Witt 2012-07-19 07:18:59 PDT
Does it pass all tests?
Does it fix the issue?
Land it!   :)
Comment 61 Robert Hogan 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.
Comment 62 Dave Hyatt 2012-09-04 14:16:40 PDT
What's the reasoning behind adding exclamation points to the multicol tests?
Comment 63 Robert Hogan 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!
Comment 64 Robert Hogan 2012-12-15 04:01:30 PST
*** Bug 18090 has been marked as a duplicate of this bug. ***
Comment 65 Levi Weintraub 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.
Comment 66 Dave Hyatt 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);
Comment 67 Dave Hyatt 2013-02-07 12:09:59 PST
Oops, my includeTrailingWhitespace check is backwards... anyway you get the idea.
Comment 68 ChangSeok Oh 2013-02-08 12:59:31 PST
Created attachment 187351 [details]
Patch
Comment 69 ChangSeok Oh 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.
Comment 70 WebKit Review Bot 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
Comment 71 ChangSeok Oh 2013-02-09 19:42:54 PST
Created attachment 187455 [details]
Patch
Comment 72 WebKit Review Bot 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
Comment 73 Build Bot 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
Comment 74 Build Bot 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
Comment 75 ChangSeok Oh 2013-02-11 23:46:47 PST
Created attachment 187783 [details]
Patch
Comment 76 WebKit Review Bot 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
Comment 77 Build Bot 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
Comment 78 Build Bot 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
Comment 79 Stijn de Witt 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.
Comment 80 Dave Hyatt 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.
Comment 81 Robert Hogan 2013-04-16 11:23:39 PDT
Created attachment 198351 [details]
Patch
Comment 82 Robert Hogan 2013-04-16 12:58:32 PDT
Created attachment 198391 [details]
Patch
Comment 83 Dave Hyatt 2013-04-17 10:57:44 PDT
Comment on attachment 198391 [details]
Patch

r=me
Comment 84 WebKit Commit Bot 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>
Comment 85 WebKit Commit Bot 2013-04-17 11:47:46 PDT
All reviewed patches have been landed.  Closing bug.