Bug 83092

Summary: Remove intrinsic padding from contentBoxRect(), etc.
Product: WebKit Reporter: Dave Barton <dbarton>
Component: Layout and RenderingAssignee: Dave Barton <dbarton>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hyatt, jchaffraix, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case with intrinsic padding (in silver)
none
Patch
none
Patch none

Dave Barton
Reported 2012-04-03 16:28:08 PDT
Created attachment 135451 [details] Test case with intrinsic padding (in silver) "Intrinsic padding" does not count as CSS padding, but is treated as padding by basic layout and rendering code, e.g. RenderBlock::layout(). A lot of code relies on the equation border-box = content-box + padding + border. To keep this valid, I think the following 5 content-box functions in RenderBox.h should not include intrinsic padding in the content box by default: contentBoxRect(), contentWidth(), contentHeight(), contentLogicalWidth(), contentLogicalHeight(). Instead, sizingBox(renderer) in CSSComputedStyleDeclaration.cpp should pass contentBoxRect() a parameter to explicitly include include intrinsic padding for computed CSS content-box values [for javascript getComputedStyle()]. This seems more consistent with how the padding...() functions behave since the patch for bug 23487, and will work better for MathML. It's hard to come up with a test case for this, since the only intrinsic padding now is in vertical positioning of table cells, and parameters are mostly passed as needed to make this case work. Apparently the only two places with observable behavior that would really be affected by this change are demonstrated in the attached test case as follows. If you double-click to the right of the topmost or bottommost portion of the silver box (intrinsic padding), the left tan box should be selected. This works in Safari 5.1.5, for instance. In the latest nightly build, since bug 33593 was fixed, this no longer works in the bottom position. Also, if you hover over the right <td> element in the Web Inspector window (right-click and "Inspect Element" to open the Inspector), the highlighted box is too tall. These cases could be fixed by each passing a parameter to e.g. contentBoxRect(). More seriously, for MathML I would like to introduce some horizontal intrinsic padding, but then e.g. { text-align: center } now works incorrectly in my tests, basically because a containing block's content-box is computed to include intrinsic padding. Thus I propose that normal flow routines in general that treat intrinsic padding as padding, to not also put the intrinsic padding in the content box. Opinions?
Attachments
Test case with intrinsic padding (in silver) (281 bytes, text/html)
2012-04-03 16:28 PDT, Dave Barton 		no flags
Details
Patch (17.90 KB, patch)
2012-04-03 20:32 PDT, Dave Barton 		no flags
DetailsFormatted DiffDiff
Patch (8.35 KB, patch)
2012-04-05 10:06 PDT, Dave Barton 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dave Barton
Comment 1 2012-04-03 20:32:06 PDT
Created attachment 135500 [details] Patch
Dave Barton
Comment 2 2012-04-04 10:42:05 PDT
The renaming part of this patch is motivated by the following. Current code includes: enum PaddingOptions { IncludeIntrinsicPadding, ExcludeIntrinsicPadding }; LayoutUnit contentWidth(PaddingOptions paddingOption = ExcludeIntrinsicPadding) const { return clientWidth() - paddingLeft(paddingOption) - paddingRight(paddingOption); } Thus contentWidth(ExcludeIntrinsicPadding) actually returns a width that includes intrinsic padding, and contentWidth(IncludeIntrinsicPadding) returns a width that excludes it. Since paddingOption really controls whether the padding includes the intrinsic padding or not, the patch proposes: enum PaddingOptions /* for intrinsic padding */ { PaddingIncludesIntrinsic, PaddingExcludesIntrinsic }; Then e.g. contentWidth(PaddingIncludesIntrinsic) returns a content width without intrinsic padding, since it's considered part of the padding and not the content box.
Julien Chaffraix
Comment 3 2012-04-04 12:39:51 PDT
Sincerely I think we should stop this madness. I wish I had stopped the original change as: 1) it makes the code a lot more error prone. 2) it exposes the intrinsic padding information to a whole new range of classes that really shouldn't know about it. Note that your change makes the situation better but I really don't think continuing down that path is helping anything. If we can, I would rather see a solution based on: * removing the enum parameter from the different widths / heights (maybe also padding*) * adding more functions that can be called when you want the information including our intrinsic quirks (like contentHeightIncludingIntrinsicPadding or fullContentHeight but I don't like those names). Most of them would actually be on RenderTableCell as only the table code needs to know about that.
Dave Barton
Comment 4 2012-04-04 14:23:20 PDT
I agree with (1) maybe but not (2). CSSComputedStyleDeclaration needs the padding to not include the intrinsic padding, whereas almost everything else needs the opposite. (TableCell and maybe TableRow need to handle it specially, of course, as will MathML.) So we have to expose the concept somehow to these classes at least. Also other derived classes in the future might want to use intrinsic padding. So I think something about it should be in RenderBox.h and even RenderBoxModelObject.h, as in the Dave Hyatt's original bug 23487 patch. We don't want CSSComputedStyleDeclaration to treat TableCell as a special case, we want it to just call a virtual function in a RenderBoxModelObject and let any derived class (like TableCell) handle any intrinsic padding privately. If the real problem is that you'd rather not have intrinsic padding spread to e.g. MathML, then we could discuss that, probably in a separate bug. But I think this patch is an improvement and needs to be landed in any case, though we could remove the parameter from some functions by creating alternative versions of them as you say. But if these versions are also virtual functions in RenderBoxModelObject.h, I don't think things are really made clearer or less error-prone. Naming the new functions is as hard as naming these enum values.
Dave Barton
Comment 5 2012-04-04 14:30:14 PDT
P.S. By the "original change", Julien presumably means the patch for bug 33593 landed a month ago. I agree that I'm just trying to revert these 5 functions to their default behavior before that patch was landed. We need bug 33593 fixed for computed CSS values, but not for all other uses of the content box.
Dave Barton
Comment 6 2012-04-04 16:46:51 PDT
I think Julien's comment is finally sinking in for me. What if we remove the enum parameter from the 5 content-box functions, putting them back to their old behavior of treating intrinsic padding as padding. Then we edit just sizingBox() in CSSComputedStyleDeclaration.cpp to use the padding functions instead of contentBoxRect(). We could then even go back to the old enum names if you want, making this a very small patch, while still fixing all my problems. Will this minimize confusion and the chance of future bugs for everyone?
Julien Chaffraix
Comment 7 2012-04-04 19:18:37 PDT
> Then we edit just sizingBox() in CSSComputedStyleDeclaration.cpp to use the padding functions instead of contentBoxRect(). We could then even go back to the old enum names if you want, making this a very small patch, while still fixing all my problems. Will this minimize confusion and the chance of future bugs for everyone? That was something I had in mind and dhyatt@ mentioned it too (IIRC). It would be a good compromise from my point of view. What I envisioned would be to split the paths and explicitly say what you get: * the usual padding* which is what RenderBoxModelObject returns and is derived directly from our CSS information. * the paddingAndIntrinsic* ones that we use internally to be able to handle cell's intrinsic padding when doing layout. I am trying something along those lines but that shouldn't stop you from a quick fix. I just fear spreading the enum more wil do much harm as people will rely on it.
Dave Barton
Comment 8 2012-04-04 19:39:56 PDT
Great! I'll make a patch now or in the morning. > What I envisioned would be to split the paths and explicitly say what you get: > * the usual padding* which is what RenderBoxModelObject returns and is derived directly from our CSS information. > * the paddingAndIntrinsic* ones that we use internally to be able to handle cell's intrinsic padding when doing layout. But we need the virtual padding*() functions to include any intrinsic padding by default, when called by any general (outside RenderTableCell.cpp) rendering code except in CSSComputedStyleDeclaration.cpp.
Dave Barton
Comment 9 2012-04-05 10:06:49 PDT
Created attachment 135846 [details] Patch
Julien Chaffraix
Comment 10 2012-04-05 19:41:36 PDT
Comment on attachment 135846 [details] Patch r=me. Filing a bug about not exposing the intrinsic padding concept outside the rendering code would be appreciated.
WebKit Review Bot
Comment 11 2012-04-05 20:05:02 PDT
Comment on attachment 135846 [details] Patch Clearing flags on attachment: 135846 Committed r113407: <http://trac.webkit.org/changeset/113407>
WebKit Review Bot
Comment 12 2012-04-05 20:05:13 PDT
All reviewed patches have been landed. Closing bug.
Dave Barton
Comment 13 2012-04-05 20:20:36 PDT
> Filing a bug about not exposing the intrinsic padding concept outside the rendering code would be appreciated. I'm not sure what this means. Do you want me to just add a comment by the enum in RenderBoxModelObject.h?
Julien Chaffraix
Comment 14 2012-04-06 10:55:41 PDT
(In reply to comment #13) > > Filing a bug about not exposing the intrinsic padding concept outside the rendering code would be appreciated. > > I'm not sure what this means. Do you want me to just add a comment by the enum in RenderBoxModelObject.h? I just filed bug 83380 to cover that. Sorry for not being clear.
Note You need to log in before you can comment on or make changes to this bug.