Issue 194713009: Make sure table cells are laid out again when fragmentainer height changes.

Issue 194713009: Make sure table cells are laid out again when fragmentainer height changes. (Closed)

Created:
6 years, 9 months ago by mstensho (USE GERRIT)

Modified:
6 years, 9 months ago

Reviewers:
leviw_travelin_and_unemployed, Julien - ping for review, ojan

CC:
blink-reviews, mstensho+blink_opera.com, zoltan1, dsinclair, bemjb+rendering_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., eseidel

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Make sure table cells are laid out again when fragmentainer height changes. We may have to update pagination struts then, which requires a deeper layout. BUG=348064 Note that this doesn't really fix the bug mentioned, but it does contribute to more correct rendering of the test case there. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169144

Patch Set 1 #

Patch Set 2 : Use the old multicol implementation in the tests. The new one has a bug that needs to be fixed befo… #

Total comments: 12

Patch Set 3 : Address code review issues raised together with the lgtm. #

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+63 lines, -10 lines)			Patch
A	LayoutTests/fast/multicol/table-cell-content-change.html	View	1 2	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/multicol/table-cell-content-change-expected.html	View	1 2	1 chunk	+15 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBox.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2	1 chunk	+9 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderTable.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderTableRow.cpp	View		1 chunk	+3 lines, -6 lines	0 comments	Download
M	Source/core/rendering/RenderTableSection.cpp	View		2 chunks	+6 lines, -3 lines	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

Julien - ping for review

lgtm https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multicol/table-cell-content-change.html File LayoutTests/fast/multicol/table-cell-content-change.html (right): https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multicol/table-cell-content-change.html#newcode4 LayoutTests/fast/multicol/table-cell-content-change.html:4: <title>Changing the contents of a table cell, increasing ...

6 years, 9 months ago (2014-03-12 15:58:22 UTC) #2

mstensho (USE GERRIT)

https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multicol/table-cell-content-change.html File LayoutTests/fast/multicol/table-cell-content-change.html (right): https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multicol/table-cell-content-change.html#newcode4 LayoutTests/fast/multicol/table-cell-content-change.html:4: <title>Changing the contents of a table cell, increasing column ...

6 years, 9 months ago (2014-03-13 08:26:47 UTC) #3

https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/table-cell-content-change.html (right):

https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/table-cell-content-change.html:4: <title>Changing the
contents of a table cell, increasing column height</title>
On 2014/03/12 15:58:23, Julien Chaffraix - PST wrote:
> Putting anything in the title is useless from DRT's perspective as we don't
dump
> it.
> 
> It's better to jut dump it into the test as it's a nice description of what
you
> are testing.

Done.

I keep thinking that my tests could be used by others than DRT, such as humans.
But yeah, it still doesn't HAVE TO be in TITLE.

https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/table-cell-content-change.html:15: <div
style="-webkit-columns:2; columns:2; x-webkit-column-rule:1px solid;
xcolumn-rule:1px solid; orphans:1; widows:1; xbackground:yellow;"> <!-- FIXME:
should ideally paint background and column rules here, to measure the column
height, but the balancing machinery in the old multicol implementation doesn't
do it properly. -->
On 2014/03/12 15:58:23, Julien Chaffraix - PST wrote:
> Can we link to bugs about fixing these bits. Putting a FIXME in a test is more
> or less dropping the ball.

Done.

https://codereview.chromium.org/194713009/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/table-cell-content-change.html:16: <div
style="display:table-row;">
On 2014/03/12 15:58:23, Julien Chaffraix - PST wrote:
> Nit: Space after ":". I didn't repeat this comment for all colons in all the
> tests.

I really prefer no space after ':' when using style attributes. That way it's
easier for the eye to see separate declarations, if you have more of them.

https://codereview.chromium.org/194713009/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/194713009/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4146: void
RenderBox::markForPaginationRelayoutIfNeeded(SubtreeLayoutScope& layoutScope)
On 2014/03/12 15:58:23, Julien Chaffraix - PST wrote:
> Ideally this should be only used for table parts but we don't have a
> RenderTablePart for them.

We used to only do this for blocks (well.. RenderBlock, so you'd get table
boxes, flexboxes, grids and probably other things for free, in addition to
regular block containers).

But you need this for anything with children that can be paginated. Is that
really only table parts, in addition to block-ish boxes?

https://codereview.chromium.org/194713009/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4149: if (needsLayout())
On 2014/03/12 15:58:23, Julien Chaffraix - PST wrote:
> This is a bad pattern. Either we know the ASSERT is never reached and the if
is
> useless or we just accept the fact that it's not enforced and remove the
ASSERT.
> Here it seems to be the option 1.

Done. Indeed. I stole this code, but bad indeed. :)

https://codereview.chromium.org/194713009/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/194713009/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.cpp:462:
toRenderBox(child)->markForPaginationRelayoutIfNeeded(layouter);
On 2014/03/12 15:58:23, Julien Chaffraix - PST wrote:
> I would rather push have this moved under the isTableSection() branch below as
> this is the only case we really care about. We may miss crazy cases but it
makes
> a lot more sense IMO.

You need it for captions too. The comment in the code looks confused, BTW
("FIXME: We should never have other type of children"). For table columns, on
the other hand, marking for relayout because of pagination makes no sense.

Anyway, I think I'll just keep the code as it is for now.

mstensho (USE GERRIT)

The CQ bit was checked by mstensho@opera.com

6 years, 9 months ago (2014-03-13 08:26:57 UTC) #4