Closed
Bug 978044
Opened 10 years ago
Closed 10 years ago
Firefox Print Preview should be center aligned
Categories
(Core :: Print Preview, defect)
Core
Print Preview
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: Debloper, Assigned: dholbert)
Details
Attachments
(3 files, 2 obsolete files)
5.37 KB,
patch
|
Details | Diff | Splinter Review | |
7.62 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.02 MB,
video/ogg
|
Details |
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.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lang=js][mentor=debloper@gmail.com]
Comment 1•10 years ago
|
||
hey .. can u tell me where can i find the source code for the preview page?
Reporter | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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?
Reporter | ||
Comment 5•10 years ago
|
||
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]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lang=c++][mentor=debloper@gmail.com] → [lang=c++][lang=js][lang=css][mentor=debloper@gmail.com]
Comment 6•10 years ago
|
||
Hey .... like i would like to work on this bug! I will upload the patch soon!
Comment 7•10 years ago
|
||
Attachment #8385561 -
Flags: review?(debloper)
Reporter | ||
Comment 8•10 years ago
|
||
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?
Comment 10•10 years ago
|
||
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.
Reporter | ||
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
Super, i did it! Can you assign me to bug please?
Reporter | ||
Comment 13•10 years ago
|
||
Done... up the patch, can't wait! :D \o/
Assignee: nobody → bhr.bhr.info8
Updated•10 years ago
|
Component: General → Print Preview
Product: Firefox → Core
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bhr.bhr.info8)
Version: unspecified → Trunk
Updated•10 years ago
|
Mentor: debloper
Whiteboard: [lang=c++][lang=js][lang=css][mentor=debloper@gmail.com] → [lang=c++][lang=js][lang=css]
Comment 14•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(debloper)
Comment 15•10 years ago
|
||
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?
Comment 16•10 years ago
|
||
Debloper, can you take a look at this?
Reporter | ||
Comment 17•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
I can confirm that changing the margin from "0.125in 0.25in" to "0.125in auto" does not solve the problem.
Reporter | ||
Comment 19•10 years ago
|
||
(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?
Assignee | ||
Comment 20•10 years ago
|
||
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.
Assignee | ||
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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.
Assignee | ||
Comment 23•10 years ago
|
||
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?
Reporter | ||
Comment 24•10 years ago
|
||
(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?
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Reporter | ||
Comment 26•10 years ago
|
||
(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. ;)
Assignee | ||
Comment 27•10 years ago
|
||
(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.
Reporter | ||
Comment 28•10 years ago
|
||
(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.
Assignee | ||
Comment 29•10 years ago
|
||
(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.)
Comment 30•10 years ago
|
||
(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.
Comment 31•10 years ago
|
||
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)
Assignee | ||
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
(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)
Comment 34•10 years ago
|
||
Tapping on the glass, here - Chirag, are you still working on this bug? Has Daniel's reply answered your question?
Comment 35•10 years ago
|
||
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++.
Assignee | ||
Comment 36•10 years ago
|
||
(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
Assignee | ||
Comment 37•10 years ago
|
||
(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 )
Assignee | ||
Comment 38•10 years ago
|
||
(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.)
Comment 39•10 years ago
|
||
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 :)
Assignee | ||
Comment 40•10 years ago
|
||
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]
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bhr.bhr.info8)
Reporter | ||
Comment 41•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8385561 -
Attachment description: Patch for Bug 978044 → patch attempt, using CSS (unfortunately doesn't work)
Attachment #8385561 -
Attachment is obsolete: true
Comment 43•10 years ago
|
||
Hey I want to work on this bug. Any suggestions where to start?
Assignee | ||
Comment 44•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 45•10 years ago
|
||
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.
Assignee | ||
Comment 46•10 years ago
|
||
(Reposting to drop an unnecessary parameter that I'd accidentally left in on my ComputeCenteringMargin() function.)
Attachment #8500677 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Whiteboard: [see comment 23 & comment 32 for what probably needs to happen]
Assignee | ||
Comment 47•10 years ago
|
||
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.
Assignee | ||
Comment 48•10 years ago
|
||
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)
Assignee | ||
Comment 49•10 years ago
|
||
Here's a screencast of how print-preview looks after this.
Assignee | ||
Comment 50•10 years ago
|
||
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 51•10 years ago
|
||
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+
Assignee | ||
Comment 52•10 years ago
|
||
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.)
Assignee | ||
Comment 53•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=30cd6d411991
Assignee | ||
Comment 54•10 years ago
|
||
(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/.)
Assignee | ||
Comment 55•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/04c92cdfcde4
Flags: in-testsuite-
Comment 56•10 years ago
|
||
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.
Description
•