Bug 221671 – Take box-sizing into account in replaced element intrinsic sizing

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

[Rob Buis]

Take into account box-sizing in replaced element intrinsic sizing

[Rob Buis]

Created attachment 419838 [details]
Patch

[Rob Buis]

Created attachment 419854 [details]
Patch

[Rob Buis]

Created attachment 420278 [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

[Rob Buis]

Created attachment 420441 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/74431153>

[Rob Buis]

Created attachment 420643 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 420643 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:579
> +                BoxSizing boxSizing = BoxSizing::ContentBox;
> +                if (style().hasAspectRatio())
> +                    boxSizing = style().boxSizingForAspectRatio();

Maybe this should be a function on RenderStyle, call boxSizingForAspectRatio()?

[Rob Buis]

Created attachment 421212 [details]
Patch

[Rob Buis]

Created attachment 421298 [details]
Patch

[Rob Buis]

Comment on attachment 420643 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:579
>> +                    boxSizing = style().boxSizingForAspectRatio();
> 
> Maybe this should be a function on RenderStyle, call boxSizingForAspectRatio()?

We already have boxSizingForAspectRatio on RenderStyle. The code may be a bit misleading, the idea is for non CSS aspect-ratio to keep doing what we did before, i.e. ignore box-sizing here, but when CSS aspect-ratio is applied, pass the box-sizing in use to resolveWidthForRatio/resolveHeightForRatio since the passed intrinsicRatio is relative to either the border-box or content-box, depeding on boxSizingForAspectRatio.

[Simon Fraser (smfr)]

Comment on attachment 421298 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:539
> +    if (boxSizing == BoxSizing::BorderBox)
> +        return roundToInt(round((logicalHeight + borderAndPaddingLogicalHeight) * aspectRatio)) - borderAndPaddingLogicalWidth;
> +    return roundToInt(round(logicalHeight * aspectRatio));

Do we know why this code does roundToInt()?

[Darin Adler]

Comment on attachment 421298 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:539
>> +    return roundToInt(round(logicalHeight * aspectRatio));
> 
> Do we know why this code does roundToInt()?

Both roundToInt and round are suspect. Neither of those snaps to a device pixel, so what is the goal?

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #13)
> Comment on attachment 421298 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421298&action=review
> 
> >> Source/WebCore/rendering/RenderReplaced.cpp:539
> >> +    return roundToInt(round(logicalHeight * aspectRatio));
> > 
> > Do we know why this code does roundToInt()?
> 
> Both roundToInt and round are suspect. Neither of those snaps to a device
> pixel, so what is the goal?

This is layout code, so we shouldn't be snapping to device pixels at this point.

I think historically line layout has rounded to integral values, and this is part of that.

[Rob Buis]

Comment on attachment 421298 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderReplaced.cpp:539
>>>> +    return roundToInt(round(logicalHeight * aspectRatio));
>>> 
>>> Do we know why this code does roundToInt()?
>> 
>> Both roundToInt and round are suspect. Neither of those snaps to a device pixel, so what is the goal?
> 
> This is layout code, so we shouldn't be snapping to device pixels at this point.
> 
> I think historically line layout has rounded to integral values, and this is part of that.

Possibly it can go, r112229 added this:
// FIXME: Remove unnecessary round/roundToInt calls from this method when layout is off ints: webkit.org/b/63656

[Rob Buis]

Created attachment 421613 [details]
Patch

[Rob Buis]

Comment on attachment 421613 [details]
Patch

Looks like we do not need roundToInt.

[Simon Fraser (smfr)]

Comment on attachment 421613 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:540
> +    if (boxSizing == BoxSizing::BorderBox)
> +        return LayoutUnit(round((logicalHeight + borderAndPaddingLogicalHeight) * aspectRatio)) - borderAndPaddingLogicalWidth;
> +    return LayoutUnit(round(logicalHeight * aspectRatio));
> +}

fromFloatRound()?

[Rob Buis]

Created attachment 421906 [details]
Patch

[Rob Buis]

Comment on attachment 421613 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:540
>> +}
> 
> fromFloatRound()?

Sure, done.

[Rob Buis]

Created attachment 421932 [details]
Patch

[Rob Buis]

Comment on attachment 421613 [details]
Patch

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

>>> Source/WebCore/rendering/RenderReplaced.cpp:540
>>> +}
>> 
>> fromFloatRound()?
> 
> Sure, done.

Using fromFloatRound caused (even visual) regressions, so I suggest we stick to using round as before, my goal here is to add CSS aspect-ratio logic and not regress the replaced aspect-ratio handling.

[EWS]

Committed r273753: <https://commits.webkit.org/r273753>

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