Bug 63074 - Border, margin and padding of an inline's inline ancestors counted twice
Summary: Border, margin and padding of an inline's inline ancestors counted twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Robert Hogan
URL: http://jsbin.com/ixuka6/5/
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-21 09:30 PDT by Francois du Toit
Modified: 2012-10-09 11:31 PDT (History)
4 users (show)

See Also:


Attachments
float wrap bug (2.31 KB, text/html)
2011-06-21 09:30 PDT, Francois du Toit
no flags Details
Patch (6.89 KB, patch)
2012-09-19 12:02 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Francois du Toit 2011-06-21 09:30:49 PDT
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)
Comment 1 Robert Hogan 2012-09-19 12:02:04 PDT
Created attachment 164760 [details]
Patch
Comment 2 Dave Hyatt 2012-09-19 12:07:41 PDT
Comment on attachment 164760 [details]
Patch

r=me
Comment 3 Eric Seidel (no email) 2012-09-19 12:08:29 PDT
Comment on attachment 164760 [details]
Patch

Was this a recent regression?  I assume our new behavior now properly matches FF and the spec?
Comment 4 Robert Hogan 2012-09-19 12:11:48 PDT
(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.
Comment 5 Eric Seidel (no email) 2012-09-19 12:17:55 PDT
Thank you again Robert. :)
Comment 6 WebKit Review Bot 2012-10-08 11:43:58 PDT
Comment on attachment 164760 [details]
Patch

Clearing flags on attachment: 164760

Committed r130663: <http://trac.webkit.org/changeset/130663>
Comment 7 WebKit Review Bot 2012-10-08 11:44:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2012-10-08 17:52:01 PDT
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.
Comment 9 Robert Hogan 2012-10-09 11:31:36 PDT
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.