Bug 103677 – [CSS Grid Layout] Support percentage paddings and margins on grid items

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Following bug 102968, we should be properly resolving percentage and calc() on width / height but no effort was made to handle padding and margin which also accepts a <percentage>.

[Julien Chaffraix]

Created attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

[Tony Chang]

Comment on attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:465
> +    // Sticky positioned element ignore any override logical width on the containing block (as they don't call
> +    // containingBlockLogicalWidthForContent). It's unclear whether this is totally fine.

Hmm, can we create a test for this?  I guess a sticky div inside a table or a flexbox or a grid should trigger this, right?

[Julien Chaffraix]

Comment on attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

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

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:465
>> +    // containingBlockLogicalWidthForContent). It's unclear whether this is totally fine.
> 
> Hmm, can we create a test for this?  I guess a sticky div inside a table or a flexbox or a grid should trigger this, right?

Yes, we should be able to reproduce it with a grid only (the other cases won't work as they don't use the override containing block logic). The grid should be the parent *and* containing block of the sticky element so that we apply the override though.

I should also throw in some positioned elements for good measure.

[Julien Chaffraix]

Comment on attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

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

>>> Source/WebCore/rendering/RenderBoxModelObject.cpp:465
>>> +    // containingBlockLogicalWidthForContent). It's unclear whether this is totally fine.
>> 
>> Hmm, can we create a test for this?  I guess a sticky div inside a table or a flexbox or a grid should trigger this, right?
> 
> Yes, we should be able to reproduce it with a grid only (the other cases won't work as they don't use the override containing block logic). The grid should be the parent *and* containing block of the sticky element so that we apply the override though.
> 
> I should also throw in some positioned elements for good measure.

OK, I have spend more time playing with that:
* Our support of positioned element doesn't make sense currently but sadly follows the spec, I proposed a spec change (http://lists.w3.org/Archives/Public/www-style/2012Nov/0546.html). This would be better postponed to a follow-up bug.
* It looks like position: sticky doesn't work inside grid or an inline grid (I couldn't get it to work inside a display: block container either). It's maybe related to bug 100054 but I am not totally sure. Regardless, it seems that we should fix the broader issue of making it work in grid before being able to test this change.

[Tony Chang]

Comment on attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

OK

[Julien Chaffraix]

Comment on attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

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

>>>> Source/WebCore/rendering/RenderBoxModelObject.cpp:465
>>>> +    // containingBlockLogicalWidthForContent). It's unclear whether this is totally fine.
>>> 
>>> Hmm, can we create a test for this?  I guess a sticky div inside a table or a flexbox or a grid should trigger this, right?
>> 
>> Yes, we should be able to reproduce it with a grid only (the other cases won't work as they don't use the override containing block logic). The grid should be the parent *and* containing block of the sticky element so that we apply the override though.
>> 
>> I should also throw in some positioned elements for good measure.
> 
> OK, I have spend more time playing with that:
> * Our support of positioned element doesn't make sense currently but sadly follows the spec, I proposed a spec change (http://lists.w3.org/Archives/Public/www-style/2012Nov/0546.html). This would be better postponed to a follow-up bug.
> * It looks like position: sticky doesn't work inside grid or an inline grid (I couldn't get it to work inside a display: block container either). It's maybe related to bug 100054 but I am not totally sure. Regardless, it seems that we should fix the broader issue of making it work in grid before being able to test this change.

Filed bug 103956 for the first point and bug 103957 for the second.

[WebKit Review Bot]

Comment on attachment 176841 [details]
Proposed change: fix more code path to use containingBlockLogicalWidthForContent and fix end margin computation for grid items.

Clearing flags on attachment: 176841

Committed r136465: <http://trac.webkit.org/changeset/136465>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.