Bug 46422 – Make printing and pagination work with vertical text.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Make printing and pagination work with vertical text.

[Adele Peterson]

<rdar://problem/8612055>

[Dave Hyatt]

Created attachment 76546 [details]
Part 1: Rename pageHeight -> pageLogicalHeight

[Dave Hyatt]

Part 1 landed in r74048.

[Dave Hyatt]

Created attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

[Simon Fraser (smfr)]

Comment on attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

r=me, but please add comments to the headers explaining what pageLogicalOffset really means.

[Dave Hyatt]

Part 2 landed in r74121.

[Eric Seidel (no email)]

Comment on attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

Marking obsolete since landed.

[Eric Seidel (no email)]

Did you still want this open Hyatt?

[Eric Seidel (no email)]

Comment on attachment 76546 [details]
Part 1: Rename pageHeight -> pageLogicalHeight

Cleared Timothy Hatcher's review+ from obsolete attachment 76546 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 76659 [details]
Part 2: pageY -> pageLogicalOffset

Cleared Simon Fraser's review+ from obsolete attachment 76659 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dave Hyatt]

Created attachment 80377 [details]
vertical-lr works great.  vertical-rl prints correctly but spasms the scrollbar in WebKit2.  vertical-rl does not print correctly in Mac WebKit1.

[Darin Adler]

Comment on attachment 80377 [details]
vertical-lr works great.  vertical-rl prints correctly but spasms the scrollbar in WebKit2.  vertical-rl does not print correctly in Mac WebKit1.

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

Apparently the patch doesn’t apply so the EWS is not helping you. You may want to post a later version just to let the EWS double check things, but since so much of this is Mac-specific that may not matter.

> Source/WebCore/page/FrameView.cpp:2285
> +        root->setPageLogicalHeight(root->style()->isHorizontalWritingMode() ? pageSize.height() : pageSize.width());

You meant to use your pageLogicalHeight local variable here, right? Why repeat the expression?

> Source/WebCore/page/FrameView.cpp:2295
> +            flooredPageLogicalWidth = std::min<int>(docLogicalWidth, static_cast<int>(pageLogicalWidth * maximumShrinkFactor));

You can use min without the std:: prefix if you put the using namespace std at the top of the file. Even though that’s not new.

Given that you specify min<int> I am surprised you need the static_cast.

I think you should put a small comment explaining why you use floor. It may be hard to keep the comment short, though!

> Source/WebCore/page/PrintContext.cpp:89
> +        pageWidth  = view->docWidth();

I don’t like those extra spaces.

> Source/WebCore/page/PrintContext.cpp:90
> +        pageHeight = floor(pageWidth * ratio);

Since this is float, you should use floor like the old code. Because floor is for double, not float.

> Source/WebCore/page/PrintContext.cpp:93
> +        pageHeight  = view->docHeight();

Same.

> Source/WebCore/page/PrintContext.cpp:94
> +        pageWidth = floor(pageHeight * ratio);

Same.

> Source/WebCore/page/PrintContext.cpp:162
> +        int pageLogicalTop = blockDirectionEnd > blockDirectionStart ?
> +                                blockDirectionStart + i * pageLogicalHeight : 
> +                                blockDirectionStart - (i + 1) * pageLogicalHeight;

I don’t love this formatting. I prefer to put the ? and : at the starts of the lines and use 4-character indent. Like this:

    int pageLogicalTop = blockDirectionEnd > blockDirectionStart
        ? blockDirectionStart + i * pageLogicalHeight
        :  blockDirectionStart - (i + 1) * pageLogicalHeight;

Hope bugs.webkit.org doesn’t wrap that, but you’ll get the idea.

> Source/WebCore/page/PrintContext.cpp:166
> +            for (int currentInlinePosition = inlineDirectionStart;
> +                 inlineDirectionEnd > inlineDirectionStart ? currentInlinePosition < inlineDirectionEnd : currentInlinePosition > inlineDirectionEnd;
> +                 currentInlinePosition += (inlineDirectionEnd > inlineDirectionStart ? pageLogicalWidth : -pageLogicalWidth)) {

A bool for inlineDirectionEnd > inlineDirectionStart could help if you can find a small enough name for it.

> Source/WebCore/page/PrintContext.cpp:200
> +    bool useViewWidth = true;

The boolean here is a little confusing. I had no idea that false meant to use the height. Might be better with orientation or something.

> Source/WebKit/mac/WebView/WebHTMLView.mm:2226
> +    bool isHorizontal = true;
> +    Document* document = frame->document();
> +    if (document && document->renderView() && !document->renderView()->style()->isHorizontalWritingMode())
> +        isHorizontal = false;

You could just initialize isHorizontal here instead of using an if.

    Document* document = frame->document();
    bool isHorizontal = !document || !document->renderView() || document->renderView()->style()->isHorizontalWritingMode();

Or you could set it inside the if like this:

    bool isHorizontal = true;
    Document* document = frame->document();
    if (document && document->renderView())
        isHorizontal = document->renderView()->style()->isHorizontalWritingMode());

> Source/WebKit/mac/WebView/WebHTMLView.mm:2263
> +    bool isHorizontal = true;
> +    Document* document = frame->document();
> +    if (document && document->renderView() && !document->renderView()->style()->isHorizontalWritingMode())
> +        isHorizontal = false;

Same comment as above. Maybe you should make a little helper function that takes a frame and gives you this boolean.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3155
> +        if (minPageLogicalWidth > 0.0) {

Maybe just > 0 rather than > 0.0.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3979
> +    bool useViewWidth = true;
> +    Frame* coreFrame = core([self _frame]);
> +    if (coreFrame) {
> +        Document* document = coreFrame->document();
> +        if (document && document->renderView())
> +            useViewWidth = document->renderView()->style()->isHorizontalWritingMode();
> +    }

This could use that frame helper I mentioned above.

Also, I am slightly annoyed that you use local variables for frame, then document, but then decide not to for renderView. What changed at that level?

> Source/WebKit/mac/WebView/WebHTMLView.mm:3981
> +    float viewLogicalWidth = useViewWidth ? NSWidth([self bounds]) : NSHeight([self bounds]);

This should be CGFloat since it’s getting things directly from NSWidth. But then inside WebCore I guess it’s all float. Somewhere it narrows on 64-bit. Probably no big deal either way.

[Dave Hyatt]

Bungled the landing a bit.  r77257-77259 to get everything.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77257 might have broken SnowLeopard Intel Release (Tests)

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> Bungled the landing a bit.  r77257-77259 to get everything.

Oddly enough, this broke printing in Chromium -- maybe in Safari too? See http://code.google.com/p/chromium/issues/detail?id=76694 for details.