Bug 91299 - Paginated views should restrict available height to column height
Summary: Paginated views should restrict available height to column height
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-07-13 16:48 PDT by Beth Dakin
Modified: 2012-07-16 14:07 PDT (History)
4 users (show)

See Also:

Attachments
Patch (3.70 KB, patch)
2012-07-13 16:53 PDT, Beth Dakin 		mitz: review-
Details | Formatted Diff | Diff
Patch with test (30.20 KB, patch)
2012-07-13 18:30 PDT, Beth Dakin 		webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (545.39 KB, application/zip)
2012-07-13 19:30 PDT, WebKit Review Bot 		no flags Details
Patch (30.28 KB, patch)
2012-07-16 13:53 PDT, Beth Dakin 		mitz: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-07-13 16:48:56 PDT 
Paginated views should restrict available height to column height.

<rdar://problem/11152108>
Comment 1 Beth Dakin 2012-07-13 16:53:18 PDT 
Created attachment 152376 [details]
Patch
Comment 2 mitz 2012-07-13 17:09:03 PDT 
Comment on attachment 152376 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +

You should be able to make a layout test for this, using window.internals.pagination.

> Source/WebCore/rendering/RenderBlock.cpp:5023
> +    if (isRenderView() && hasColumns())
> +        return columnInfo()->columnHeight();

Why can’t RenderView be the one overriding this function? If we’re not a RenderView, isRenderView() is going to return false anyway.

> Source/WebCore/rendering/RenderBlock.h:199
> +    virtual LayoutUnit availableLogicalHeight() const;

This override (though I think it will go in RenderView.h) should be annotated with the OVERRIDE macro.
Comment 3 Beth Dakin 2012-07-13 18:30:47 PDT 
Created attachment 152393 [details]
Patch with test
Comment 4 WebKit Review Bot 2012-07-13 19:30:55 PDT 
Comment on attachment 152393 [details]
Patch with test

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

New failing tests:
fast/multicol/shrink-to-column-height-for-pagination.html
Comment 5 WebKit Review Bot 2012-07-13 19:30:58 PDT 
Created attachment 152399 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 6 mitz 2012-07-13 21:49:52 PDT 
Comment on attachment 152393 [details]
Patch with test

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

> Source/WebCore/ChangeLog:25
> +        New window.internals function allows setting the pagination with a pageLength.

Since this is a JavaScript API, you can just add pageLength as an optional parameter to the existing function. Existing tests that don’t pass anything will behave as if they’d passed 0, I think, which is fine because 0 is the default value anyway.
Comment 7 Beth Dakin 2012-07-16 13:53:51 PDT 
Created attachment 152605 [details]
Patch
Comment 8 mitz 2012-07-16 13:59:06 PDT 
Comment on attachment 152605 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:111
> +

Extra empty line.
Comment 9 Beth Dakin 2012-07-16 14:07:38 PDT 
Thanks Dan! Committed: http://trac.webkit.org/changeset/122761