|
|
Created:
6 years ago by dsinclair Modified:
6 years ago CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionLayout captions in simplifiedNormalFlowLayout for tables.
It is currently possible to get into simplifiedNormalFlowLayout for a table
while the captions need layout. Currently the simplified code will just
iterate over all of the table sections and lay them out if needed.
This CL adds iteration over the table captions as well and makes sure they're
also laid out during simplifedNormalFlowLayout.
BUG=414109
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187509
Patch Set 1 #
Total comments: 13
Patch Set 2 : Review feedback #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 20 (4 generated)
dsinclair@chromium.org changed reviewers: + jchaffraix@chromium.org
PTAL.
Drive-by comments. Not adding myself as reviewer. I'll leave reviewing to Julien, once he enters the stage. :) https://codereview.chromium.org/804383002/diff/1/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/TestExpectations... LayoutTests/TestExpectations:1100: crbug.com/414109 fast/css/display-table-caption.html [ NeedsRebaseline ] Couldn't you just generate the expectation file as part of this CL? https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... File LayoutTests/fast/css/display-table-caption.html (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... LayoutTests/fast/css/display-table-caption.html:1: <!DOCTYPE html> I think the test would sit better in LayoutTests/fast/table/ https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... LayoutTests/fast/css/display-table-caption.html:14: <option> This test can be simplified quite a bit. Like using something less weird than the OPTION element, but perhaps you wanted to be evil. :) https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) It may not matter much (since I assume and really hope that the children's position remain untouched during simplified layout), but why not lay out the table children in tree order (regardless of being captions or header/body/footer sections), like in RenderTable::layout()?
https://codereview.chromium.org/804383002/diff/1/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/TestExpectations... LayoutTests/TestExpectations:1100: crbug.com/414109 fast/css/display-table-caption.html [ NeedsRebaseline ] On 2014/12/16 15:27:01, mstensho wrote: > Couldn't you just generate the expectation file as part of this CL? Done. Force of habit. https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... File LayoutTests/fast/css/display-table-caption.html (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... LayoutTests/fast/css/display-table-caption.html:1: <!DOCTYPE html> On 2014/12/16 15:27:01, mstensho wrote: > I think the test would sit better in LayoutTests/fast/table/ Done. https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... LayoutTests/fast/css/display-table-caption.html:14: <option> On 2014/12/16 15:27:01, mstensho wrote: > This test can be simplified quite a bit. Like using something less weird than > the OPTION element, but perhaps you wanted to be evil. :) This is a reduction from a CRASH, so I'd like to keep it as close to the original as possible. I've reduced as much out of here as I can but have it still crash. https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 15:27:01, mstensho wrote: > It may not matter much (since I assume and really hope that the children's > position remain untouched during simplified layout), but why not lay out the > table children in tree order (regardless of being captions or header/body/footer > sections), like in RenderTable::layout()? Filed crbug.com/442737 and added a comment. I'd like to keep this CL about fixing the crash, we can do the refactoring later to make it nicer. That sound good?
https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... File LayoutTests/fast/css/display-table-caption.html (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display... LayoutTests/fast/css/display-table-caption.html:14: <option> On 2014/12/16 15:40:32, dsinclair wrote: > On 2014/12/16 15:27:01, mstensho wrote: > > This test can be simplified quite a bit. Like using something less weird than > > the OPTION element, but perhaps you wanted to be evil. :) > > This is a reduction from a CRASH, so I'd like to keep it as close to the > original as possible. I've reduced as much out of here as I can but have it > still crash. Acknowledged. https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 15:40:32, dsinclair wrote: > On 2014/12/16 15:27:01, mstensho wrote: > > It may not matter much (since I assume and really hope that the children's > > position remain untouched during simplified layout), but why not lay out the > > table children in tree order (regardless of being captions or > header/body/footer > > sections), like in RenderTable::layout()? > > Filed crbug.com/442737 and added a comment. I'd like to keep this CL about > fixing the crash, we can do the refactoring later to make it nicer. That sound > good? Acknowledged.
jchaffraix@chromium.org changed reviewers: + mortens@opera.com
Morten, you should feel free to review patches. We all started with informal reviews and more feedback is better than less. https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 15:43:31, mstensho wrote: > On 2014/12/16 15:40:32, dsinclair wrote: > > On 2014/12/16 15:27:01, mstensho wrote: > > > It may not matter much (since I assume and really hope that the children's > > > position remain untouched during simplified layout), but why not lay out the > > > table children in tree order (regardless of being captions or > > header/body/footer > > > sections), like in RenderTable::layout()? > > > > Filed crbug.com/442737 and added a comment. I'd like to keep this CL about > > fixing the crash, we can do the refactoring later to make it nicer. That sound > > good? > > Acknowledged. Captions needs to be laid out first so that we can stack them on top of each other (at least for those that are at the top) so I am not sure this is such a great idea. https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... File LayoutTests/fast/table/display-table-caption-crash.html (right): https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... LayoutTests/fast/table/display-table-caption-crash.html:11: .c4:nth-of-type(2n+1) { counter-increment: c 578; } Nit: It's preferred to give meaningful name for classes (especially when they come from ClusterFuzz). https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... LayoutTests/fast/table/display-table-caption-crash.html:17: You have a gazillion unneeded empty lines in your test. While some spaces is welcome, this is past the required amount. https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... LayoutTests/fast/table/display-table-caption-crash.html:30: runAfterDisplay(function() { That's not needed AFAICT. Forcing a synchronous layout should work. It would also remove the need for waitUntilDone / notifyDone. https://codereview.chromium.org/804383002/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderTable.cpp:393: caption->layoutIfNeeded(); You're laying out the captions but nowhere are you placing them (see RenderTable::layoutCaption()), which seems wrong. It's possible that under normal flow layout, we don't expect the captions to change size (in which case, we should ASSERT it).
jchaffraix@chromium.org changed reviewers: + mstensho@opera.com - mortens@opera.com
+the right Morten :)
https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 16:05:53, Julien Chaffraix - PST wrote: > On 2014/12/16 15:43:31, mstensho wrote: > > On 2014/12/16 15:40:32, dsinclair wrote: > > > On 2014/12/16 15:27:01, mstensho wrote: > > > > It may not matter much (since I assume and really hope that the children's > > > > position remain untouched during simplified layout), but why not lay out > the > > > > table children in tree order (regardless of being captions or > > > header/body/footer > > > > sections), like in RenderTable::layout()? > > > > > > Filed crbug.com/442737 and added a comment. I'd like to keep this CL about > > > fixing the crash, we can do the refactoring later to make it nicer. That > sound > > > good? > > > > Acknowledged. > > Captions needs to be laid out first so that we can stack them on top of each > other (at least for those that are at the top) so I am not sure this is such a > great idea. So, you think this should be removed? Or should we discuss it in the bug and remove later if desired? https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... File LayoutTests/fast/table/display-table-caption-crash.html (right): https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... LayoutTests/fast/table/display-table-caption-crash.html:11: .c4:nth-of-type(2n+1) { counter-increment: c 578; } On 2014/12/16 16:05:53, Julien Chaffraix - PST wrote: > Nit: It's preferred to give meaningful name for classes (especially when they > come from ClusterFuzz). Done. https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... LayoutTests/fast/table/display-table-caption-crash.html:17: On 2014/12/16 16:05:53, Julien Chaffraix - PST wrote: > You have a gazillion unneeded empty lines in your test. While some spaces is > welcome, this is past the required amount. Done. https://codereview.chromium.org/804383002/diff/20001/LayoutTests/fast/table/d... LayoutTests/fast/table/display-table-caption-crash.html:30: runAfterDisplay(function() { On 2014/12/16 16:05:53, Julien Chaffraix - PST wrote: > That's not needed AFAICT. Forcing a synchronous layout should work. It would > also remove the need for waitUntilDone / notifyDone. Done. https://codereview.chromium.org/804383002/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderTable.cpp:393: caption->layoutIfNeeded(); On 2014/12/16 16:05:53, Julien Chaffraix - PST wrote: > You're laying out the captions but nowhere are you placing them (see > RenderTable::layoutCaption()), which seems wrong. It's possible that under > normal flow layout, we don't expect the captions to change size (in which case, > we should ASSERT it). Done.
https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 16:05:53, Julien Chaffraix - OOO til 1-1 wrote: > On 2014/12/16 15:43:31, mstensho wrote: > > On 2014/12/16 15:40:32, dsinclair wrote: > > > On 2014/12/16 15:27:01, mstensho wrote: > > > > It may not matter much (since I assume and really hope that the children's > > > > position remain untouched during simplified layout), but why not lay out > the > > > > table children in tree order (regardless of being captions or > > > header/body/footer > > > > sections), like in RenderTable::layout()? > > > > > > Filed crbug.com/442737 and added a comment. I'd like to keep this CL about > > > fixing the crash, we can do the refactoring later to make it nicer. That > sound > > > good? > > > > Acknowledged. > > Captions needs to be laid out first so that we can stack them on top of each > other (at least for those that are at the top) so I am not sure this is such a > great idea. Isn't it normally the parent that positions the child? In the simplified layout case, I was assuming that no repositioning was going to take place. After all, nobody has been bothering to reposition the sections (the code right below). Same in RenderBlock::simplifiedNormalFlowLayout() - no repositioning. I think the current proposed change is fine, but I suggested that they be laid out in the same order as in layout(), just for the sake of consistency. Not that I really think it would matter, behavior-wise. Considering that for a separate CL is probably the right thing, though.
I took a look at turning this into a check-layout.js test but ran into a couple issues. When running content_shell we will get the same offset for the LI as current Chrome does. For some reason, running with dump-render-tree we get a 2 pixel difference in the y-axis. Firefox has a different offset value from Chrome. So, it's possible we're positioning wrong, but it seems like an existing issue if we are. I'd like to land this to fix the crash. We can file a new bug to look at if we're positioning things correctly if desired.
https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderTable.cpp:393: layoutCaption(*caption); This would become the only place in the engine where we actually attempt to reposition anything in-flow during simplifiedNormalFlowLayout(). The simplifiedNormalFlowLayout() call site has this comment: // Lay out positioned descendants or objects that just need to recompute overflow. What you used to have here - caption->layoutIfNeeded() - is the right thing. simplifiedLayout is only used when you need to drill down to containing blocks of abspos descendants that need layout - no? <!DOCTYPE html> <table cellspacing="0" cellpadding="0"> <caption> <div style="position:relative; height:2em;"> <div id="elm" style="position:absolute; left:0;">above</div> </div> </caption> <tr> <td>below</td> </tr> </table> <script> onclick = function () { document.getElementById('elm').style.left = '1px'; } </script> If you click, "above" will jump below "below".
On 2014/12/17 19:03:07, mstensho wrote: > https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/Re... > File Source/core/rendering/RenderTable.cpp (right): > > https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/Re... > Source/core/rendering/RenderTable.cpp:393: layoutCaption(*caption); > This would become the only place in the engine where we actually attempt to > reposition anything in-flow during simplifiedNormalFlowLayout(). > > The simplifiedNormalFlowLayout() call site has this comment: > // Lay out positioned descendants or objects that just need to recompute > overflow. > > What you used to have here - caption->layoutIfNeeded() - is the right thing. > > simplifiedLayout is only used when you need to drill down to containing blocks > of abspos descendants that need layout - no? > > <!DOCTYPE html> > <table cellspacing="0" cellpadding="0"> > <caption> > <div style="position:relative; height:2em;"> > <div id="elm" style="position:absolute; left:0;">above</div> > </div> > </caption> > <tr> > <td>below</td> > </tr> > </table> > <script> > onclick = function () { document.getElementById('elm').style.left = '1px'; } > </script> > > If you click, "above" will jump below "below". You're right. I've added your example above as a ref-test and switched back to just calling layoutIfNeeded.
https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... File LayoutTests/fast/table/caption-position-expected.html (right): https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... LayoutTests/fast/table/caption-position-expected.html:1: Test passes is above is on top. Should have <!DOCTYPE html> https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... File LayoutTests/fast/table/caption-position.html (right): https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... LayoutTests/fast/table/caption-position.html:20: runAfterDisplay(function() { Instead of using the runAfterDisplay() machinery, couldn't you just do an onload = function() { document.body.offsetTop; /* force layout */ document.getElementById('elm').style.left = '1px'; } ? You'd get rid of the notifyDone() thing as well then.
https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... File LayoutTests/fast/table/caption-position-expected.html (right): https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... LayoutTests/fast/table/caption-position-expected.html:1: Test passes is above is on top. On 2014/12/17 20:36:01, mstensho wrote: > Should have <!DOCTYPE html> Doh, trimmed too much. Done. https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... File LayoutTests/fast/table/caption-position.html (right): https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/c... LayoutTests/fast/table/caption-position.html:20: runAfterDisplay(function() { On 2014/12/17 20:36:01, mstensho wrote: > Instead of using the runAfterDisplay() machinery, couldn't you just do an onload > = function() { document.body.offsetTop; /* force layout */ > document.getElementById('elm').style.left = '1px'; } ? > > You'd get rid of the notifyDone() thing as well then. I thought I'd tried that, but apparently not. Done.
lgtm
The CQ bit was checked by inferno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/804383002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187509 |