Summary: | Border, margin and padding of an inline's inline ancestors counted twice | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Francois du Toit <bluegray> | ||||||
Component: | CSS | Assignee: | Robert Hogan <robert> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | eric, robert, shanestephens, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
URL: | http://jsbin.com/ixuka6/5/ | ||||||||
Attachments: |
|
Description
Francois du Toit
2011-06-21 09:30:49 PDT
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. |