Bug 220143 - Take aspect-ratio into account for percentage resolution
Summary: Take aspect-ratio into account for percentage resolution
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 47738
  Show dependency treegraph
 
Reported: 2020-12-24 08:12 PST by Rob Buis
Modified: 2021-01-11 13:11 PST (History)
8 users (show)

See Also:


Attachments
Patch (7.61 KB, patch)
2020-12-24 08:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2021-01-07 12:41 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2021-01-08 00:39 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.18 KB, patch)
2021-01-11 12:14 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 2020-12-24 08:12:44 PST
Take aspect-ratio into account for percentage resolution
Comment 1 Rob Buis 2020-12-24 08:14:57 PST
Created attachment 416750 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-12-31 08:13:13 PST
<rdar://problem/72755718>
Comment 3 Rob Buis 2021-01-07 12:41:12 PST
Created attachment 417203 [details]
Patch
Comment 4 Darin Adler 2021-01-07 14:13:54 PST
Comment on attachment 417203 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3310
> +        return blockSizeFromAspectRatio(horizontalBorderAndPaddingExtent(), verticalBorderAndPaddingExtent(), style().logicalAspectRatio() , style().boxSizing(), logicalWidth());

Stray space here before a ",".

> Source/WebCore/rendering/RenderBox.h:704
> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)

Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?
Comment 5 Rob Buis 2021-01-08 00:39:31 PST
Created attachment 417249 [details]
Patch
Comment 6 Rob Buis 2021-01-08 01:14:47 PST
Comment on attachment 417203 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3310
>> +        return blockSizeFromAspectRatio(horizontalBorderAndPaddingExtent(), verticalBorderAndPaddingExtent(), style().logicalAspectRatio() , style().boxSizing(), logicalWidth());
> 
> Stray space here before a ",".

Done.

>> Source/WebCore/rendering/RenderBox.h:704
>> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)
> 
> Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?

I tried this, but this causes a test failure in replaced-element-011.html, I think when doing a double -> LayoutUnit -> double conversion in RenderSVGRoot::computeIntrinsicRatioInformation.
Comment 7 EWS 2021-01-08 03:14:23 PST
Found 1 new test failure: media/controls/showControlsButton.html
Comment 8 EWS 2021-01-08 05:29:06 PST
Committed r271293: <https://trac.webkit.org/changeset/271293>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417249 [details].
Comment 9 Darin Adler 2021-01-08 10:17:01 PST
Comment on attachment 417203 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBox.h:704
>>> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)
>> 
>> Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?
> 
> I tried this, but this causes a test failure in replaced-element-011.html, I think when doing a double -> LayoutUnit -> double conversion in RenderSVGRoot::computeIntrinsicRatioInformation.

Something doesn’t make logical sense with that. This function takes a double and *always* converts it to a LayoutUnit. Doing the conversion before calling instead couldn’t possibly give a different result!

Maybe you made a bigger change, where we actually use LayoutUnit for to compute the value, rather than converting right at the point where we call?
Comment 10 Rob Buis 2021-01-09 11:56:54 PST
Comment on attachment 417203 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderBox.h:704
>>>> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)
>>> 
>>> Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?
>> 
>> I tried this, but this causes a test failure in replaced-element-011.html, I think when doing a double -> LayoutUnit -> double conversion in RenderSVGRoot::computeIntrinsicRatioInformation.
> 
> Something doesn’t make logical sense with that. This function takes a double and *always* converts it to a LayoutUnit. Doing the conversion before calling instead couldn’t possibly give a different result!
> 
> Maybe you made a bigger change, where we actually use LayoutUnit for to compute the value, rather than converting right at the point where we call?

I put some quick debug statements in logicalAspectRatio:

        auto ret = LayoutUnit(aspectRatioHeight() / aspectRatioWidth());
        fprintf(stderr, "ret %lf actual %lf\n", ret.toDouble(), aspectRatioHeight() / aspectRatioWidth());
        return ret;

For replaced-element-011.html, which uses aspect-ratio: 5/1, I see:
ret 0.187500 actual 0.200000

This must be because of clampToInteger. So unless there is another way to use LayoutUnit that keeps the double, I do not see how this can work.
Comment 11 Darin Adler 2021-01-11 10:39:52 PST
Comment on attachment 417203 [details]
Patch

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

>>>>> Source/WebCore/rendering/RenderBox.h:704
>>>>> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)
>>>> 
>>>> Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?
>>> 
>>> I tried this, but this causes a test failure in replaced-element-011.html, I think when doing a double -> LayoutUnit -> double conversion in RenderSVGRoot::computeIntrinsicRatioInformation.
>> 
>> Something doesn’t make logical sense with that. This function takes a double and *always* converts it to a LayoutUnit. Doing the conversion before calling instead couldn’t possibly give a different result!
>> 
>> Maybe you made a bigger change, where we actually use LayoutUnit for to compute the value, rather than converting right at the point where we call?
> 
> I put some quick debug statements in logicalAspectRatio:
> 
>         auto ret = LayoutUnit(aspectRatioHeight() / aspectRatioWidth());
>         fprintf(stderr, "ret %lf actual %lf\n", ret.toDouble(), aspectRatioHeight() / aspectRatioWidth());
>         return ret;
> 
> For replaced-element-011.html, which uses aspect-ratio: 5/1, I see:
> ret 0.187500 actual 0.200000
> 
> This must be because of clampToInteger. So unless there is another way to use LayoutUnit that keeps the double, I do not see how this can work.

What does this have to do with the code below? The code converts the aspectRatio double to a LayoutUnit before dong anything further with it. There’s no conversion to LayoutUnit and then back to a double.
Comment 12 Rob Buis 2021-01-11 10:56:53 PST
Comment on attachment 417203 [details]
Patch

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

>>>>>> Source/WebCore/rendering/RenderBox.h:704
>>>>>> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)
>>>>> 
>>>>> Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?
>>>> 
>>>> I tried this, but this causes a test failure in replaced-element-011.html, I think when doing a double -> LayoutUnit -> double conversion in RenderSVGRoot::computeIntrinsicRatioInformation.
>>> 
>>> Something doesn’t make logical sense with that. This function takes a double and *always* converts it to a LayoutUnit. Doing the conversion before calling instead couldn’t possibly give a different result!
>>> 
>>> Maybe you made a bigger change, where we actually use LayoutUnit for to compute the value, rather than converting right at the point where we call?
>> 
>> I put some quick debug statements in logicalAspectRatio:
>> 
>>         auto ret = LayoutUnit(aspectRatioHeight() / aspectRatioWidth());
>>         fprintf(stderr, "ret %lf actual %lf\n", ret.toDouble(), aspectRatioHeight() / aspectRatioWidth());
>>         return ret;
>> 
>> For replaced-element-011.html, which uses aspect-ratio: 5/1, I see:
>> ret 0.187500 actual 0.200000
>> 
>> This must be because of clampToInteger. So unless there is another way to use LayoutUnit that keeps the double, I do not see how this can work.
> 
> What does this have to do with the code below? The code converts the aspectRatio double to a LayoutUnit before dong anything further with it. There’s no conversion to LayoutUnit and then back to a double.

The problem is that computeIntrinsicRatioInformation ultimately wants a double, see RenderSVGRoot::computeIntrinsicRatioInformation for example.
Was your suggestion to keep logicalAspectRatio to return a double and just have blockSizeFromAspectRatio/inlineSizeFromAspectRatio take a LayoutUnit for the ratio? If that is the case I misinterpreted your suggestion!
Comment 13 Darin Adler 2021-01-11 11:06:22 PST
Comment on attachment 417203 [details]
Patch

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

>>>>>>> Source/WebCore/rendering/RenderBox.h:704
>>>>>>> +    static LayoutUnit blockSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, double aspectRatio, BoxSizing boxSizing, LayoutUnit inlineSize)
>>>>>> 
>>>>>> Since all paths convert aspectRatio into LayoutUnit, could we take the argument as a LayoutUnit instead of converting inside the function?
>>>>> 
>>>>> I tried this, but this causes a test failure in replaced-element-011.html, I think when doing a double -> LayoutUnit -> double conversion in RenderSVGRoot::computeIntrinsicRatioInformation.
>>>> 
>>>> Something doesn’t make logical sense with that. This function takes a double and *always* converts it to a LayoutUnit. Doing the conversion before calling instead couldn’t possibly give a different result!
>>>> 
>>>> Maybe you made a bigger change, where we actually use LayoutUnit for to compute the value, rather than converting right at the point where we call?
>>> 
>>> I put some quick debug statements in logicalAspectRatio:
>>> 
>>>         auto ret = LayoutUnit(aspectRatioHeight() / aspectRatioWidth());
>>>         fprintf(stderr, "ret %lf actual %lf\n", ret.toDouble(), aspectRatioHeight() / aspectRatioWidth());
>>>         return ret;
>>> 
>>> For replaced-element-011.html, which uses aspect-ratio: 5/1, I see:
>>> ret 0.187500 actual 0.200000
>>> 
>>> This must be because of clampToInteger. So unless there is another way to use LayoutUnit that keeps the double, I do not see how this can work.
>> 
>> What does this have to do with the code below? The code converts the aspectRatio double to a LayoutUnit before dong anything further with it. There’s no conversion to LayoutUnit and then back to a double.
> 
> The problem is that computeIntrinsicRatioInformation ultimately wants a double, see RenderSVGRoot::computeIntrinsicRatioInformation for example.
> Was your suggestion to keep logicalAspectRatio to return a double and just have blockSizeFromAspectRatio/inlineSizeFromAspectRatio take a LayoutUnit for the ratio? If that is the case I misinterpreted your suggestion!

> Was your suggestion to keep logicalAspectRatio to return a double and just have blockSizeFromAspectRatio/inlineSizeFromAspectRatio take a LayoutUnit for the ratio?

Yes.
Comment 14 Rob Buis 2021-01-11 12:14:55 PST
Reopening to attach new patch.
Comment 15 Rob Buis 2021-01-11 12:14:58 PST
Created attachment 417400 [details]
Patch
Comment 16 Darin Adler 2021-01-11 12:21:08 PST
Comment on attachment 417400 [details]
Patch

I think this is better. Not 100% sure, but thank you for doing it.
Comment 17 EWS 2021-01-11 13:11:43 PST
Committed r271375: <https://trac.webkit.org/changeset/271375>

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