Bug 242052

Summary: Element with aspect-ratio, padding and height: 100% grow in size when repainted
Product: WebKit Reporter: Gordon <gordonclark127>
Component: Layout and RenderingAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, bfulgham, changseok, clopez, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, rbuis, simon.fraser, webkit-bug-importer, webkit-layout-noreply, youennf, zalan
Priority: P2 Keywords: InRadar
Version: Safari 15   
Hardware: Mac (Intel)   
OS: macOS 12   
Attachments:
Description Flags
Smallest viable reproduction attachment
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Gordon 2022-06-27 23:54:50 PDT 
Created attachment 460514 [details]
Smallest viable reproduction attachment

Elements that have the CSS properties: aspect-ratio, padding and height (specifically to 100%) set seem to inadvertently scale upwards whenever a repaint is triggered. These three properties were the least amount of properties I could find to set that would trigger the problem

I've included an attachment of the HTML I used, but the problems can easily be previewed on this code sandbox: https://codesandbox.io/s/broken-safari-forked-1n9o4k?file=/index.html. It's important to trigger the hover effect to see that the button elements grow. 

In practice I've found the hover effect is unimportant, but it helps to demonstrate the problem. Toggling styles in the developer tools (such as cursor) can cause the elements to scale upward.
Comment 1 Rob Buis 2022-06-30 06:06:01 PDT 
In case anybody else is looking, this seems specific to grid, I tried display:flex and display:block for example and the issue is not there.
Comment 2 Radar WebKit Bug Importer 2022-07-04 23:55:12 PDT 
<rdar://problem/96419984>
Comment 3 Rob Buis 2022-07-06 06:17:05 PDT 
Created attachment 460712 [details]
Patch
Comment 4 Rob Buis 2022-07-20 03:06:52 PDT 
Created attachment 461040 [details]
Patch
Comment 5 Rob Buis 2022-07-20 08:06:10 PDT 
Created attachment 461041 [details]
Patch
Comment 6 Rob Buis 2022-07-22 03:25:29 PDT 
Created attachment 461135 [details]
Patch
Comment 7 EWS Watchlist 2022-07-22 03:28:19 PDT 
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 8 Simon Fraser (smfr) 2022-07-22 11:19:01 PDT 
Comment on attachment 461135 [details]
Patch

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

> COMMIT_MESSAGE:1
> +Element with aspect-ratio, padding and height: 100% grow in size when repainted 

"when repainted" seems wrong. This is per layout, right?
Comment 9 Rob Buis 2022-07-22 11:58:59 PDT 
Created attachment 461147 [details]
Patch
Comment 10 Rob Buis 2022-07-22 12:00:41 PDT 
Comment on attachment 461135 [details]
Patch

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

>> COMMIT_MESSAGE:1
>> +Element with aspect-ratio, padding and height: 100% grow in size when repainted 
> 
> "when repainted" seems wrong. This is per layout, right?

Right, layout is wrong so as a consequence the (re)paint problem is visible. I stuck to the original bug title since I had a hard time making a terse first line, but I did another attempt.
Comment 11 Darin Adler 2022-07-25 17:24:14 PDT 
Comment on attachment 461147 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:5489
> +        if (style().logicalWidth().isPercentOrCalculated() && parent()->style().logicalWidth().isFixed())

Wait, the rest of this function does not dereference parent(). Is this guaranteed to be non-null?
Comment 12 zalan 2022-07-25 17:36:39 PDT 
(In reply to Darin Adler from comment #11)
> Comment on attachment 461147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=461147&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:5489
> > +        if (style().logicalWidth().isPercentOrCalculated() && parent()->style().logicalWidth().isFixed())
> 
> Wait, the rest of this function does not dereference parent(). Is this
> guaranteed to be non-null?
Also in block layout, the canonical way to get to "parent" is by calling containingBlock() (i.e parent in the renderer tree is not necessarily the "parent" in CSS terms.) It's rarely the case when you specifically want "tree parent" and not containing block in block layout (the confusion comes from the fact that containing block translates to "tree parent" in most cases, so it's easy to just call parent() instead of containingBlock()).
Comment 13 EWS 2022-07-26 02:12:58 PDT 
Committed 252819@main (39aa8ef8099b): <https://commits.webkit.org/252819@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461147 [details].
Comment 14 Rob Buis 2022-07-26 02:17:20 PDT 
Comment on attachment 461147 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBox.cpp:5489
>>> +        if (style().logicalWidth().isPercentOrCalculated() && parent()->style().logicalWidth().isFixed())
>> 
>> Wait, the rest of this function does not dereference parent(). Is this guaranteed to be non-null?
> 
> Also in block layout, the canonical way to get to "parent" is by calling containingBlock() (i.e parent in the renderer tree is not necessarily the "parent" in CSS terms.) It's rarely the case when you specifically want "tree parent" and not containing block in block layout (the confusion comes from the fact that containing block translates to "tree parent" in most cases, so it's easy to just call parent() instead of containingBlock()).

In this case I rely on isGridItem, which established parent() exists and is a RenderGrid. That is the container I want to test the percentage against and see if the percentage turns out to be definite.