Bug 105260

Summary: table-caption stride over pages.
Product: WebKit Reporter: Yuki Sekiguchi <yuki.sekiguchi>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: ahmad.saleem792, commit-queue, eric, esprehn+autocc, glenn, hyatt, jchaffraix, kondapallykalyan, ojan.autocc, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Reproduced content
none
Patch
none
Patch
none
Patch beidson: review-, beidson: commit-queue-

Description Yuki Sekiguchi 2012-12-17 23:46:27 PST
Created attachment 179887 [details]
Reproduced content

In the attached HTML content, "caption" text in caption element stride over columns.

There should be the "caption" at the top of 2nd column.

I write content using multi-column, but printing caption element also stride over pages.
Comment 1 Yuki Sekiguchi 2012-12-17 23:59:45 PST
Created attachment 179889 [details]
Patch
Comment 2 Robert Hogan 2013-01-28 11:26:53 PST
Yuki: you should go on to IRC and ping jchaffraix to look at this. It seems OK to me.
Comment 3 Julien Chaffraix 2013-02-06 10:17:32 PST
Comment on attachment 179889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179889&action=review

The change is probably good, I just have a hard time understanding the justification.

> Source/WebCore/ChangeLog:10
> +        Fix table-caption stride over pages.
> +        RenderBlock::adjustLinePositionForPagination() don't adjust 1st line inside table-caption.
> +        Make adjustLinePositionForPagination() adjust 1st line inside table-caption.

This ChangeLog is very imprecise and doesn't describe *why* it's correct / advisable / good, which is usually the most interesting part of a change.

> LayoutTests/ChangeLog:8
> +        Test that table-caption which stride over columns should be at the top of 2nd column.

I am not familiar with the term 'stride' nor does the meaning I found explain this sentence (nor the other ChangeLog FWIW) in any meaningful way. Care to reformulate?
Comment 4 Yuki Sekiguchi 2013-02-12 00:51:10 PST
Created attachment 187800 [details]
Patch
Comment 5 Yuki Sekiguchi 2013-02-12 02:26:16 PST
Thank you for reviewing.

I change the code, because I think this code is better than the old one.

(In reply to comment #3)
> (From update of attachment 179889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179889&action=review
> 
> The change is probably good, I just have a hard time understanding the justification.
> 
> > Source/WebCore/ChangeLog:10
> > +        Fix table-caption stride over pages.
> > +        RenderBlock::adjustLinePositionForPagination() don't adjust 1st line inside table-caption.
> > +        Make adjustLinePositionForPagination() adjust 1st line inside table-caption.
> 
> This ChangeLog is very imprecise and doesn't describe *why* it's correct / advisable / good, which is usually the most interesting part of a change.

I write *why* description.

> 
> > LayoutTests/ChangeLog:8
> > +        Test that table-caption which stride over columns should be at the top of 2nd column.
> 
> I am not familiar with the term 'stride' nor does the meaning I found explain this sentence (nor the other ChangeLog FWIW) in any meaningful way. Care to reformulate?

Change a sentence.
Comment 6 Benjamin Poulain 2013-02-18 15:23:06 PST
Comment on attachment 187800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187800&action=review

> LayoutTests/fast/multicol/table-multicolumn-caption.html:8
> +<body style="font-family: ahem">
> +<div style="-webkit-column-count: 2; width: 400px; height: 100px; border: solid 3px red">
> +    <div style="width: 100px; height: 90px"></div>
> +    <table><caption>caption</caption></table>
> +</div>
> +</body>

It is useful to also describe the expected behavior in text in the test.

This way, someone can manually check results by simply reading the description and verifying the output matches.
Comment 7 Dave Hyatt 2013-02-18 15:31:20 PST
Comment on attachment 187800 [details]
Patch

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));
Comment 8 Yuki Sekiguchi 2013-03-01 01:19:23 PST
Created attachment 190906 [details]
Patch
Comment 9 Yuki Sekiguchi 2013-03-01 01:20:37 PST
(In reply to comment #6)
> (From update of attachment 187800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187800&action=review
> 
> > LayoutTests/fast/multicol/table-multicolumn-caption.html:8
> > +<body style="font-family: ahem">
> > +<div style="-webkit-column-count: 2; width: 400px; height: 100px; border: solid 3px red">
> > +    <div style="width: 100px; height: 90px"></div>
> > +    <table><caption>caption</caption></table>
> > +</div>
> > +</body>
> 
> It is useful to also describe the expected behavior in text in the test.
> 
> This way, someone can manually check results by simply reading the description and verifying the output matches.

I added expectation to test content.
Comment 10 Yuki Sekiguchi 2013-03-01 01:53:26 PST
Hi  Dave,
Thank you for reviewing.

(In reply to comment #7)
> (From update of attachment 187800 [details])
> 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));

Thank you for good advices.
I apply your suggestions.
Comment 11 Brady Eidson 2016-05-24 21:36:02 PDT
Comment on attachment 190906 [details]
Patch

r- to clear stale patches from the review queue
Comment 13 Ahmad Saleem 2023-07-04 16:56:25 PDT
RenderTable.cpp: (Simple Change) on Line 286

// Apply the margins to the location now that they are definitely available from layout
    LayoutUnit captionLogicalTop = collapsedMarginBeforeForChild(caption) + logicalHeight();
    caption.setLogicalLocation(LayoutPoint(caption.marginStart(), captionLogicalTop));

______________

RenderBlockFlow.cpp changes are bit confusing for me:

https://searchfox.org/wubkat/source/Source/WebCore/rendering/RenderBlockFlow.cpp#1900

^ from above remove '&& !isTableCell()'

Static function in Blink (shouldSetStrutOnBlock), does not exist in same shape in WebKit but it exist here in some change / form:

https://searchfox.org/wubkat/source/Source/WebCore/rendering/RenderBlockFlow.cpp#2081

______________

Have to create new bool function (allowsPaginationStrut).


_______________________

If someone can guide me on second part, I am happy to look into merging these patches. I am not clear on 'return' bit or I can create new static function but which parts to remove from the linked area.