[webkit-reviews] review denied: [Bug 105260] table-caption stride over pages. : [Attachment 187800] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Yuki Sekiguchi
<yuki.sekiguchi at access-company.com>'s request for review:
Bug 105260: table-caption stride over pages.
https://bugs.webkit.org/show_bug.cgi?id=105260

Attachment 187800: Patch
https://bugs.webkit.org/attachment.cgi?id=187800&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=187800&action=review


r-

> Source/WebCore/ChangeLog:11
> +	   When the first line block steps over pages, it sets strut to its
parent RenderBlock (called P).
> +	   When the parent RenderBlock of P layouts P, it shifts P's position
using struts of P.
> +	   This is executed at RenderBlock::adjustBlockChildForPagination() and
layoutBlockChild().
> +	   However, when a RenderTable layouts RenderCaptions, it doesn't shift
a position of RenderCaptions.

How about:

"In normal block layout, pagination struts are propagated from child blocks to
parent blocks in order to indicate that an object needs to push to the next
page. Table captions were setting a pagination strut for a push, but this strut
was not being used. Make sure RenderTable applies the caption's pagination
strut when placing the caption."

> Source/WebCore/rendering/RenderTable.cpp:358
> -    caption->setLogicalLocation(LayoutPoint(caption->marginStart(),
caption->marginBefore() + logicalHeight()));
> +    caption->setLogicalLocation(LayoutPoint(caption->marginStart(),
caption->marginBefore() + logicalHeight() + caption->paginationStrut()));

This is a good change. There are a few things you should do to keep it
consistent with blocks though.

First, we should only add in the paginationStrut when the layout state says so.
Second, you should clear out the child's pagination strut once you have
factored it into the placement.

LayoutUnit captionLogicalTop = caption->marginBefore() + logicalHeight();
if (layoutState->isPaginated()) {
    captionLogicalTop += caption->paginationStrut();
    caption->setPaginationStrut(0);
}
caption->setLogicalLocation(LayoutPoint(caption->marginStart(),
captionLogicalTop));