Bug 63074 – Border, margin and padding of an inline's inline ancestors counted twice

• Component: CSS
• Status: RESOLVED FIXED

[Francois du Toit]

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)

[Robert Hogan]

Created attachment 164760 [details]
Patch

[Dave Hyatt]

Comment on attachment 164760 [details]
Patch

r=me

[Eric Seidel (no email)]

Comment on attachment 164760 [details]
Patch

Was this a recent regression?  I assume our new behavior now properly matches FF and the spec?

[Robert Hogan]

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

[Eric Seidel (no email)]

Thank you again Robert. :)

[WebKit Review Bot]

Comment on attachment 164760 [details]
Patch

Clearing flags on attachment: 164760

Committed r130663: <http://trac.webkit.org/changeset/130663>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Robert Hogan]

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.