UNCONFIRMED 105260
table-caption stride over pages. 
https://bugs.webkit.org/show_bug.cgi?id=105260
Summary table-caption stride over pages.
Yuki Sekiguchi
Reported 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.
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
DetailsFormatted DiffDiff
Patch (4.13 KB, patch)
2013-02-12 00:51 PST, Yuki Sekiguchi 		no flags
DetailsFormatted DiffDiff
Patch (4.31 KB, patch)
2013-03-01 01:19 PST, Yuki Sekiguchi 		beidson: review-
beidson: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yuki Sekiguchi
Comment 1 2012-12-17 23:59:45 PST
Created attachment 179889 [details] Patch
Robert Hogan
Comment 2 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.
Julien Chaffraix
Comment 3 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?
Yuki Sekiguchi
Comment 4 2013-02-12 00:51:10 PST
Created attachment 187800 [details] Patch
Yuki Sekiguchi
Comment 5 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.
Benjamin Poulain
Comment 6 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.
Dave Hyatt
Comment 7 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));
Yuki Sekiguchi
Comment 8 2013-03-01 01:19:23 PST
Created attachment 190906 [details] Patch
Yuki Sekiguchi
Comment 9 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.
Yuki Sekiguchi
Comment 10 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.
Brady Eidson
Comment 11 2016-05-24 21:36:02 PDT
Comment on attachment 190906 [details] Patch r- to clear stale patches from the review queue
Ahmad Saleem
Comment 13 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.
Note You need to log in before you can comment on or make changes to this bug.