Bug 209203 - [css-grid] Changes in grid or elements inside the grid affects margin on other elements in the grid
Summary: [css-grid] Changes in grid or elements inside the grid affects margin on othe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-17 16:16 PDT by Javier Fernandez
Modified: 2020-03-19 15:28 PDT (History)
15 users (show)

See Also:


Attachments
Test case to reproduce the issue (573 bytes, text/html)
2020-03-17 16:16 PDT, Javier Fernandez
no flags Details
Patch (51.53 KB, patch)
2020-03-17 16:26 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (51.54 KB, patch)
2020-03-19 06:00 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2020-03-17 16:16:54 PDT
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.
Comment 1 Javier Fernandez 2020-03-17 16:26:16 PDT
Created attachment 393800 [details]
Patch
Comment 2 Darin Adler 2020-03-18 21:03:32 PDT
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.
Comment 3 Darin Adler 2020-03-18 21:04:11 PDT
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.
Comment 4 Javier Fernandez 2020-03-19 06:00:02 PDT
Created attachment 393969 [details]
Patch
Comment 5 Javier Fernandez 2020-03-19 06:08:51 PDT
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.
Comment 6 EWS 2020-03-19 15:27:12 PDT
Committed r258735: <https://trac.webkit.org/changeset/258735>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393969 [details].
Comment 7 Radar WebKit Bug Importer 2020-03-19 15:28:16 PDT
<rdar://problem/60655776>