Bug 243134

Summary:

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

Product:

WebKit

Reporter:

Rob Buis <rbuis>

Component:

CSS

Assignee:

Rob Buis <rbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari 15

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Rob Buis 2022-07-23 09:26:13 PDT

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

Comment 1 Rob Buis 2022-07-23 09:27:58 PDT

Created attachment 461158 [details]
Patch

Comment 2 Rob Buis 2022-07-24 05:59:49 PDT

Created attachment 461174 [details]
Patch

Comment 3 Rob Buis 2022-07-24 11:26:09 PDT

Created attachment 461179 [details]
Patch

Comment 4 Darin Adler 2022-07-25 16:37:27 PDT

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.

Comment 5 EWS 2022-07-26 01:12:53 PDT

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].

Comment 6 Radar WebKit Bug Importer 2022-07-26 01:13:16 PDT

<rdar://problem/97592883>

Comment 7 Rob Buis 2022-07-26 01:19:26 PDT

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.