Bug 9526 – CSS page-break properties quirk

• Component: Printing
• Status: RESOLVED FIXED

[Arjen]

In Safari(2.03) a page-break-before enclosed in an element that has an overflow other than ‘visible’ (which is the default) will not render a page-break when printed.

[David Kilzer (:ddkilzer)]

Confirmed in WebKit r14934 as well.  Changing component to Printing since that seems to match up better with the issue.

Steps to reproduce:

1. Load the URL: http://devel.thebluebus.nl/pages/20/Overflow_page-break-before_quirk.html
2. Hit Cmd-P to print.
3. Click the Preview button.

Expected results:

The web page should be printed on five pages.

Actual results:

The web page is printed on two pages; only the last style is applied when printing.

[David Kilzer (:ddkilzer)]

Created attachment 8940 [details]
Test case from web site

[Pam Greene (IRC:pamg)]

Possibly related, the following also doesn't apply the page break. (Granted that  a <div> isn't allowed within a <span>, so it might not be worth spending too much time on this; but it may provide a clue to the problem above.)

<span class="printonly">
   <hr>
   Blah blah
   <div style="page-break-after: always;"></div>
</span>

[Yuzo Fujishima]

Created attachment 46811 [details]
When page-break-{after,before} is set to always, force page breaks even for tables, floated, positioned, and/or overflow-specified elements.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderBlock.cpp:1650:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBlock.cpp:1661:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2


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

[Eric Seidel (no email)]

Comment on attachment 46811 [details]
When page-break-{after,before} is set to always, force page breaks even for tables, floated, positioned, and/or overflow-specified elements.

We need tests for this.  I believe you added some automated testing in another bug.  This would be an easy r+ if it had tests.

[Yuzo Fujishima]

Hi, Eric,

Thank you for the review.
I'll add tests once the patch for https://bugs.webkit.org/show_bug.cgi?id=33840 is landed.

Yuzo

[Yuzo Fujishima]

Created attachment 47910 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderBlock.cpp:1661:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBlock.cpp:1672:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2


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

[Yuzo Fujishima]

Hi, Eric,

I've added tests for overflow:*.

(This patch alone is not enough to fix page break behavior for tables and floats.
So I haven't added tests for them.)

Yuzo

[Eric Seidel (no email)]

It's always OK to add failing tests that we will fix later. :)  Makes the later fixing easier.

[Eric Seidel (no email)]

Comment on attachment 47910 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

Style:
 if (layoutTestController)
 21 {
 22     for (var i = 0; i < expectedPages.length; i++)
 23     {

(Yes, I know we don't have an official JS style for WebKit, but I think most people follow the c++ rules more or less.)

OK, so obviously you are asserting that inRootBlockContext is wrong, but you don't explain why in the ChangeLog.

I belive you that this is the right pathc.  I'm just don't quite understand why someone (clearly intentionally) coded this "wrong" before.  Can you please explain (ideally in the ChangeLog)?

[Shinichiro Hamaji]

Comment on attachment 47910 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

Adding a comment about the test.

> +description("Test page-break-{before,after}:always for overflow:{visible,hidden,scroll,auto} elements.");
> +
> +var overflowValues = ["visible", "hidden", "scroll", "auto"];
> +var pageBreakPositions = ["page-break-before", "page-break-after"];
> +
> +var testNumber = 0;
> +var testHtml = "";
> +for (var position = 0; position < pageBreakPositions.length; position++) {
> +    for (var value = 0; value < overflowValues.length; value++, testNumber++) {
> +        var overflowStyle = "overflow:" + overflowValues[value];
> +        var pageBreakStyle = pageBreakPositions[position] + ":always";
> +        testHtml += '<div style="' + overflowStyle + '"><p id="test' + testNumber + '" style="' + pageBreakStyle + '">' + overflowStyle + ', ' + pageBreakStyle + '</p></div>\n';

I'd use test-visible-page-break-before, test-visible-page-braek-after, ... instead of test0, test1, ... as their ID. In this way we can see the failing test easily.

> +var expectedPages = [1, 2, 3, 4, 4, 5, 6, 7, 8];

It would be better to add comments for this expectations.

Also, please fix the style issue the bot is saying.

[Yuzo Fujishima]

Created attachment 48316 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

[Yuzo Fujishima]

Hi, reviewers, can you take another look?

Eric,

- Fixed the JavaScript style.
- Explained in the change log why inRootBlockContext should be removed.

Shinichiro,

- Rewrote the test such that the test IDs and the expected values are
   self descriptive.
- Fixed the style lint error.

Yuzo

[Shinichiro Hamaji]

Comment on attachment 48316 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

The change for test looks good except for one nitpick.

> +if (layoutTestController) {

This would raise an exception when layoutTestController is not defined. Please use window.layoutTestController instead.

[Yuzo Fujishima]

Created attachment 48327 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

[Yuzo Fujishima]

Thank you for the review.

Fixed as:
  if (window.layoutTestController) {

[Yuzo Fujishima]

Ping?

[Eric Seidel (no email)]

Comment on attachment 48327 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

So how has our behavior changed when no page breaks are specified by CSS?

[Yuzo Fujishima]

The WebKit behaves the same way unless page-break-{after,before}:always is specified.

The patch doesn't take effect if
    child->style()->pageBreakBefore() == PBALWAYS
is false.

[Eric Seidel (no email)]

Comment on attachment 48327 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

OK.

[WebKit Commit Bot]

Comment on attachment 48327 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

Rejecting patch 48327 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
Queue/WebCore/rendering/SVGRootInlineBox.cpp -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/SVGRootInlineBox.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/RenderBlock.o /Users/eseidel/Projects/CommitQueue/WebCore/rendering/RenderBlock.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/288802

[Eric Seidel (no email)]

Attachment 48327 [details] was posted by a committer and has review+, assigning to Yuzo Fujishima for commit.

[Yuzo Fujishima]

Committed r55067: <http://trac.webkit.org/changeset/55067>