Bug 82617 – prevent page breaks in table rows

• Component: Printing
• Status: NEW

[Milian Wolff]

Created attachment 134584 [details]
test file (html) that, when printed, produces bad results

Currently, printing long tables produces bad results, with page breaks in the middle of table rows. this e.g. splits text on two pages making it unreadable.

I'll attach a test .html file as well as a generated print-to-file PDF that shows this issue. Furthermore, I will attach a patch, including a unit test, that resolves this issue.

I've build (qt)-webkit from source at f7ec460464a95c7a35d633188587ccfaa0499aca from git://gitorious.org/webkit/webkit.git

[Milian Wolff]

Created attachment 134585 [details]
status-quo of printing above test.html file to pdf using the QtTestBrowser

notice how row #47 is split over two pages. while the text is readable in this case, sometimes it is split as well (I think it's somewhat noticeable at the end of page 1).

[Milian Wolff]

Created attachment 134587 [details]
improved printing of the long table, again created using QtTestBrowser

notice how row 47 is nicely put to the next page, resolving the former issue. Apparently though, there is still an issue with the calculation of the row height, as can be seen by the slightly larger height of row 47. This is odd, since my unit test checks that and does to find any such issue.

[Milian Wolff]

Created attachment 134589 [details]
patch that improves the printing of long tables by moving table rows to the next page if they would incur a page break

patch with unit test that improves the situation (see attached after.pdf). the unit test requires new api that needs to be implemented for the other providers, this patch only adds it for Qt.

[Dave Hyatt]

This is not the right approach to fixing this issue. There are plenty of layouts where breaking on the cells inside a row is actually desirable, and shifting the whole row to the next page would create a bunch of blank space.

Have you investigated what is going wrong in the table code that led to the bad pagination in the first place?

[WebKit Review Bot]

Attachment 134589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/printing/resources/paged-media..." exit_code: 1
Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h:148:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134589 [details]
patch that improves the printing of long tables by moving table rows to the next page if they would incur a page break

Attachment 134589 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12253307

[WebKit Review Bot]

Comment on attachment 134589 [details]
patch that improves the printing of long tables by moving table rows to the next page if they would incur a page break

Attachment 134589 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12265283

[Dave Hyatt]

The poor pagination here has nothing to do with table rows. This is just another example of the "border/padding should be connected to the first bit of content and follow it to the next page" issue. We have a bug open on this issue already. I'll try to find it.

[Dave Hyatt]

Leaving more notes for myself.... the basic issue is we need to add support for pagination struts to tables. The pagination strut propagation gets stopped at the table cell, and we need to propagate it outwards and then use that as a means of pushing cells themselves (I think it's better to have struts pushed per-cell, even if it means cells inside a row have varying heights and alignments of the cell borders.)

Here's the relevant snippet from RenderBlock.cpp:

if (paginationStrut) {
        // We are willing to propagate out to our parent block as long as we were at the top of the block prior
        // to collapsing our margins, and as long as we didn't clear or move as a result of other pagination.
        if (atBeforeSideOfBlock && oldTop == result && !isPositioned() && !isTableCell()) {

Note the !isTableCell() check. We need to remove this and get struts properly stored for cells and then patch table layout to handle what to do when a cell has a strut (probably in RenderTableSection::layoutRows).

[Milian Wolff]

Hey there,

sorry for the long absence. OK, so I take it that my approach is completely wrong? Too bad :-/ What about "page-break-inside:avoid" on table rows/cells, how would that be handled? Could we maybe re-use my code for that?

Regarding the pagination struts: Is there any documentation to figure out what "properly stored for cells" would mean? Or if you could explain it some more I could try to come up with a patch for this, but I'm now reluctant to do so considering that my previous approach was apparently totally wrong. You can also hit me up on #webkit (milian) if you'd like to chat about it there.

Thanks

[Milian Wolff]

Oh and one more thing: I've used my patch as a basis to come up with a patch that would repaint thead + tfoot on every page a table spans (bug 17205). How would that be doable without preventing page breaks inside table rows?

[Milian Wolff]

Created attachment 136454 [details]
test file (html) that, when printed, produces bad results

better version of the test file. when printed, the result shows the issue more prominently than before

[Milian Wolff]

Created attachment 136455 [details]
status-quo of printing above test.html file to pdf using the QtTestBrowser

updated version of the "print to file" version of test.html with QtTestBrowser

note the horrible state of row #47 which is split in between

[Milian Wolff]

maybe related to: https://bugs.webkit.org/show_bug.cgi?id=85377