Bug 221671 - Take box-sizing into account in replaced element intrinsic sizing
Summary: Take box-sizing into account in replaced element intrinsic sizing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 47738
  Show dependency treegraph
 
Reported: 2021-02-10 05:53 PST by Rob Buis
Modified: 2021-03-02 12:23 PST (History)
11 users (show)

See Also:


Attachments
Patch (6.29 KB, patch)
2021-02-10 05:55 PST, Rob Buis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2021-02-10 09:11 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.56 KB, patch)
2021-02-15 01:16 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.46 KB, patch)
2021-02-16 02:16 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.76 KB, patch)
2021-02-17 07:34 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.07 KB, patch)
2021-02-22 10:54 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.73 KB, patch)
2021-02-23 03:34 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.69 KB, patch)
2021-02-25 22:53 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.79 KB, patch)
2021-03-01 22:41 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.76 KB, patch)
2021-03-02 07:27 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2021-02-10 05:53:33 PST
Take into account box-sizing in replaced element intrinsic sizing
Comment 1 Rob Buis 2021-02-10 05:55:42 PST
Created attachment 419838 [details]
Patch
Comment 2 Rob Buis 2021-02-10 09:11:24 PST
Created attachment 419854 [details]
Patch
Comment 3 Rob Buis 2021-02-15 01:16:56 PST
Created attachment 420278 [details]
Patch
Comment 4 EWS Watchlist 2021-02-15 01:17:48 PST
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 5 Rob Buis 2021-02-16 02:16:52 PST
Created attachment 420441 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2021-02-17 05:54:13 PST
<rdar://problem/74431153>
Comment 7 Rob Buis 2021-02-17 07:34:09 PST
Created attachment 420643 [details]
Patch
Comment 8 Simon Fraser (smfr) 2021-02-22 10:13:39 PST
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()?
Comment 9 Rob Buis 2021-02-22 10:54:37 PST
Created attachment 421212 [details]
Patch
Comment 10 Rob Buis 2021-02-23 03:34:01 PST
Created attachment 421298 [details]
Patch
Comment 11 Rob Buis 2021-02-23 11:17:40 PST
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.
Comment 12 Simon Fraser (smfr) 2021-02-25 14:44:53 PST
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()?
Comment 13 Darin Adler 2021-02-25 15:19:09 PST
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?
Comment 14 Simon Fraser (smfr) 2021-02-25 15:25:21 PST
(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.
Comment 15 Rob Buis 2021-02-25 22:49:24 PST
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
Comment 16 Rob Buis 2021-02-25 22:53:05 PST
Created attachment 421613 [details]
Patch
Comment 17 Rob Buis 2021-02-26 01:04:10 PST
Comment on attachment 421613 [details]
Patch

Looks like we do not need roundToInt.
Comment 18 Simon Fraser (smfr) 2021-03-01 15:20:36 PST
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()?
Comment 19 Rob Buis 2021-03-01 22:41:28 PST
Created attachment 421906 [details]
Patch
Comment 20 Rob Buis 2021-03-01 22:56:23 PST
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.
Comment 21 Rob Buis 2021-03-02 07:27:32 PST
Created attachment 421932 [details]
Patch
Comment 22 Rob Buis 2021-03-02 08:18:54 PST
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.
Comment 23 EWS 2021-03-02 12:23:55 PST
Committed r273753: <https://commits.webkit.org/r273753>

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