Bug 94848 – When paged-x/y is specified on the root, columnGap is ignored, and garbage pixels are likely

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Beth Dakin]

https://bugs.webkit.org/show_bug.cgi?id=94401 added support for paged-x/paged-y. When paged-x/y is specified on the root, columnGap is ignored, and garbage pixels are likely. This is evident with LayoutTests/fast/overflow/paged-x-on-root.html. If you manually scroll to the right edge in that test case, you are very likely to get a 16px wide stretch of garbage pixels. And if you assign any -webkit-column-gap to that example, it will be ignored.

[Beth Dakin]

Created attachment 160586 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160586 [details]
Patch

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

New failing tests:
fast/overflow/paged-x-div-with-column-gap.html
fast/overflow/paged-x-with-column-gap.html

[WebKit Review Bot]

Created attachment 160588 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Beth Dakin]

Created attachment 160866 [details]
Patch that takes a different approach

Comment on attachment 160866 [details]
Patch that takes a different approach

View in context: https://bugs.webkit.org/attachment.cgi?id=160866&action=review

> Source/WebCore/page/FrameView.cpp:647
> +    if (!documentOrBodyRenderer)
> +        return;

Is it OK to return without resetting pagination in this case?

> Source/WebCore/page/FrameView.cpp:657
> +    if (overflowY == OPAGEDX) {
> +        pagination.mode = WebCore::paginationModeForRenderStyle(documentOrBodyRenderer->style());
> +        pagination.gap = static_cast<unsigned>(documentOrBodyRenderer->style()->columnGap());
> +    } else if (overflowY == OPAGEDY) {
> +        pagination.mode = WebCore::paginationModeForRenderStyle(documentOrBodyRenderer->style());
> +        pagination.gap = static_cast<unsigned>(documentOrBodyRenderer->style()->columnGap());

You seem to be doing the same thing in both cases.

> Source/WebCore/page/FrameView.cpp:660
> +        return;

Seem like you should definitely reset pagination in this case, or you’ll never be able to leave paginated mode.

> Source/WebCore/rendering/RenderView.cpp:253
> +    if (m_frameView) {
> +        if (m_frameView->pagination().mode != Pagination::Unpaginated)

This can be written as
    if (m_frameView && m_frameView->pagination().mode != Pagination::Unpaginated)

[Beth Dakin]

Created attachment 160868 [details]
Patch that addresses Dan's comments

[WebKit Review Bot]

Comment on attachment 160868 [details]
Patch that addresses Dan's comments

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

New failing tests:
fast/overflow/paged-x-div-with-column-gap.html
fast/overflow/paged-x-with-column-gap.html

[WebKit Review Bot]

Created attachment 160886 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Beth Dakin]

Thanks Dan! Committed change with http://trac.webkit.org/changeset/126840

And I filed a bug for checking in new Chromium expectations: https://bugs.webkit.org/show_bug.cgi?id=95167

[Julien Chaffraix]

It's unfortunate that the tests from this change are living examples of what shouldn't be done.

The tests don't describe what they are testing nor what the expected results and conditions for passing / failing should be. A link to the bug for context is also welcome.

Reading the change and the tests, we probably wanted to see that there was an empty 100px gap between the columns. Those seem like easy candidates for a ref-tests but nowhere was it pointed out.

If a ref-test is unfeasible, the tests were (unfortunately) designed to be platform dependent:
* We don't care about the text's content, yet we don't use Ahem to have consistent baselines.
* We don't care about the scrollbars, yet we let them be there (overflow: hidden would remove them and you can still scroll (not sure if it's an option in this case)). If we need them, you could opt into mock scrollbars.
* Not 100% sure about that but it looks like the TEXT output is unneeded as we don't dump any meaningful information related to pagination.

Beth, could you please fix some of the previous points?