Last Comment Bug 663028 - Make Print Preview themable with CSS, and modernize its look
: Make Print Preview themable with CSS, and modernize its look
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Print Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Michael Ventnor
:
Mentors:
Depends on: 696566
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 21:39 PDT by Michael Ventnor
Modified: 2012-03-19 07:03 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (22.36 KB, patch)
2011-06-08 21:42 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 (21.87 KB, patch)
2011-06-08 23:37 PDT, Michael Ventnor
roc: review+
Details | Diff | Splinter Review

Description Michael Ventnor 2011-06-08 21:39:33 PDT
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 2011-06-08 21:42:43 PDT
Created attachment 538169 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-06-08 22:01:05 PDT
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 2011-06-08 23:37:18 PDT
Created attachment 538191 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-06-09 03:20:40 PDT
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 2011-06-09 03:39:36 PDT
(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 2011-06-09 04:05:29 PDT
Hmm, using mComputedMinHeight and min-height: 100% doesn't work, it doesn't stretch the background to the bottom.
Comment 7 Michael Ventnor 2011-06-09 21:19:16 PDT
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) (Exited; email my personal email if necessary) 2011-06-10 02:10:19 PDT
Comment on attachment 538191 [details] [diff] [review]
Patch 2

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

Let's just go with this.
Comment 11 Michael Ventnor 2011-06-22 04:09:59 PDT
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 2011-06-22 04:11:33 PDT
And I should add that not all CSS rules are supported, mostly just the visual ones like background.
Comment 13 Michael Ventnor 2011-06-22 04:14:35 PDT
(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] 2011-08-24 02:43:00 PDT
Could anyone please provide a test case in order to get this issue verified on QA side?

Note You need to log in before you can comment on or make changes to this bug.