Bug 105260 - table-caption stride over pages.
Summary: table-caption stride over pages.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-17 23:46 PST by Yuki Sekiguchi
Modified: 2016-05-24 21:36 PDT (History)
10 users (show)

See Also:


Attachments
Reproduced content (278 bytes, text/html)
2012-12-17 23:46 PST, Yuki Sekiguchi
no flags Details
Patch (4.10 KB, patch)
2012-12-17 23:59 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2013-02-12 00:51 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2013-03-01 01:19 PST, Yuki Sekiguchi
beidson: review-
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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