Bug 243134

Summary: Fix aspect-ratio/fieldset-element-001.html
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 15   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Rob Buis
Reported 2022-07-23 09:26:13 PDT
Fix aspect-ratio/fieldset-element-001.html.
Attachments
Patch (2.73 KB, patch)
2022-07-23 09:27 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (3.21 KB, patch)
2022-07-24 05:59 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (2.83 KB, patch)
2022-07-24 11:26 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Rob Buis
Comment 1 2022-07-23 09:27:58 PDT
Created attachment 461158 [details] Patch
Rob Buis
Comment 2 2022-07-24 05:59:49 PDT
Created attachment 461174 [details] Patch
Rob Buis
Comment 3 2022-07-24 11:26:09 PDT
Created attachment 461179 [details] Patch
Darin Adler
Comment 4 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.
EWS
Comment 5 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].
Radar WebKit Bug Importer
Comment 6 2022-07-26 01:13:16 PDT
<rdar://problem/97592883>
Rob Buis
Comment 7 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.
Note You need to log in before you can comment on or make changes to this bug.