Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196413002:
Don't allow position:sticky for table columns. (Closed)
|
Created:
6 years, 9 months ago by ostap Modified:
6 years, 9 months ago CC:
blink-reviews, mstensho+blink_opera.com, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr. Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionDon't allow position:sticky for table columns.
Table columns don't create RenderLayer and it causes crash in sticky offset calculation.
BUG=351390
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169149
Patch Set 1 #
Total comments: 4
Patch Set 2 : Disable position:sticky for table columns. #
Total comments: 2
Patch Set 3 : Added test. #
Total comments: 10
Patch Set 4 : Updated by review comments. #
Messages
Total messages: 20 (0 generated)
This change needs a test case (cleaned up from clusterfuzz to be more to our standards). https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTableCol.h (right): https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTableCol.h:95: if (isStickyPositioned()) columns are only style placeholder. That's the reason we never allocate a layer for them. Could you explain why you feel this is the right fix (I may be missing some context here)?
https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTableCol.h (right): https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTableCol.h:95: if (isStickyPositioned()) On 2014/03/12 03:14:42, Julien Chaffraix - PST wrote: > columns are only style placeholder. That's the reason we never allocate a layer > for them. Could you explain why you feel this is the right fix (I may be missing > some context here)? I think that table column can be sticky positioned. Is it right?
https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTableCol.h (right): https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTableCol.h:95: if (isStickyPositioned()) On 2014/03/12 05:26:24, ostap wrote: > On 2014/03/12 03:14:42, Julien Chaffraix - PST wrote: > > columns are only style placeholder. That's the reason we never allocate a > layer > > for them. Could you explain why you feel this is the right fix (I may be > missing > > some context here)? > > I think that table column can be sticky positioned. Is it right? Yes, it can but that doesn't mean that we should allow it! It wouldn't be the first time we adjust the style to prevent some situation we don't want from happening (see StyleAdjuster). Back on the current change, I was asking why you took this approach because for me it doesn't feel like the right fix. We disable RenderLayers on <col> because they make no sense. On top of that, we don't have a solid answer whether sticky requires a RenderLayer (e.g. position:relative doesn't)
https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTableCol.h (right): https://codereview.chromium.org/196413002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTableCol.h:95: if (isStickyPositioned()) On 2014/03/12 15:17:04, Julien Chaffraix - PST wrote: > > I think that table column can be sticky positioned. Is it right? > Yes, it can but that doesn't mean that we should allow it! It wouldn't be the > first time we adjust the style to prevent some situation we don't want from > happening (see StyleAdjuster). Ok. I'll disable sticky support for table elements that don't create RenderLayer. > Back on the current change, I was asking why you took this approach because for > me it doesn't feel like the right fix. We disable RenderLayers on <col> because > they make no sense. On top of that, we don't have a solid answer whether sticky > requires a RenderLayer (e.g. position:relative doesn't) I think because relative positioned elements don't change their offset dynamically, but sticky do change based on scroll position of the related scrollbox.
The code change is reasonable but you still need a test case and this shouldn't get in without one. https://codereview.chromium.org/196413002/diff/20001/Source/core/css/resolver... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/196413002/diff/20001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:272: && style->position() == StickyPosition)) I would rather have that as a separate branch below. That would make the code more readable.
On 2014/03/12 18:41:38, Julien Chaffraix - PST wrote: > The code change is reasonable but you still need a test case and this shouldn't > get in without one. Done https://codereview.chromium.org/196413002/diff/20001/Source/core/css/resolver... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/196413002/diff/20001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:272: && style->position() == StickyPosition)) On 2014/03/12 18:41:38, Julien Chaffraix - PST wrote: > I would rather have that as a separate branch below. That would make the code > more readable. Done.
lgtm https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... File LayoutTests/fast/css/sticky/sticky-table-col-crash-expected.html (right): https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... LayoutTests/fast/css/sticky/sticky-table-col-crash-expected.html:5: <p>This test should not crash.</p> Next patch should use: git cl upload --no-find-copies https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... File LayoutTests/fast/css/sticky/sticky-table-col-crash.html (right): https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... LayoutTests/fast/css/sticky/sticky-table-col-crash.html:14: <p>This test should not crash.</p> Let's add a description of what you are testing as this is very focused test. https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:263: // After performing the display mutation, check table rows. We do not honor position:relativetable rows or cells. Nit: Space after the colon. https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:271: // Do not allow position:sticky for table columns and column groups because table columns Same nit. https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:272: // don't have RenderLayer I don't think this is a good explanation. A better one is that we can't support position: sticky with our current code as we only do background painting through columns / column groups. The fact that we don't want a RenderLayer is a consequence of that, not a cause.
https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... File LayoutTests/fast/css/sticky/sticky-table-col-crash-expected.html (right): https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... LayoutTests/fast/css/sticky/sticky-table-col-crash-expected.html:5: <p>This test should not crash.</p> On 2014/03/13 01:08:06, Julien Chaffraix - PST wrote: > Next patch should use: > > git cl upload --no-find-copies Done. https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... File LayoutTests/fast/css/sticky/sticky-table-col-crash.html (right): https://codereview.chromium.org/196413002/diff/40001/LayoutTests/fast/css/sti... LayoutTests/fast/css/sticky/sticky-table-col-crash.html:14: <p>This test should not crash.</p> On 2014/03/13 01:08:06, Julien Chaffraix - PST wrote: > Let's add a description of what you are testing as this is very focused test. Done. https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:263: // After performing the display mutation, check table rows. We do not honor position:relativetable rows or cells. On 2014/03/13 01:08:06, Julien Chaffraix - PST wrote: > Nit: Space after the colon. Done. https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:271: // Do not allow position:sticky for table columns and column groups because table columns On 2014/03/13 01:08:06, Julien Chaffraix - PST wrote: > Same nit. Done. https://codereview.chromium.org/196413002/diff/40001/Source/core/css/resolver... Source/core/css/resolver/StyleAdjuster.cpp:272: // don't have RenderLayer On 2014/03/13 01:08:06, Julien Chaffraix - PST wrote: > I don't think this is a good explanation. A better one is that we can't support > position: sticky with our current code as we only do background painting through > columns / column groups. The fact that we don't want a RenderLayer is a > consequence of that, not a cause. Done.
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/196413002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_blink_compile
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/196413002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_blink_compile
The CQ bit was checked by sl.ostapenko@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/196413002/60001
Message was sent while issue was closed.
Change committed as 169149 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
