Created attachment 97998 [details] float wrap bug Inline ul and li elements wrap inconsistently in floated divs. Depending on the content, lines are wrapping incorrectly. See the following test cases to reproduce: http://jsbin.com/ixuka6/5/ (also attached)
Created attachment 164760 [details] Patch
Comment on attachment 164760 [details] Patch r=me
Comment on attachment 164760 [details] Patch Was this a recent regression? I assume our new behavior now properly matches FF and the spec?
(In reply to comment #3) > (From update of attachment 164760 [details]) > Was this a recent regression? I assume our new behavior now properly matches FF and the spec? It must be as old as inlineLogicalWidth() so I'm not sure it's a regression - may have always been this way. The new behaviour matches FF and Opera.
Thank you again Robert. :)
Comment on attachment 164760 [details] Patch Clearing flags on attachment: 164760 Committed r130663: <http://trac.webkit.org/changeset/130663>
All reviewed patches have been landed. Closing bug.
Comment on attachment 164760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164760&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:289 > +static bool shouldAddBorderPaddingMargin(RenderObject* child, bool &checkSide) This is incorrect formatting for the WebKit project. It should be bool& checkSide. Also, I find the name and purpose of this argument to be quite confusing. I suspect this function should be marked inline. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:291 > + if (!child || (child->isText() && !toRenderText(child)->textLength())) This needs a “why” comment. Why is this the correct rule? The change log says, but the code needs to say it too. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:294 > + checkSide = false; > + return checkSide; Why not just return false here? This strange function that returns a boolean and also ands that same boolean with an argument isn}t really justified by the call site. This could just be a normal function and the rest of the logic could be done in a clearer way at the call site.
I've created https://bugs.webkit.org/show_bug.cgi?id=98803 to track this. (In reply to comment #8) > (From update of attachment 164760 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164760&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:289 > > +static bool shouldAddBorderPaddingMargin(RenderObject* child, bool &checkSide) > > This is incorrect formatting for the WebKit project. It should be bool& checkSide. > > Also, I find the name and purpose of this argument to be quite confusing. > > I suspect this function should be marked inline. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:291 > > + if (!child || (child->isText() && !toRenderText(child)->textLength())) > > This needs a “why” comment. Why is this the correct rule? The change log says, but the code needs to say it too. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:294 > > + checkSide = false; > > + return checkSide; > > Why not just return false here? > > This strange function that returns a boolean and also ands that same boolean with an argument isn}t really justified by the call site. This could just be a normal function and the rest of the logic could be done in a clearer way at the call site.