[webkit-reviews] review denied: [Bug 9526] CSS page-break properties quirk : [Attachment 47910] When page-break-{after, before} is set to always, force page breaks even for overflow-specified elements.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 4 22:58:43 PST 2010


Shinichiro Hamaji <hamaji at chromium.org> has denied Yuzo Fujishima
<yuzo at google.com>'s request for review:
Bug 9526: CSS page-break properties quirk
https://bugs.webkit.org/show_bug.cgi?id=9526

Attachment 47910: When page-break-{after,before} is set to always, force page
breaks even for overflow-specified elements.
https://bugs.webkit.org/attachment.cgi?id=47910&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
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.


More information about the webkit-reviews mailing list