Bug 206317 – Clamp paddingBoxWidth/Height to a minimum of zero

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

[Sunny He]

Under rare circumstances the computed paddingBoxWidth/Height may become negative, raising the possibility for errors in later computations. Clamp these values to a minimum of zero to ensure this doesn't happen.

[Sunny He]

rdar://57102010

[Sunny He]

Created attachment 387848 [details]
Patch

[Sunny He]

Created attachment 388592 [details]
Patch

[Sunny He]

Created attachment 388593 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 388593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388593&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:464
> +    WTFLogAlways("computeConstrainedLogicalWidth logicalWidth=%f, marginStart=%f, marginEnd=%f, size().width()=%f, clientWidth()=%f",
> +        logicalWidth.toFloat(), marginStart.toFloat(), marginEnd.toFloat(), size().width().toFloat(), clientWidth().toFloat());
> +    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + borderLeft() + borderRight() + verticalScrollbarWidth())));
> +    WTFLogAlways("computeConstrainedLogicalWidth final logicalWidth=%f", logicalWidth.toFloat());

No logging please!

[Sunny He]

Created attachment 388594 [details]
Patch

[Sunny He]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 388593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388593&action=review
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:464
> > +    WTFLogAlways("computeConstrainedLogicalWidth logicalWidth=%f, marginStart=%f, marginEnd=%f, size().width()=%f, clientWidth()=%f",
> > +        logicalWidth.toFloat(), marginStart.toFloat(), marginEnd.toFloat(), size().width().toFloat(), clientWidth().toFloat());
> > +    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + borderLeft() + borderRight() + verticalScrollbarWidth())));
> > +    WTFLogAlways("computeConstrainedLogicalWidth final logicalWidth=%f", logicalWidth.toFloat());
> 
> No logging please!

Fixed, looks like EWS is happy with this one.

[zalan]

Comment on attachment 388594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388594&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:461
> -    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + (size().width() - clientWidth()))));
> +    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + borderLeft() + borderRight() + verticalScrollbarWidth())));

I am confused how this works (or even worked before). The logical width here (which is the content logical width) of an inline replaced box is supposed to be the containing block's content box width - (margin start + border inline start + padding inline start + padding inline end + border inline end + margin end), since the constraint equation is as follows containing block's content width = margin s + border s + padding s + content width + padding e + border e + margin e.
It's even in the comment a few lines above.

[Darin Adler]

Comment on attachment 388594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388594&action=review

> Source/WebCore/rendering/RenderBox.h:221
> +    LayoutUnit paddingBoxWidth() const { return std::max<LayoutUnit>(0, width() - borderLeft() - borderRight() - verticalScrollbarWidth()); }
> +    LayoutUnit paddingBoxHeight() const { return std::max<LayoutUnit>(0, height() - borderTop() - borderBottom() - horizontalScrollbarHeight()); }

A more economical way to write this is "std::max(0_lu,"

>> Source/WebCore/rendering/RenderReplaced.cpp:461
>> +    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + borderLeft() + borderRight() + verticalScrollbarWidth())));
> 
> I am confused how this works (or even worked before). The logical width here (which is the content logical width) of an inline replaced box is supposed to be the containing block's content box width - (margin start + border inline start + padding inline start + padding inline end + border inline end + margin end), since the constraint equation is as follows containing block's content width = margin s + border s + padding s + content width + padding e + border e + margin e.
> It's even in the comment a few lines above.

I am going to say review+ here, but I am concerned that Alan is confused about this, since this is an area he's expert on. Please follow up with him.

[zalan]

(In reply to Darin Adler from comment #9)
> Comment on attachment 388594 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388594&action=review
> 
> > Source/WebCore/rendering/RenderBox.h:221
> > +    LayoutUnit paddingBoxWidth() const { return std::max<LayoutUnit>(0, width() - borderLeft() - borderRight() - verticalScrollbarWidth()); }
> > +    LayoutUnit paddingBoxHeight() const { return std::max<LayoutUnit>(0, height() - borderTop() - borderBottom() - horizontalScrollbarHeight()); }
> 
> A more economical way to write this is "std::max(0_lu,"
> 
> >> Source/WebCore/rendering/RenderReplaced.cpp:461
> >> +    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + borderLeft() + borderRight() + verticalScrollbarWidth())));
> > 
> > I am confused how this works (or even worked before). The logical width here (which is the content logical width) of an inline replaced box is supposed to be the containing block's content box width - (margin start + border inline start + padding inline start + padding inline end + border inline end + margin end), since the constraint equation is as follows containing block's content width = margin s + border s + padding s + content width + padding e + border e + margin e.
> > It's even in the comment a few lines above.
> 
> I am going to say review+ here, but I am concerned that Alan is confused
> about this, since this is an area he's expert on. Please follow up with him.

(In reply to zalan from comment #8)
> Comment on attachment 388594 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388594&action=review
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:461
> > -    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + (size().width() - clientWidth()))));
> > +    logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd + borderLeft() + borderRight() + verticalScrollbarWidth())));
> 
> I am confused how this works (or even worked before). The logical width here
> (which is the content logical width) of an inline replaced box is supposed
> to be the containing block's content box width - (margin start + border
> inline start + padding inline start + padding inline end + border inline end
> + margin end), since the constraint equation is as follows containing
> block's content width = margin s + border s + padding s + content width +
> padding e + border e + margin e.
> It's even in the comment a few lines above.
I guess this is fine since you are not changing behavior here. I am just baffled how incorrect this is (as per https://www.w3.org/TR/CSS22/visudet.html#inline-replaced-width) and specifically that there's even a comment above this line disagreeing with the logic here. I would put a fixme or some kind of explanation why it is ok to have it this way.

[zalan]

Please address Darin's comment before landing.

[Sunny He]

Created attachment 389189 [details]
Patch

[Sunny He]

Comment on attachment 389189 [details]
Patch

Need to take another look at this, just changing to match the spec's expression is clobbering some css tests.

[Sunny He]

Created attachment 389201 [details]
Patch

[Sunny He]

Created attachment 389207 [details]
Patch

[Sunny He]

(In reply to Sunny He from comment #14)
> Created attachment 389201 [details]
> Patch

Figures, missed the tests again.

[WebKit Commit Bot]

Comment on attachment 389207 [details]
Patch

Clearing flags on attachment: 389207

Committed r255413: <https://trac.webkit.org/changeset/255413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.