Bug 183933 – [css-grid] Fix auto repeat tracks computation with definite min sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Fix auto repeat tracks computation with definite min sizes

[Sergio Villar Senin]

Created attachment 336367 [details]
Patch

[Javier Fernandez]

Comment on attachment 336367 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:516
> +    if (needsToFulfillMinimumSize && freeSpace)

Is this clause correct if freeSpace is < 0 ?

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-auto-repeat-001.html:18
> +    justify-content: start;

nit: just a suggestion, you can use the contentStart class defined in the css/support/alignment.css file.

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/support/grid.css:1
> +.grid {

pretty bad that we have to duplicated this file again, but I guess there is nothing we can do about for now.

[Sergio Villar Senin]

Comment on attachment 336367 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:516
>> +    if (needsToFulfillMinimumSize && freeSpace)
> 
> Is this clause correct if freeSpace is < 0 ?

It's guaranteed that freeSpace is >=0. There is a previous early return for freeSpace <= 0. So when computing the repetitions we know that freeSpace is positive.

Then we divide freeSpace by autoRepeatSizeWithGap (we can skip the "1 +" because we do a "- 1" in the next instruction to compute the freeSpace) which gives us a number of repetitions >=0. As you can see we use autoRepeatSizeWithGap later for substracting the consumed space from freeSpace. If the previous division is exact then we would end up with 0 freeSpace. Otherwise if the division generates some remainder then we know that the amount we substract from freeSpace is less than the available freeSpace.

Hope I have explained it properly :)

>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-auto-repeat-001.html:18
>> +    justify-content: start;
> 
> nit: just a suggestion, you can use the contentStart class defined in the css/support/alignment.css file.

Right. If you don't mind I'd leave this as is because otherwise I'd have to provide a patch to wpt first and then reimport it for this patch.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/support/grid.css:1
>> +.grid {
> 
> pretty bad that we have to duplicated this file again, but I guess there is nothing we can do about for now.

Not sure what you mean, that file was already in WPT, it's just that the import pulled it in.

[Javier Fernandez]

Comment on attachment 336367 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:510
> +    unsigned repetitions = 1 + (freeSpace / autoRepeatSizeWithGap).toInt();

Are you sure this is correct ? Converting a LayoutUnit to int is what we want here ?

> Source/WebCore/rendering/RenderGrid.cpp:511
> +    freeSpace -= autoRepeatSizeWithGap * (repetitions - 1);

Umm, I didn't realize before, but mixing here LayoutUnit and unsigned could lead to an incorrect result.

>>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/support/grid.css:1
>>> +.grid {
>> 
>> pretty bad that we have to duplicated this file again, but I guess there is nothing we can do about for now.
> 
> Not sure what you mean, that file was already in WPT, it's just that the import pulled it in.

Yes, you're right.

[Sergio Villar Senin]

Created attachment 336377 [details]
Patch

Added an ASSERT and using toUnsigned to avoid an integer->unsigned conversion

[WebKit Commit Bot]

Comment on attachment 336377 [details]
Patch

Clearing flags on attachment: 336377

Committed r229897: <https://trac.webkit.org/changeset/229897>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38796366>