Bug 209203 – [css-grid] Changes in grid or elements inside the grid affects margin on other elements in the grid

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Created attachment 393799 [details]
Test case to reproduce the issue

Margin should not be affected when other elements' content are modified in the grid.

It seems that we are not recalculating correctly the auto margins when the grid's size changes.
Apparently, the root case is that the grid item's margins are not cleared when their container is laid out again, due to a size (width in this case) change.

[Javier Fernandez]

Created attachment 393800 [details]
Patch

[Darin Adler]

Comment on attachment 393800 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1178
> +    LayoutUnit marginLogicalWidth;

How does this patch work? We compute this value, but never use it.

> Source/WebCore/rendering/RenderGrid.cpp:1209
> +    LayoutUnit marginLogicalHeight;

Same here.

[Darin Adler]

Comment on attachment 393800 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:1178
>> +    LayoutUnit marginLogicalWidth;
> 
> How does this patch work? We compute this value, but never use it.

There must be something wrong with the test, too, since it’s passing without any behavior change.

[Javier Fernandez]

Created attachment 393969 [details]
Patch

[Javier Fernandez]

Comment on attachment 393800 [details]
Patch

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

>>> Source/WebCore/rendering/RenderGrid.cpp:1178
>>> +    LayoutUnit marginLogicalWidth;
>> 
>> How does this patch work? We compute this value, but never use it.
> 
> There must be something wrong with the test, too, since it’s passing without any behavior change.

Wow, I'm so sorry about that. I must have submitted an old version of the patch, which even had test failures in the expected.txt files. 

As a matter of fact, I applied some changes in the tests to ensure the test-harness checks are done after the Ahem web-font is loaded. I have submitted a PR [1] for the WPT github repo as well with these changes.

[1] https://github.com/web-platform-tests/wpt/pull/22332

>> Source/WebCore/rendering/RenderGrid.cpp:1209
>> +    LayoutUnit marginLogicalHeight;
> 
> Same here.

yeah, already submitted a patch to fix this.

[EWS]

Committed r258735: <https://trac.webkit.org/changeset/258735>

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

[Radar WebKit Bug Importer]

<rdar://problem/60655776>