Closed Bug 978044 Opened 10 years ago Closed 10 years ago

Firefox Print Preview should be center aligned

Categories

(Core :: Print Preview, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Debloper, Assigned: dholbert)

Details

Attachments

(3 files, 2 obsolete files)

The print-preview display of Firefox is left aligned, if the page-width is smaller than the viewport width.

Can't think of any possible reason, why should it be that way; but in general, it makes more sense to align it with the vertical middle.
Whiteboard: [lang=js][mentor=debloper@gmail.com]
hey .. can u tell me where can i find the source code for the preview page?
Ideally you should start here: http://mxr.mozilla.org/mozilla-central/
Try to look for the source code that affect the print-preview mode.

I'm hoping you're all set with the environment to build Firefox. If not, this should help you get started: https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions

If you need help with something specific, please don't be shy to ask.
Hey .... i think the page exists at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/ .... but i still need to confirm this.
Hey .... like this isnt the source code for that component.... can u help me find the souce code file for the print preview display region?
Hey Gaurav, sorry for the delayed response... the weekend was a dash!

In short, this particular line needs some work: http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#204

For more in-depth understanding of why-so, you may wanna take a look at: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2594

Let me know if you come up with the solution! ;)
Whiteboard: [lang=js][mentor=debloper@gmail.com] → [lang=c++][mentor=debloper@gmail.com]
Whiteboard: [lang=c++][mentor=debloper@gmail.com] → [lang=c++][lang=js][lang=css][mentor=debloper@gmail.com]
Hey .... like i would like to work on this bug! 

I will upload the patch soon!
Comment on attachment 8385561 [details] [diff] [review]
patch attempt, using CSS (unfortunately doesn't work)

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

Hey Gaurav,

The top/bottom margin are best to be left with absolute value (in inch - as it was).
We can't predict the page height, and so there will be discrepancy in top-margin on page-resize.
Let it be... or change it to "0.25in" if that looks any better (but stick with absolute value).

The left/right margin should be something more generic than "30%".
I am guessing that looks okay on your screen-resolution, with the page size you've selected - but will not FIX the bug for someone with a different setup.

Can you please make the left/right margin a bit more generic, so that it's agnostic of user's screen-resolution/page-size?
Attachment #8385561 - Flags: review?(debloper) → review-
I want to work on this bug but I have difficulties to compile code. Can you tell me how I can do this on Linux and MacIOS?
Hey,

I tried the same patch on a different resolution. The page was still in the center, however it needed to be be horizontally scrolled a little to have a proper display. This same thing occurs in the Landscape mode of most resolutions. But the page remains in the center.
(In reply to bhr from comment #9)
> I want to work on this bug but I have difficulties to compile code. Can you
> tell me how I can do this on Linux and MacIOS?

Please check Comment 5

(In reply to gauravmittal from comment #10)
> The page was still in the center, however it needed to be
> horizontally scrolled a little to have a proper display.

Yes, my concern is with the "horizontally scrolled a little" part. Having 30% margin, that's supposed to happen. If you check with other resolution, you'll find same discrepancy.

You may wanna try & look into the CSS calc() to fix it, I wager.
Super, i did it! Can you assign me to bug please?
Done... up the patch, can't wait! :D \o/
Assignee: nobody → bhr.bhr.info8
Component: General → Print Preview
Product: Firefox → Core
Flags: needinfo?(bhr.bhr.info8)
Version: unspecified → Trunk
Mentor: debloper
Whiteboard: [lang=c++][lang=js][lang=css][mentor=debloper@gmail.com] → [lang=c++][lang=js][lang=css]
Hi. I can work on this bug if no one else currently is and if it still exists in the latest version of Firefox. This is my first time so I might need some help. Can someone please confirm if the lines marked in Comment 5 are still relevant, since the comment is 3 months old and I'm guessing some code might have changed the lines along the way.
Flags: needinfo?(debloper)
Hi, I just checked in the nightly version, the bug still exists. I tried to fix it by using calc(50% - 407px) or calc(50vw - 407px) but it doesn't seem to work (407px = half of the preview container's width).

If I use % or vw, it calculates these with respect to the container's width instead of with respect to the width of the window width. Can someone please guide me with this?
Debloper, can you take a look at this?
(In reply to Chirag Bhatia from comment #15)
> Hi, I just checked in the nightly version, the bug still exists. I tried to
> fix it by using calc(50% - 407px) or calc(50vw - 407px) but it doesn't seem
> to work (407px = half of the preview container's width).

You can never know the absolute value of the margin (2*407px, in this case), as it will vary on different resolution.


> If I use % or vw, it calculates these with respect to the container's width
> instead of with respect to the width of the window width. Can someone please
> guide me with this?

If the parent element is given block display, then the child block element can easily go with a margin: <Xpx> auto; Let me know if that works. I'll be glad to have a chat on this, maybe catch me online? ;)
Flags: needinfo?(debloper)
I can confirm that changing the margin from "0.125in 0.25in" to "0.125in auto" does not solve the problem.
(In reply to Nathan Yee from comment #18)
> I can confirm that changing the margin from "0.125in 0.25in" to "0.125in
> auto" does not solve the problem.

If the parent element isn't a block element, then applying margin-left-right: auto; to a child element won't work. That seems to be the catch.

Explained to Chirag the same over a chat; any luck there?
Indeed, the parent frame of the "-moz-page" element is a nsSimplePageSequenceFrame, not a nsBlockFrame, as shown in the commented tree structure here:
> 2701       ViewportFrame
> 2702         nsSimplePageSequenceFrame
> 2703           nsPageFrame [fixed-cb]
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=7589cc5c6787#2699

And the parent frame is responsible for interpreting what an "auto" margin means. And nsSimplePageSequenceFrame apparently doesn't do anything special with it.

So, I suspect it's not possible to get a pure-CSS centering solution here, at the moment.

I think we could do one of the following to fix this (all of which are C++ changes, except for the last one):
 (1) insert an anonymous auto-sized "wrapper" nsBlockFrame around each nsPageFrame, so that the nsPageFrame's auto margins can be resolved against that block. (A flex container frame would work as well, but a block frame is probably simpler & faster.)
...or...
 (2) Extend nsSimplePageSequenceFrame to handle auto margins like blocks do.
...or...
 (3) Hard-code some centering logic into nsSimplePageSequenceFrame's reflow code. (sort of like we do for vertical centering of buttons' contents)
...or...
 (4) Do some centering in JS, wherever the UI is for the print-preview view, to figure out what the horizontal margin should be and then set it (as a pixel-value instead of "auto")

I think I prefer option (1). That option might break some assumptions (e.g. nsSimplePageSequenceFrame currently assumes that each of its children should be a nsPageFrame, and nsPageFrame assumes that its parent is a nsSimplePageSequenceFrame); any trouble from that should probably be workaroundable, but I'm not sure how bad the fallout would be.

This might mean this isn't "good first bug" material (since it's not just a CSS tweak), though it might be a "good second bug" (or possibly first) for someone who's comfortable with poking around in Gecko C++ code for layout.
Actually, I think (2) or (3) might be better than (1)... I'm just remembering that nsSimplePageSequenceFrame is used in actual-printing, too, and it might be too hacky to have a bogus extra block frame inserted there.  (Plus, creating continuation frames on the fly would become a little more complicated, if we had to create a block continuation for each new page frame.)

Perhaps better to just make nsSimplePageSequenceFrame smarter.
We actually have a code-comment in nsSimplePageSequenceFrame that implies that we do (3) right now (hardcoded centering), but AFAICT there's no actual code to do it:
> 223     // Place and size the page. If the page is narrower than our
> 224     // max width then center it horizontally
> 225     ReflowChild(kidFrame, aPresContext, kidSize, kidReflowState, x, y, 0, status);
> 226 
> 227     FinishReflowChild(kidFrame, aPresContext, kidSize, nullptr, x, y, 0);
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=a4ba6995c87e#223


I think what needs to happen here is: between the ReflowChild call and the FinishReflowChild call, we need to figure out how much extra space there is, and increase "x" if there is some extra space.  We might be able to even do this earlier (and make "x" start out at the right value, up-front), if the page-size is known up-front. It looks like we use nsPresContext::GetPageSize() in this function (and cache the result in a local variable) -- if that does what it sounds like it does, then I think we could subtract that along with the page's horizontal css-margin components from aReflowState.ComputedWidth() and (if that produces a positive result) divide it by two, and use that in combination with the page's left-margin to initialize 'x' here.
So e.g. right before:
> 221     const nscoord x = pageCSSMargin.left;
...we'd want:
  nscoord extraSpace = aReflowState.ComputedWidth() - pageSize.width - pageCSSMargin.LeftRight();
  nscoord centeringMargin = extraSpace > 0 ? extraSpace/2 : 0;

...and then change "x = pageCSSMargin.left" to "x = pageCSSMargin.left + centeringMargin".

chirag, want to give that a shot?
(In reply to Daniel Holbert [:dholbert] from comment #20 - #23)
If we could just apply { display: block; } to nsSimplePageSequenceFrame (not sure can't we, or what would be the downside), this would solve everything (or so I'd presume).

Chirag was a bit confused about "How do we debug style changes for nsContainerFrame, like we can do with devtools for the content part?" Being able to apply styles to the C++ generated views/elements will also be helpful (if there's a way).

Thoughts on that?
(In reply to Soumya Deb [:Debloper] from comment #24)
> (In reply to Daniel Holbert [:dholbert] from comment #20 - #23)
> If we could just apply { display: block; } to nsSimplePageSequenceFrame (not
> sure can't we, or what would be the downside), this would solve everything
> (or so I'd presume).

We can't do that. nsSimplePageSequenceFrame is its own type of frame, which manages the splitting of things into pages in printing.  If we applied "display:block" to its content, then we'd have a nsBlockFrame instead of a nsSimplePageSequenceFrame, and we'd be missing that pagination functionality.

 
> Chirag was a bit confused about "How do we debug style changes for
> nsContainerFrame, like we can do with devtools for the content part?" Being
> able to apply styles to the C++ generated views/elements will also be
> helpful (if there's a way).
> 
> Thoughts on that?

I'm not sure I understand what you're saying here.
(In reply to Daniel Holbert [:dholbert] from comment #20)
> Indeed, the parent frame of the "-moz-page" element is a
> nsSimplePageSequenceFrame, not a nsBlockFrame, as shown in the commented
> tree structure here:
> > 2701       ViewportFrame
> > 2702         nsSimplePageSequenceFrame
> > 2703           nsPageFrame [fixed-cb]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/
> nsCSSFrameConstructor.cpp?rev=7589cc5c6787#2699

Correct me if I'm wrong, but I think the tree is stacked for bottom up tracing, with root element at the bottom most. Hence, nsSimplePageSequenceFrame is child of nsPageFrame, not parent.

I kind of made a short-sighted statement in comment #24
> If we could just apply { display: block; } to nsSimplePageSequenceFrame
Actually I meant to apply that style to nsPageContentFrame - parent of nsPageFrame. I should've re-read what I wrote; sorry.

(In reply to Daniel Holbert [:dholbert] from comment #25)
> > Chirag was a bit confused about "How do we debug style changes for nsContainerFrame,
> > like we can do with devtools for the content part?" Being able to apply styles to
> > the C++ generated views/elements will also be helpful (if there's a way).
> 
> I'm not sure I understand what you're saying here.

Some tool/way to apply styles on C++ generated chrome elements for live debugging. Not a crucial part of the problem in hand. Can be discussed off thread. ;)
(In reply to Soumya Deb [:Debloper] from comment #26)
> Correct me if I'm wrong, but I think the tree is stacked for bottom up
> tracing, with root element at the bottom most. Hence,
> nsSimplePageSequenceFrame is child of nsPageFrame, not parent.

Nope, it's the other way around. nsSimplePageSequenceFrame is the container for a sequence of nsPageFrames.

> Actually I meant to apply that style to nsPageContentFrame - parent of
> nsPageFrame. I should've re-read what I wrote; sorry.

(You were correct the first time, RE the parentage, so this isn't applicable.)

> (In reply to Daniel Holbert [:dholbert] from comment #25)
> Some tool/way to apply styles on C++ generated chrome elements for live
> debugging. Not a crucial part of the problem in hand. Can be discussed off
> thread. ;)

Gotcha. :) Yeah, I don't know of a way to do that, currently.
(In reply to Daniel Holbert [:dholbert] from comment #27)
> Nope, it's the other way around. nsSimplePageSequenceFrame is the container
> for a sequence of nsPageFrames.

Hmm, okay... then how about applying { text-align: center; } to the parent of nsSimplePageSequenceFrame (nsHTMLScrollFrame)? The reason being, we aren't looking for each page (items in the paginated sequence) being separately centered - rather, we want the entire sequence to be centered, right?

Applying centered text-alignment to nsHTMLScrollFrame (it's a block elem, apparently) will ensure the nsSimplePageSequenceFrame being horizontally at the middle of the page (w/o any hack) - unless I'm missing anything. What do you think?

I'm not sure what's the selector for nsHTMLScrollFrame, but following is the closest I can get:
[*|*::-moz-viewport, *|*::-moz-viewport-scroll] - please correct me here.
(In reply to Soumya Deb [:Debloper] from comment #28)
> (In reply to Daniel Holbert [:dholbert] from comment #27)
> > Nope, it's the other way around. nsSimplePageSequenceFrame is the container
> > for a sequence of nsPageFrames.
> 
> Hmm, okay... then how about applying { text-align: center; } to the parent
> of nsSimplePageSequenceFrame (nsHTMLScrollFrame)? The reason being, we
> aren't looking for each page (items in the paginated sequence) being
> separately centered - rather, we want the entire sequence to be centered,
> right?

I don't think that would work, because the nsSimplePageSequenceFrame is the full width of the window, IIRC (so it's already "centered" in that it's the full width).

Also, even if it were skinnier -- "text-align" only centers inline-level content in an inline-level flow, which is not what we have here. (nsHTMLScrollFrame does not lay out inline frames.)

I really think comment 23 is probably what we want here.  (That, or adding a block wrapper -- option (1) from comment 20.  But that would require C++ changes to accomodate the extra frame layer between nsSimplePageSequenceFrame & nsPageFrame, so it's probably more complex.)
(In reply to Daniel Holbert [:dholbert] from comment #23)
> So e.g. right before:
> > 221     const nscoord x = pageCSSMargin.left;
> ...we'd want:
>   nscoord extraSpace = aReflowState.ComputedWidth() - pageSize.width -
> pageCSSMargin.LeftRight();
>   nscoord centeringMargin = extraSpace > 0 ? extraSpace/2 : 0;
> 
> ...and then change "x = pageCSSMargin.left" to "x = pageCSSMargin.left +
> centeringMargin".
> 
> chirag, want to give that a shot?

Hi, thanks for the pointers, Daniel.

I tried your approach, but it didn't seem to take any effect.

Here are the changes that I made:

> 222     nscoord extraSpace = aReflowState.ComputedWidth() - pageSize.width - pageCSSMargin.LeftRight();
> 223     nscoord centerMargin = extraSpace > 0 ? extraSpace/2 : 0;
> 224     const nscoord x = pageCSSMargin.left + centerMargin;
> 225     //Standard output in console to show values for debugging
> 226     std::cout << "Extraspace: " << extraSpace << " centerMargin: " << centerMargin << " x: " << x << std::endl;

I used stdout for help in debugging (not sure if this is the best method to debug as I'm not very good at C++).

Here is the output that I got:
> Extraspace: -3920 centerMargin: 0 x: 1440
> Extraspace: -3920 centerMargin: 0 x: 1440

I also noticed that if I remove the margin from the ua.css file, the value of x changes to 0 and there's no margin. Not sure if this helps.
Ok, I tried to remove the margin for the "*|*::-moz-page" selector from the ua.css file and changed the stdout line to generate some more output for debugging:

> Results in Potrait mode:
> aReflowState.ComputedWidth(): 47920, pageSize.width: 48960, CSSMargin.LeftRight(): 0, extraSpace: 47920, pageCSSMargin.left: 0, Extraspace: 47920, CenterMargin: 23960, x: 23960
> Results in Landscape mode:
> aReflowState.ComputedWidth(): 62320, pageSize.width: 63360, CSSMargin.LeftRight(): 0, extraSpace: 62320, pageCSSMargin.left: 0, Extraspace: 62320, CenterMargin: 31160, x: 31160

Hope this helps.

Soumya, any thoughts?
Flags: needinfo?(dholbert)
Flags: needinfo?(debloper)
Ah, so nsSimplePageReflowState is a bit sneakier than I thought.

It only *actually* reflows its children *on the first time it gets a Reflow call*.  On its subsequent Reflows, we don't get to the code you added in comment 30. Instead, we take this clause, which accepts the size passed in by our parent and return early...
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=05eb059f70bf#134
...without bothering to tell our children about the change.

And unfortunately, on the first reflow (the only one where we're hitting your added code), the nsSimplePageSequenceFrame is given its intrinsic size (the width of its children), and then only later do we reflow it again at a larger width. (And in that later reflow, we return early before hitting your code.)

The initial not-full-window-width reflow is triggered by this line of JS:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js?rev=34c44a0bc706#217

and the delayed reflow (at the full window width) is triggered by this line:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js?rev=34c44a0bc706#272

I'm guessing the size-change is because we go from .collapsed = true to .collapsed = false, right before that last line's focus(). I'm not sure what .collapsed means or why we need it, but it looks like it was necessary to fix a bug in the past, per this comment:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js?rev=34c44a0bc706#92


ANYWAY:
The early-return from nsSimplePageReflowState's reflow method is pretty broken for a frame class that's expecting dynamic changes, but it sort of makes sense (as a hack) for something static like a printed page, I suppose.  (Though given that we reflow it several times, it *is* getting dynamic changes [albeit just size changes], so it's a bit bogus for it to not expect dynamic changes.)  Per the comment right below that early-return, it sounds like it's there because of some table brokenness.

So, there are a number of things we could do here:
 (A) Do away with that early-return. (Arguably the "right thing", but probably risky, given the comment about tables)
 (B) Do away with this .collapse hack. (Again, probably risky.)

...but the thing we probably should do is:
 (C) In addition to calculating centerMargin where you've got it, *also* add some code in the early-return linked at the top of this comment, to walk over the child frames and: 
   - calculate "centerMargin" and "x" for the frame (like you're already doing, further down). You'll need to create a nsCSSOffsetState for the child frame [which is a lighter-weight nsHTMLReflowState, for use when you're not going to actually reflow].  You need this offsetState so that you can call .ComputedPhysicalMargin() on it (to get the "pageCSSMargin", which you need to compute the new "x").
   - If this produces an "x" that's different from the child frame's current x-position (from frame->GetPosition().x), then call MovePosition() on the child frame to adjust its position, by nsPoint(newX - frame->GetPosition().x, 0)

Let me know if that makes sense.

You might want to refactor the x-computation into a helper-function, too, to avoid duplicated arithmetic-logic between your code in comment 30 and this new code.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #32)
> The early-return from nsSimplePageReflowState's reflow method is pretty
> broken [...] Per the comment right below that early-return, it sounds
> like it's there because of some table brokenness.

Sorry, I meant "the comment right *before* that early-return" ("Don't do incremental reflow until we've taught tables [...]", in nsSimplePageSequenceFrame.cpp)
Tapping on the glass, here - Chirag, are you still working on this bug? Has Daniel's reply answered your question?
Hi Mike,

yes, I'm still working on it.

Daniel, I forgot to mention that I made some other changes that I did not mention in the cpp file in Comment #31.

> nscoord extraSpace = aReflowState.ComputedWidth() - pageCSSMargin.LeftRight();
> nscoord centerMargin = extraSpace > 0 ? extraSpace/2 : 0;
> const nscoord x = pageCSSMargin.left + centerMargin;
> std::cout << "aReflowState.ComputedWidth(): " << aReflowState.ComputedWidth() << ", pageSize.width: " << pageSize.width << ", CSSMargin.LeftRight(): " << pageCSSMargin.LeftRight() << ", extraSpace: " << extraSpace << ", pageCSSMargin.left: " << pageCSSMargin.left << ", Extraspace: " << extraSpace << ", CenterMargin: " << centerMargin << ", x: " << x << s    td::endl;

And the changes I made in the ua.css file included removing the margin styling for the "*|*::-moz-page" selector.

This gave the output that I mentioned earlier in Comment #31:
> Results in Potrait mode:
> aReflowState.ComputedWidth(): 47920, pageSize.width: 48960, CSSMargin.LeftRight(): 0, extraSpace: 47920, pageCSSMargin.left: 0, Extraspace: 47920, CenterMargin: 23960, x: 23960
> Results in Landscape mode:
> aReflowState.ComputedWidth(): 62320, pageSize.width: 63360, CSSMargin.LeftRight(): 0, extraSpace: 62320, pageCSSMargin.left: 0, Extraspace: 62320, CenterMargin: 31160, x: 31160

These changes were not made during Comment #30, so that output still stays true.

I hope this doesn't change any of your previous conclusions.

Also, I could use some pointers as to where and what change to make since this is my first bug and I'm not very experienced at C++.
(In reply to Chirag Bhatia from comment #35)
> yes, I'm still working on it.
> 
> Daniel, I forgot to mention that I made some other changes that I did not
> mention in the cpp file in Comment #31.
[...]
> I hope this doesn't change any of your previous conclusions.

Nope, I don't think so.

> Also, I could use some pointers as to where and what change to make since
> this is my first bug and I'm not very experienced at C++.

I think the things we need to change are at the end of Comment 32, part (C) in particular. The early-return I'm referring to there is http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=05eb059f70bf#134
(and by "walk over the child frames", I'm talking about a loop like this one:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=05eb059f70bf#204
)
(In reply to Daniel Holbert [:dholbert] from comment #37)
> (and by "walk over the child frames", I'm talking about a loop like this one:
Actually, that's a bad example of walking over child frames. You really want something like this:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.h?rev=ea66ed3c2ca1&mark=500-501#500

(I filed bug 1050412 on fixing that bad example.)
Hi Daniel, I tried to spend a few hours today trying to figure out how this is working based on your comments. I'm afraid my knowledge about the code in Firefox & C++ in general is quite limited to solve this bug. I really do appreciate your help in this and I hope your comments help the next guy figure out how to solve this.

Soumya, I appreciate your help with this bug as well. I think it would be better to reassign this bug to someone else more knowledgable than me on this :)
Thanks for the update, Chirag, and thanks for having taken a crack at this! I think this bug turned out to be much trickier than anyone here was expecting.

I'm unassigning you, per your request, and updating the "whiteboard" field to indicate that this turned out to be a mostly (or perhaps entirely) C++ bug.
Assignee: bhr.bhr.info8 → nobody
Whiteboard: [lang=c++][lang=js][lang=css] → [lang=c++] [see comment 23 & comment 32 for what probably needs to happen]
Flags: needinfo?(bhr.bhr.info8)
Thanks for taking your time with this Chirag.
Sorry I couldn't be as much of help as I should've been.

Daniel, I think it'd be better if you mentor this bug.
If you're okay with it, please feel free to swap my nick in the mentor flag with yours.
Flags: needinfo?(debloper)
Sounds good, yup. Thanks!
Mentor: debloper → dholbert
Attachment #8385561 - Attachment description: Patch for Bug 978044 → patch attempt, using CSS (unfortunately doesn't work)
Attachment #8385561 - Attachment is obsolete: true
Hey I want to work on this bug. Any suggestions where to start?
Hi! Thanks for your interest. I'm actually un-flagging this as a mentored bug -- I think you probably want to pick another bug to start with.

This was originally marked as 'mentored' because the original mentor thought it'd just be a 1-line CSS tweak, but it unfortunately turned out to be more complicated than that.  Moreover, the existing code (that needs adjusting) is broken in several ways, which makes the fix less staightforward (per comment 32), and which also makes it hard to tell whether the suspected-fix will actually fix it or have side effects.  I think the things that a new contributor would need to struggle with & the structures they'd need to learn about to solve this bug are not particularly useful for getting them off the ground.

So, since I think I know what needs changing to fix this (as I've gone over a bit in misc. previous comments): I'm just going to spin up a fix for this myself, rather than trying to summarize the required change for a mentee to implement.

Thanks again for the interest, though! (And thanks to debloper and chirag for their earlier work here.)
Mentor: dholbert
Whiteboard: [lang=c++] [see comment 23 & comment 32 for what probably needs to happen] → [see comment 23 & comment 32 for what probably needs to happen]
Assignee: nobody → dholbert
As I suspected, there are more rabbit-holes remaining here. :)

The last (?) one seems to be that the print-preview document has 80 app units per pixel, whereas normally we have 60 app units per pixel. For some reason, this ends up throwing off the centering code that I've suggested thus far.

Here's a patch with the centering code that I think is needed, *and* with a hack to force the app units per dev pixel to 60 (on the device context). This works, though it doesn't work if I take out the hack.

I think there might be a bug with establishing the viewport size here. I'm not going to poke further into this at the moment, but I'll hopefully circle back to it before too long.
(Reposting to drop an unnecessary parameter that I'd accidentally left in on my ComputeCenteringMargin() function.)
Attachment #8500677 - Attachment is obsolete: true
Whiteboard: [see comment 23 & comment 32 for what probably needs to happen]
I've resolved the issue in comment 45. Basically, what happens is:
 - The entire print-preview UI (up to its container scrollframe) has AppUnitsPerDevPixel = 80 (on my system at least).  Outside of that, I have AppUnitsPerDevPixel = 60.

 - The print-preview document's PresContext has a special scale-factor stored on it (& available via GetPrintPreviewScale()) to correct for this, to let us render the content at approximately the same size that it would render in a normal browser-tab.  In my case, that scale factor is 1.3333[...], which is 80/60.

 - This scale factor gets applied at display-list-creation time, by the nsSimplePageSequenceFrame, to its child frames. That happens here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=3ac1b309271d#802

So -- to visually center the page frames in their parent (the nsSimplePageSequenceFrame), we have to figure out how big *they're actually going to render* when scaled up, and subtract *that* size from the parent's size to get the centering offset, and then scale the resulting offset back down. (so that it'll be scaled up to produce the right amount when we're painting).

Patch coming up to do that.
Attached patch fix v3Splinter Review
mats, see comment 47 for an explanation of the scaling that's going on here. (The comments in the code hopefully explain it reasonably well, too.)

If you're curious why we need to calculate this centering offset in two different places, see comment 32. (I went with option (C) from the possibilities that I laid out at the end of that comment.)

And if you're curious why we can't just do this centering in CSS, see comment 20 - 21.
Attachment #8500849 - Flags: review?(mats)
Here's a screencast of how print-preview looks after this.
Also, for comparison, I tested the following other browsers, which all center-align their print-preview views:
 - Chrome 39 on my Linux system (it's got some controls on the left side, and then the doc is centered in the remaining space on the right)
 - IE11 on Windows 8.1
 - Safari 7 on Mac (via "Print | click PDF menu | Open PDF in Preview")
 - Firefox on Mac (via same "Open PDF in Preview" path) (Preview is an external program)
Comment on attachment 8500849 [details] [diff] [review]
fix v3

r=mats with a few minor nits - fix as you see fit:

>layout/generic/nsSimplePageSequenceFrame.cpp
>+nscoord
>+nsSimplePageSequenceFrame::
>+  ComputeCenteringMargin(nscoord aContainerContentBoxWidth,

The indentation here is a bit unusual.  I'd prefer to keep the method
name on the same line, like so:

nsSimplePageSequenceFrame::ComputeCenteringMargin(
  nscoord aContainerContentBoxWidth,

because I often search (emacs/less/grep) for method definitions
by "::Method".  Also, I think the above is what I've seen elsewhere
in similar situations.

>+  // We'll be centering our child's marign-box, so get the size of that:

margin-box

>+  nscoord childMarginBoxWidth =
>+    aChildPaddingBoxWidth + aChildPhysicalMargin.LeftRight();
>+
>+  // When rendered, our child's rect will actually be scaled up by the
>+  // print-preview scale factor, via ComputePageSequenceTransform().
>+  // We really want to center *that scaled-up rendering* inside of
>+  // aContainerContentBoxWidth.  So, we scale up our margin-box here...

"our margin-box" is a slightly ambiguous, is "its margin-box" better?
(since it's the child's margin box, not "our" in the sense of "this"?)

> nsSimplePageSequenceFrame::Reflow
>+    if (mRect.Width() != aDesiredSize.Width()) {

s/mRect/GetRect()/

>+        // Adjust child's x-position:
>+        child->MovePositionBy(nsPoint(...

"Adjust the child's" reads a little better, but I guess you're
referring to the variable name there rather than the English word.

>+  /* XXXdholbert*/
>   content.AppendNewToTop(new (aBuilder)

Intended?  If so, please describe the problem.
Attachment #8500849 - Flags: review?(mats) → review+
All good points, thanks! Fixed locally.

(In reply to Mats Palmgren (:mats) from comment #51)
> >+  /* XXXdholbert*/
> >   content.AppendNewToTop(new (aBuilder)
> 
> Intended?  If so, please describe the problem.

Nope, not intended -- left over from debugging. (I had locally commented-out the nsDisplayTransform that's being appended here, because that removes the need for the scale-up/scale-down centering dance; but then I forgot to drop my placeholder comment.)
(I'm giving this a Try run because this does impact the rendering of "reftest-print" reftests -- their 3x5 cards will now be centered in the reftest window, since it uses a print-preview-style rendering. This shouldn't break any tests, because it affects the reference cases in the same way as the testcases. I sanity-checked locally that the reftest renderings appear to be correctly centered in the reftest window, during a run of ./mach reftest layout/reftests/printing/.)
https://hg.mozilla.org/mozilla-central/rev/04c92cdfcde4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.