Bug 83380 – Don't expose the intrinsic padding concept to the code outside rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

Currently intrinsic padding is available through a boolean on the padding* functions. This however exposes it to the outside world (and actually CSSComputedStyleDeclaration already relies on / works around it) when only the table rendering code should know about this concept.

There has been some back and forth discussion going on about that. See for example, bug 33593 and bug 83092.

[Julien Chaffraix]

Created attachment 136054 [details]
Proposed clean-up.

[Dave Barton]

I vote to rename the computedPadding* functions to cssOnlyPadding*. In all the padding functions something is "computed". I know you mean this in the sense of "computed CSS", but I don't know if a casual reader would get it.

> // The content area of the box (excludes padding - and intrinsic padding for table cells - and border).
// The content area of the box (excludes padding - and intrinsic padding for table cells, etc. - and border).

> // This returns the CSS computed padding values.
// These return the CSS computed padding values.

> // Those functions are used during layout. Table cells override
> // them to include an alignment padding (called intrinsic padding). 
These functions are used during layout. Table cells and some MathML renderers override
them to include extra "intrinsic padding" for alignment and other uses. 

I need paddingStart and paddingEnd to be virtual also, for MathML.

===

With the above changes, I really like this patch!

For the record, here's my point of view. I'd like to use intrinsic padding for MathML also, as in my recent patch for bug 82353, where intrinsic padding leaves room for a square root sign. I like to think of intrinsic padding as maybe "decorated padding" that's similar to a fancy kind of border, but contained in the "padding". This would be very helpful for MathML. That is, when RenderBlock::layout() uses the equation border-box = content-box + padding + border (+ maybe scroll bars), I want it to allow this padding to be more than just padding from CSS styles. Then this "rendering padding" needs to be treated as padding by availableLogicalWidth() for example when computing space for laying out or centering children.

So RenderBoxModelObject is a base class with two kinds of padding functions. The rendering one that includes intrinsic padding is virtual, so derived classes like TableCell and some MathML classes can override it. Other classes don't know the specifics of intrinsic padding in these derived classes, but they do know there are two kinds of padding functions. Actually the one without intrinsic padding is mostly just used by computed styles, and doesn't need to be virtual, as you point out.

[Alexis Menard (darktears)]

Comment on attachment 136054 [details]
Proposed clean-up.

Yes seems like a better solution and less intrusive.

[Julien Chaffraix]

(In reply to comment #2)
> I vote to rename the computedPadding* functions to cssOnlyPadding*. In all the padding functions something is "computed". I know you mean this in the sense of "computed CSS", but I don't know if a casual reader would get it.

For the record, I thought about a similar name (cssPadding*) but I thought this would be confusing as it's not the raw CSS information (ie a Length) but a computed CSS information (percent / calculated length have been resolved). computedCSSPadding* could be a good compromise here.

[Dave Barton]

computedCSSPadding* sounds great to me.

[Julien Chaffraix]

Created attachment 136562 [details]
Updated patch against ToT, also took Dave's comment into account.

[Dave Barton]

Comment on attachment 136562 [details]
Updated patch against ToT, also took Dave's comment into account.

LGTM, FWIW.

[Eric Seidel (no email)]

Comment on attachment 136562 [details]
Updated patch against ToT, also took Dave's comment into account.

Seems reasonable to me.

[WebKit Review Bot]

Comment on attachment 136562 [details]
Updated patch against ToT, also took Dave's comment into account.

Clearing flags on attachment: 136562

Committed r113823: <http://trac.webkit.org/changeset/113823>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.