Bug 220143 – Take aspect-ratio into account for percentage resolution

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

[Rob Buis]

Take aspect-ratio into account for percentage resolution

[Rob Buis]

Created attachment 416750 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/72755718>

[Rob Buis]

Created attachment 417203 [details]
Patch

[Darin Adler]

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?

[Rob Buis]

Created attachment 417249 [details]
Patch

[Rob Buis]

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.

[EWS]

Found 1 new test failure: media/controls/showControlsButton.html

[EWS]

Committed r271293: <https://trac.webkit.org/changeset/271293>

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

[Darin Adler]

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?

[Rob Buis]

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.

[Darin Adler]

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.

[Rob Buis]

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!

[Darin Adler]

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.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 417400 [details]
Patch

[Darin Adler]

Comment on attachment 417400 [details]
Patch

I think this is better. Not 100% sure, but thank you for doing it.

[EWS]

Committed r271375: <https://trac.webkit.org/changeset/271375>

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