Bug 9526 - CSS page-break properties quirk
Summary: CSS page-break properties quirk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 417.x
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Yuzo Fujishima
URL: http://devel.thebluebus.nl/pages/20/O...
Keywords:
Depends on: 33840
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-21 02:14 PDT by Arjen
Modified: 2013-06-18 23:44 PDT (History)
10 users (show)

See Also:


Attachments
Test case from web site (4.34 KB, text/html)
2006-06-21 03:46 PDT, David Kilzer (:ddkilzer)
no flags Details
When page-break-{after,before} is set to always, force page breaks even for tables, floated, positioned, and/or overflow-specified elements. (3.29 KB, patch)
2010-01-18 04:11 PST, Yuzo Fujishima
eric: commit-queue-
Details | Formatted Diff | Diff
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements. (7.72 KB, patch)
2010-02-02 00:06 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements. (9.48 KB, patch)
2010-02-07 22:20 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements. (9.49 KB, patch)
2010-02-08 02:28 PST, Yuzo Fujishima
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arjen 2006-06-21 02:14:41 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2006-06-21 03:44:42 PDT
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.
Comment 2 David Kilzer (:ddkilzer) 2006-06-21 03:46:26 PDT
Created attachment 8940 [details]
Test case from web site
Comment 3 Pam Greene (IRC:pamg) 2009-01-06 14:18:43 PST
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>

Comment 4 Yuzo Fujishima 2010-01-18 04:11:16 PST
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.
Comment 5 WebKit Review Bot 2010-01-18 04:16:58 PST
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.
Comment 6 Eric Seidel (no email) 2010-01-26 14:16:00 PST
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.
Comment 7 Yuzo Fujishima 2010-01-26 18:15:51 PST
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
Comment 8 Yuzo Fujishima 2010-02-02 00:06:48 PST
Created attachment 47910 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.
Comment 9 WebKit Review Bot 2010-02-02 00:12:33 PST
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.
Comment 10 Yuzo Fujishima 2010-02-02 00:19:38 PST
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
Comment 11 Eric Seidel (no email) 2010-02-04 16:02:20 PST
It's always OK to add failing tests that we will fix later. :)  Makes the later fixing easier.
Comment 12 Eric Seidel (no email) 2010-02-04 16:35:07 PST
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)?
Comment 13 Shinichiro Hamaji 2010-02-04 22:58:44 PST
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.
Comment 14 Yuzo Fujishima 2010-02-07 22:20:13 PST
Created attachment 48316 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.
Comment 15 Yuzo Fujishima 2010-02-07 22:26:08 PST
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
Comment 16 Shinichiro Hamaji 2010-02-08 02:09:35 PST
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.
Comment 17 Yuzo Fujishima 2010-02-08 02:28:03 PST
Created attachment 48327 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.
Comment 18 Yuzo Fujishima 2010-02-08 02:29:57 PST
Thank you for the review.

Fixed as:
  if (window.layoutTestController) {
Comment 19 Yuzo Fujishima 2010-02-15 23:57:34 PST
Ping?
Comment 20 Eric Seidel (no email) 2010-02-17 14:30:43 PST
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?
Comment 21 Yuzo Fujishima 2010-02-17 17:05:11 PST
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.
Comment 22 Eric Seidel (no email) 2010-02-19 12:33:13 PST
Comment on attachment 48327 [details]
When page-break-{after,before} is set to always, force page breaks even for overflow-specified elements.

OK.
Comment 23 WebKit Commit Bot 2010-02-19 12:46:22 PST
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
Comment 24 Eric Seidel (no email) 2010-02-19 16:22:44 PST
Attachment 48327 [details] was posted by a committer and has review+, assigning to Yuzo Fujishima for commit.
Comment 25 Yuzo Fujishima 2010-02-21 20:13:03 PST
Committed r55067: <http://trac.webkit.org/changeset/55067>