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

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?

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.

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.

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

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.

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>

All reviewed patches have been landed. Closing bug.