663028 - Make Print Preview themable with CSS, and modernize its look

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Michael Ventnor]

The original scope of this idea was to change the shadows of each page to use CSS box-shadow, however it was later increased to make more CSS rules applicable to print preview.

We do this by not treating a print preview page as a canvas element anymore so that arbitrary backgrounds can be painted, and by calling more standard reflow functions that these elements did not call before.

In doing so, I aim to make print preview look more modern.

Comment 1

[Michael Ventnor]

Attachment: Patch

This will allow print preview to take visual CSS properties as well as margin. I have sent this to the try server and it's looking OK so far, but I'd like to get a code review soon assuming this patch is fine.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538169 [details] [diff] [review]
Patch

Review of attachment 538169 [details] [diff] [review]:
-----------------------------------------------------------------

I really like this idea!

::: layout/generic/nsIPageSequenceFrame.h
@@ -81,5 @@
>  
>    NS_IMETHOD SetTotalNumPages(PRInt32 aTotal) = 0;
>  
> -  // Gets the dead space (the gray area) around the Print Preview Page
> -  NS_IMETHOD GetDeadSpaceValue(nscoord* aValue) = 0;

rev interface IID

::: layout/generic/nsSimplePageSequence.cpp
@@ +161,5 @@
> +      PresContext()->PresShell()->FrameManager()->GetRootFrame()->GetSize();
> +    aDesiredSize.width = NS_MAX(aReflowState.availableWidth,
> +                                nscoord(aWidth * PresContext()->GetPrintPreviewScale()));
> +    aDesiredSize.height = NS_MAX(topFrameSize.height,
> +                                 nscoord(aHeight * PresContext()->GetPrintPreviewScale()));

This seems a bit hacky. Can you set height:100% in ua.css and then set your desired height to the reflow state computed height?

@@ +360,3 @@
>    // correct size
> +  nscoord w = maxLeftMargin + availSize.width + maxRightMargin;
> +  SetDesiredSize(aDesiredSize, aReflowState, w, y);

This seems weird. Instead of maxLeftMargin/maxRightMargin, why not just compute the max of the XMost of all the pages? (i.e. max over all pages of "pageCSSMargin.left + kidSize.width + pageCSSMargin.right")

Comment 3

[Michael Ventnor]

Attachment: Patch 2

(In reply to comment #2)
> I really like this idea!

Of course you do, I came up with it.

> ::: layout/generic/nsIPageSequenceFrame.h
> @@ -81,5 @@
> >  
> >    NS_IMETHOD SetTotalNumPages(PRInt32 aTotal) = 0;
> >  
> > -  // Gets the dead space (the gray area) around the Print Preview Page
> > -  NS_IMETHOD GetDeadSpaceValue(nscoord* aValue) = 0;
> 
> rev interface IID

There is no IID in this file to rev, which is strange considering it is declared as an interface.

> ::: layout/generic/nsSimplePageSequence.cpp
> @@ +161,5 @@
> > +      PresContext()->PresShell()->FrameManager()->GetRootFrame()->GetSize();
> > +    aDesiredSize.width = NS_MAX(aReflowState.availableWidth,
> > +                                nscoord(aWidth * PresContext()->GetPrintPreviewScale()));
> > +    aDesiredSize.height = NS_MAX(topFrameSize.height,
> > +                                 nscoord(aHeight * PresContext()->GetPrintPreviewScale()));
> 
> This seems a bit hacky. Can you set height:100% in ua.css and then set your
> desired height to the reflow state computed height?

Thank you! I've been wanting to do this from the start but couldn't figure out what made it work.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538191 [details] [diff] [review]
Patch 2

Review of attachment 538191 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSimplePageSequence.cpp
@@ +157,5 @@
> +    // Use availableWidth so we don't cause a needless horizontal scrollbar.
> +    aDesiredSize.width = NS_MAX(aReflowState.availableWidth,
> +                                nscoord(aWidth * PresContext()->GetPrintPreviewScale()));
> +    aDesiredSize.height = NS_MAX(aReflowState.ComputedHeight(),
> +                                 nscoord(aHeight * PresContext()->GetPrintPreviewScale()));

Actually if there was a computed height, you should probably set the desired height to it. It's probably better to set min-height to 100% and use mComputedMinHeight here.

Comment 5

[Michael Ventnor]

(In reply to comment #4)
> Comment on attachment 538191 [details] [diff] [review] [review]
> Patch 2
> 
> Review of attachment 538191 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsSimplePageSequence.cpp
> @@ +157,5 @@
> > +    // Use availableWidth so we don't cause a needless horizontal scrollbar.
> > +    aDesiredSize.width = NS_MAX(aReflowState.availableWidth,
> > +                                nscoord(aWidth * PresContext()->GetPrintPreviewScale()));
> > +    aDesiredSize.height = NS_MAX(aReflowState.ComputedHeight(),
> > +                                 nscoord(aHeight * PresContext()->GetPrintPreviewScale()));
> 
> Actually if there was a computed height, you should probably set the desired
> height to it. It's probably better to set min-height to 100% and use
> mComputedMinHeight here.

I had to keep that approach because ComputedHeight didn't take the children into account, so I would always get a frame the same height as the viewport (and no way to scroll to see more pages).
min-height sounds like a better one to use, so I'll do that.

Comment 6

[Michael Ventnor]

Hmm, using mComputedMinHeight and min-height: 100% doesn't work, it doesn't stretch the background to the bottom.

Comment 7

[Michael Ventnor]

Found the problem, and it doesn't look fixable either...

A PageSequence frame is a direct child of a nsHTMLScrollFrame. The key code is in nsHTMLScrollFrame::ReflowScrolledFrame.

  nscoord computedMinHeight = aState->mReflowState.mComputedMinHeight;
  nscoord computedMaxHeight = aState->mReflowState.mComputedMaxHeight;
.....
  kidReflowState.mComputedMinHeight = computedMinHeight;
  kidReflowState.mComputedMaxHeight = computedMaxHeight;

We get the scroll frame's computed min-height no matter what, and it is always set to 0. Changing the above code to a constant integer passed that down as the min-height to the PageSequence frame.

So because we're a child of a scroll frame, we can't use min- and max- height.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538191 [details] [diff] [review]
Patch 2

Review of attachment 538191 [details] [diff] [review]:
-----------------------------------------------------------------

Let's just go with this.

Comment 9

[Michael Ventnor]

http://hg.mozilla.org/integration/mozilla-inbound/rev/251688029a9d

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/251688029a9d

Comment 11

[Michael Ventnor]

Perhaps this should be documented for Fx7? Since I think ua.css can be overridden with a theme, and some theme authors may want to style print preview. The relevant code is in layout/style/ua.css and looks like this:

*|*::-moz-page-sequence, *|*::-moz-scrolled-page-sequence {
  /* Collection of pages in print/print preview. Visual styles may only appear
   * in print preview. */
  display: block !important;
  background: -moz-linear-gradient(top, #606060, #8a8a8a) fixed;
  height: 100%;
}

*|*::-moz-page {
  /* Individual page in print/print preview. Visual styles may only appear
   * in print preview. */
  display: block !important;
  background: white;
  box-shadow: 5px 5px 8px #202020;
  margin: 0.125in 0.25in;
}

moz-page-sequence represents the background of print preview, moz-page represents a single page.
I for one would love to see what a foxkeh print preview looks like.

Comment 12

[Michael Ventnor]

And I should add that not all CSS rules are supported, mostly just the visual ones like background.

Comment 13

[Michael Ventnor]

(In reply to comment #11)
> moz-page-sequence represents the background of print preview

Forgot to mention moz-scrolled-page-sequence is just as important. In fact, I think that's the one used in print preview.

Sorry for all the bugspam.

Comment 14

[Virgil Dicu [:virgil] [QA]]

Could anyone please provide a test case in order to get this issue verified on QA side?

Comment 15

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en/Firefox_7_for_developers#Other_Changes
https://developer.mozilla.org/en/CSS/::-moz-page
https://developer.mozilla.org/en/CSS/::-moz-page-sequence
https://developer.mozilla.org/en/CSS/::-moz-scrolled-page-sequence