Bug 243134 – Fix aspect-ratio/fieldset-element-001.html

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Fix aspect-ratio/fieldset-element-001.html.

[Rob Buis]

Created attachment 461158 [details]
Patch

[Rob Buis]

Created attachment 461174 [details]
Patch

[Rob Buis]

Created attachment 461179 [details]
Patch

[Darin Adler]

Comment on attachment 461179 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3193
> -                    *intrinsicHeight -= borderAndPaddingLogicalHeight();
> +                    *intrinsicHeight -= RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter();

Obviously OK here, but are we sure that none of the other clients of borderAndPaddingLogicalHeight() need this adjustment?

If it was used anywhere else we’d want a named function rather than a long list.

[EWS]

Committed 252817@main (8bfc902ada00): <https://commits.webkit.org/252817@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/97592883>

[Rob Buis]

Comment on attachment 461179 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3193
>> +                    *intrinsicHeight -= RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter();
> 
> Obviously OK here, but are we sure that none of the other clients of borderAndPaddingLogicalHeight() need this adjustment?
> 
> If it was used anywhere else we’d want a named function rather than a long list.

I tried to change the borderAndPaddingLogicalHeight occurrence a few lines below and that caused test failures.  Hopefully once we refactor aspect-ratio code some more we can find out if there is anything specific to aspect-ratio or other places need it two. Right now we use this approach in two places (including this change) but indeed if we need it more a function is in order.