Closed Bug 663028 Opened 13 years ago Closed 13 years ago

Make Print Preview themable with CSS, and modernize its look

Categories

(Core :: Print Preview, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: ventnor.bugzilla, Assigned: ventnor.bugzilla)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → ventnor.bugzilla
Attachment #538169 - Flags: review?(roc)
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")
Attached patch Patch 2Splinter Review
(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.
Attachment #538169 - Attachment is obsolete: true
Attachment #538169 - Flags: review?(roc)
Attachment #538191 - Flags: review?(roc)
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.
(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.
Hmm, using mComputedMinHeight and min-height: 100% doesn't work, it doesn't stretch the background to the bottom.
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 on attachment 538191 [details] [diff] [review]
Patch 2

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

Let's just go with this.
Attachment #538191 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/251688029a9d
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
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.
Keywords: dev-doc-needed
And I should add that not all CSS rules are supported, mostly just the visual ones like background.
(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.
Summary: Make Print Preview themable with CSS → Make Print Preview themable with CSS, and modernize its look
Could anyone please provide a test case in order to get this issue verified on QA side?
Depends on: 696566
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: