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.
Created attachment 160586 [details] Patch
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
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
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)
Created attachment 160868 [details] Patch that addresses Dan's comments
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
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
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
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?