Bug 242052 – Element with aspect-ratio, padding and height: 100% grow in size when repainted

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Gordon]

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.

[Rob Buis]

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.

[Radar WebKit Bug Importer]

<rdar://problem/96419984>

[Rob Buis]

Created attachment 460712 [details]
Patch

[Rob Buis]

Created attachment 461040 [details]
Patch

[Rob Buis]

Created attachment 461041 [details]
Patch

[Rob Buis]

Created attachment 461135 [details]
Patch

[EWS Watchlist]

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

[Simon Fraser (smfr)]

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?

[Rob Buis]

Created attachment 461147 [details]
Patch

[Rob Buis]

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.

[Darin Adler]

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?

[zalan]

(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()).

[EWS]

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

[Rob Buis]

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.