RenderStyle::resetPadding sets incorrect computed value (auto)

[zalan]

should be 0

[zalan]

Created attachment 412361 [details]
Patch

[zalan]

Alternatively we could just set it to LengthBox(Fixed). Not sure what the preferred way is in RenderStyle.

[Simon Fraser (smfr)]

Comment on attachment 412361 [details]
Patch

Is this testable with CSS reset?

[Darin Adler]

Comment on attachment 412361 [details]
Patch

Does this have any effect?

[zalan]

(In reply to Darin Adler from comment #4)
> Comment on attachment 412361 [details]
> Patch
> 
> Does this have any effect?
Absolutely.

This is how in IFC we normally resolve the computed padding value (e.g. padding left):
 valueForLength(style.paddingLeft(), containingBlockWidth);
Now for the (incorrect)auto specified value, valueForLength returns the containingBlockWidth as opposed to the expected, initial value of 0.
(I noticed this after enabling <select> element in IFC. RenderTheme calls style.resetPadding() on the select renderer and when IFC resolves the computed value, it sees a non-zero padding left/right/top/bottom).

[zalan]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 412361 [details]
> Patch
> 
> Is this testable with CSS reset?
the unset? It does not seem to call resetPadding.

[Antti Koivisto]

Comment on attachment 412361 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.h:1027
> +    void resetPadding() { SET_VAR(m_surroundData, padding, LengthBox(initialPadding().intValue())); }

Just initialPadding()?

[zalan]

(In reply to Antti Koivisto from comment #7)
> Comment on attachment 412361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412361&action=review
> 
> > Source/WebCore/rendering/style/RenderStyle.h:1027
> > +    void resetPadding() { SET_VAR(m_surroundData, padding, LengthBox(initialPadding().intValue())); }
> 
> Just initialPadding()?
That was my initial attempt, but LengthBox does not take Length.

[EWS]

Committed r269037: <https://trac.webkit.org/changeset/269037>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412361 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70719975>

[Darin Adler]

If this has an effect why isn’t a test case going from fail to pass?

[zalan]

(In reply to Darin Adler from comment #11)
> If this has an effect why isn’t a test case going from fail to pass?
I should have been more specific. There's no effect on trunk yet because the rendering code is written in a way that the incorrect 'auto' value gets resolved to 0.   

LayoutUnit RenderBoxModelObject::computedCSSPadding(const Length& padding) const
{
    LayoutUnit w;
    if (padding.isPercentOrCalculated())
        w = containingBlockLogicalWidthForContent();
    return minimumValueForLength(padding, w);
}
minimumValueForLength() returns 0 for auto values. 
The "from fail to pass" progression is behind a flag which is currently turned off on trunk (where we start exercising some more LFC code for inline content, specifically where LFC calls valueForLength(style.paddingLeft(), containingBlockWidth) to resolve the computed value.)
I could have waited with this fix until after we enabled the flag to show the progression but it looked like a valid correctness fix in itself (since 'auto' is an incorrect value for padding).

[Darin Adler]

(In reply to zalan from comment #12)
> There's no effect on trunk yet because the
> rendering code is written in a way that the incorrect 'auto' value gets
> resolved to 0.

OK.