In css writing-mode:vertical-rl, Only one page printed out/preview

Hello, I get only 1 page in print preview when I load attachment 8607323 [details] in Firefox 41.0a1 buildID=20150513191426 (Orientation is Portrait, Scale is 100%, Size of paper is US Letter). On the other hand, I get 5 pages in print preview when I load attachment 8607323 [details] in Firefox 38 buildID=20150511103818 (Orientation is Portrait, Scale is 100%, Size of paper is US Letter). ------------ I get only 1 page in print preview when I load http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/page-flow-direction-002.htm in Firefox 41.0a1 buildID=20150513191426 (Orientation is Portrait, Scale is 100%, Size of paper is US Letter). On the other hand, I get 4 pages in print preview (expected result) when I load such page-flow-direction-002.htm test in Firefox 38 buildID=20150511103818 (Orientation is Portrait, Scale is 100%, Size of paper is US Letter). In both Firefox 38 and 41, I have (in about: config) layout.scrollbar.side set to 1 and layout.css.vertical-text.enabled set to true. I use Linux 3.13.0-53-generic x86_64, Qt: 4.8.6, KDE 4.13.3; Kubuntu (trusty) 14.04.02 LTS

Other paged media tests from Writing-Modes test suite where Firefox 45.0a1 buildID=20151127030231 should render 4 pages in print preview (Orientation is Portrait, Scale is 100%, Size of paper is US Letter) but where I get only 1 page: (using writing-mode: vertical-lr) http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/page-flow-direction-003.htm (using writing-mode: sideways-rl) http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/page-flow-direction-srl-004.htm (using writing-mode: sideways-lr) http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/page-flow-direction-slr-005.htm

[Tracking Requested - why for this release]: unable to print out whole page by Firefox Bug 1297097 has been landed in Nightly51, so unable to print out whole page by Firefox.

Track for 51+ as this is a printing issue. Hi :TYLin, Can you help to take a look at this one?

I cannot promise to fix this bug soon. I skim through nsSimplePageSequenceFrame::Reflow(), and it looks like the function still use width/height instead of ISize/BSize for calculation. I'm not sure how big the task is to revise paginated mode with respect to writing-mode, but it doesn't seem trivial.

Jet, can you help find someone who can work on this, or set a priority ? I realize printing doesn't have a clear owner at this point. Is the issue that you can't print more than one page, or does it print, but not show in print preview?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #6) > Jet, can you help find someone who can work on this, or set a priority ? I > realize printing doesn't have a clear owner at this point. Layout team owns Printing. We also own Vertical text, so this bugs ours. P3 > Is the issue that you can't print more than one page, or does it print, but not show in print preview? The issue is that, prior to Firefox 41, we would render vertical text as horizontal text and, though rendered incorrectly, would print all the text. Firefox 41 added correct vertical text, but doesn't handle the printing case. That's a legitimate bug. This one doesn't sound too hard to fix. Jonathan: WDYT?

We consciously didn't convert printing/pagination code to logical coordinates during the original writing-modes implementation -- as it wasn't essential to shipping initial support -- so it's not surprising that vertical-WM documents will not paginate properly. So yes, this is a legitimate bug, in that it's a gap in our implementation so far. A first-level fix here -- converting the printing-related code to logical coords -- should be fairly straightforward, I think. I'd expect that should fix the problem for simple cases where the whole document, starting from the root element, has vertical writing-mode. As CSS writing-modes points out[1], things may not work so well in more complex, mixed-direction cases, but I don't think we need to worry about that for now. Authors who write weird-shaped documents have been fairly warned in the spec! [1] https://drafts.csswg.org/css-writing-modes-3/#orthogonal-pagination

As part of fixing this bug, I noticed that the spec does not definitively state if headers and footers must be rotated when printing with say, vertical-rl writing mode. So, for now, we ignore writing modes for headers and footers and always print them with writing mode horizontal-tb. The bug to determine the correct behavior here is Bug 1332472.

Comment on attachment 8828173 [details] Bug 1166147 - Part1: Ignore writing mode while printing header and footer https://reviewboard.mozilla.org/r/105650/#review106842 This looks pretty good -- just some editorial nits. ::: layout/base/nsLayoutUtils.h:122 (Diff revision 2) > ScrollFrame > }; > > +// Flags that specify if the headers and footers added during printing > +// are to use the parent frame's writing mode or not. > +enum class HeaderFooterWritingModeFlags { This name is a bit too specific -- the point of a "Flags" parameter to a function is to allow *general* customization of a function's behavior, and then the flags can have custom names for what they enable/disable. SO: Please rename this enum "DrawStringFlags", and update its documentation to just say "Flags to customize the behavior of nsLayoutUtils::DrawString" ::: layout/base/nsLayoutUtils.h:124 (Diff revision 2) > > +// Flags that specify if the headers and footers added during printing > +// are to use the parent frame's writing mode or not. > +enum class HeaderFooterWritingModeFlags { > + eDefault = 0x0, > + eIgnoreStyleWritingMode = 0x1 This name isn't quite right either -- let's rename this to "eForceHorizontal", and insert a comment on the line before it saying: // Forces the text to be drawn horizontally. My reasoning: this flag does more than make us ignore the writing mode -- it also makes us actively call aFontMetrics.SetVertical(false), which stomps on whatever was in that structure before. So if someone called this method with a nsFontMetrics that had |mVertical = true|, this flag would override that via the SetVertical(false) call. (And this happens independent of the writing-mode value.) (TANGENT: This stomping seems maybe a bit heavy-handed -- maybe we should trust whatever's inside the nsFontMetrics? -- but I'm comfortable with it since it's already what we do right now, unconditionally [via respecting the writing-mode, which happens to always be horizontal right now]. So I don't think there could be any cases that this nsFontMetrics-stomping would break, so I'm OK with it.) ::: layout/base/nsLayoutUtils.h:1617 (Diff revision 2) > - static void DrawString(const nsIFrame* aFrame, > - nsFontMetrics& aFontMetrics, > - nsRenderingContext* aContext, > - const char16_t* aString, > - int32_t aLength, > - nsPoint aPoint, > - nsStyleContext* aStyleContext = nullptr); > + static void DrawString(const nsIFrame* aFrame, > + nsFontMetrics& aFontMetrics, > + nsRenderingContext* aContext, > + const char16_t* aString, > + int32_t aLength, > + nsPoint aPoint, > + nsStyleContext* aStyleContext = nullptr, After the enum is renamed to DrawStringFlags as I suggest above, you probably won't need to reindent all of these lines. (or not quite as much) ::: layout/base/nsLayoutUtils.h:1624 (Diff revision 2) > + HeaderFooterWritingModeFlags aFlags = > + HeaderFooterWritingModeFlags::eDefault); After the rename, this won't need to be wrapped to 2 lines anymore -- it should fit on 1 line just fine. ::: layout/base/nsLayoutUtils.cpp:5742 (Diff revision 2) > - nsFontMetrics& aFontMetrics, > - nsRenderingContext* aContext, > - const char16_t* aString, > - int32_t aLength, > - nsPoint aPoint, > - nsStyleContext* aStyleContext) > + nsFontMetrics& aFontMetrics, > + nsRenderingContext* aContext, > + const char16_t* aString, > + int32_t aLength, > + nsPoint aPoint, > + nsStyleContext* aStyleContext, (As above, you won't need to reindent this anymore (or not as much) after the enum rename) ::: layout/base/nsLayoutUtils.cpp:5752 (Diff revision 2) > + bool ignoreWritingMode = !!(aFlags & HeaderFooterWritingModeFlags:: > + eIgnoreStyleWritingMode); > + > + if (!ignoreWritingMode) { > - aFontMetrics.SetVertical(WritingMode(aStyleContext).IsVertical()); > + aFontMetrics.SetVertical(WritingMode(aStyleContext).IsVertical()); > + } > + else { > + aFontMetrics.SetVertical(false); > + } Since this is just used once, let's just drop the variable and perform the boolean check directly at the usage site. And while you're at it, let's flip the "if" so that there are fewer negations involved in the logic, so that it's easier to reason about. So, this should be something like: if (aFlags & DrawStringFlags::eForceHorizontal) { aFontMetrics.SetVertical(false); } else { aFontMetrics.SetVertical(WritingMode(aStyleContext).IsVertical());

Comment on attachment 8828528 [details] Bug 1166147 - Part2: Convert physical co-ordinates to logical co-ordinates for nsSimplePageSequenceFrame. https://reviewboard.mozilla.org/r/105892/#review106852 Commit message nit: > Bug 1166147 - Part2: Fix printing for vertical writing modes by overriding GetWritingMode() to pull correct writing mode from child frames r?dholbert How about we replace the second half (after "by") "...by overriding GetWritingMode() on page frames to defer to root element" That's a bit shorter, and a bit clearer about the intent of the change. (In the current commit message, a reader may be left wondering why we expect that child frames would have the "correct" writing mode, in general. And indeed, they don't -- it's just that in these cases, they're simply our path to the root element.) ::: layout/generic/nsCanvasFrame.cpp:185 (Diff revision 2) > +mozilla::WritingMode > +nsCanvasFrame::GetWritingMode() const For brevity/consistency, let's drop the mozilla:: prefix here (as well as in all of the other .cpp files where you're adding this). (Normally we only bother with the mozilla:: prefix in .h files, where we forbid 'using' declarations.) ::: layout/generic/nsCanvasFrame.cpp:188 (Diff revision 2) > + nsIContent* rootElem = GetContent(); > + if (rootElem) { > + nsIFrame* rootElemFrame = rootElem->GetPrimaryFrame(); > + if (rootElemFrame) { Sorry, one more "split" request -- could you spin off a "part 1.5" patch that is *just* about moving nsCanvasFrame::GetWritingMode to the .cpp file, without changing its behavior? And then this main patch would simply add the new printing-specific logic to it. (The numbering is aribtrary; they could be parts 2 and 3 if you prefer. :)) It's best not to mix (non-functional) code-moves together in the same patch as code-rewrites. Hypothetically, if this bug ends up causing a regression, it'll be much easier to identify what the problem is if we can view *just* the functional changes (and ignore the non-functional code-moving changes). And splitting out functional vs. non-functional changes lets us do that. ::: layout/generic/nsCanvasFrame.cpp:194 (Diff revision 2) > + } else { > + /** > + * For printing, it is possible that the content node for nsCanvasFrame is > + * null. Therefore, we must pull the correct writing mode from the first A few nits: (1) Right now this function has several repeated flavors of "get root elem, then get its frame, then get its writing mode". Instead, we should more clearly separate this function's logic into two steps: - Get the root element (with some extra thoroughness if we're printing). - And then, ask it for its writing mode. (2) We should guard the printing-specific thoroughness in a printing-specific check. You can check "PresContext()->IsPaginated()", I think. So it should look like: if (!rootElem && PresContext()->IsPaginated()) { ... } (In my imagination, this ^^ should encompass all of the changes to this function in this patch, I think.) (3) The explanatory comment should be a bit clearer -- something like this: "In a printed document, each nsCanvasFrame will have a null content node (because nsCSSFrameConstructor::ConstructPageFrame Init's them with nullptr). So to get the root element, we need to ask the child (and specifically our FirstContinuation()'s child -- because if we aren't the first continuation, then we won't have any children until we pull them in, partway through our first reflow)." ::: layout/generic/nsPageContentFrame.cpp:120 (Diff revision 2) > + /** > + * For printing, it is possible that successive continuations may not have > + * the correct writing mode set on creation (as this is set during reflow). > + * Therefore, we must always pull the correct writing mode from the > + * first continuation. > + */ * "For printing" isn't really necessary here, since this is nsPageContentFrame which is clearly printing-specific. :) ("page" = printed-page) * The explanation is not quite correct here -- I don't think it's that continuations have the wrong writing-mode "set". It's just that these continuations don't have any children until they get reflowed, which means that their GetWritingMode() implementation [which defers to their children] may produce the wrong result until children have been pulled in. * Also, before discussing continuations, this comment should first mention why we're deferring to the child in the first place. (IIRC it's because we want to consistently use the *root element's* writing-mode in all of the page wrapper frames, and our child's GetWritingMode() method will give us that.) ::: layout/generic/nsPageFrame.cpp:172 (Diff revision 2) > PR_PL(("[%d,%d]\n", aReflowInput.AvailableWidth(), aReflowInput.AvailableHeight())); > > NS_FRAME_SET_TRUNCATION(aStatus, aReflowInput, aDesiredSize); > } > > +mozilla::WritingMode nsPageFrame::GetWritingMode() const nsPageFrame should be at the start of a second line here (and drop the mozilla:: specifier as noted elsewhere in this review feedback, too) ::: layout/generic/nsPageFrame.cpp:174 (Diff revision 2) > + /** > + * For printing, it is possible that successive continuations may not have > + * the correct writing mode set on creation (as this is set during reflow). My notes on nsPageContentFrame's comment all apply here, too. ::: layout/generic/nsSimplePageSequenceFrame.cpp:139 (Diff revision 2) > + nsIFrame* firstChild = mFrames.FirstChild(); > + if (firstChild) { As noted for nsPageContentFrame::GetWritingMode, you should briefly mention why we're deferring to the child here. (because we want to return the root element's writing mode for all of the page wrapper frames, our child frame's GetWritingMode impl will give us that)

Comment on attachment 8828173 [details] Bug 1166147 - Part1: Ignore writing mode while printing header and footer https://reviewboard.mozilla.org/r/105650/#review107234 r=me on part 1, thanks!

Comment on attachment 8828528 [details] Bug 1166147 - Part2: Convert physical co-ordinates to logical co-ordinates for nsSimplePageSequenceFrame. https://reviewboard.mozilla.org/r/105892/#review107236 This is nearly there! Marking r- for now, because it's probably worth giving the reftest [requested below] a review pass, once you've got one. ==== Two non-code requests: (1) The commit message here isn't quite right: > Fix printing for vertical writing modes by overriding nsCanvasFrame::GetWritingMode to defer to root element In particular: current mozilla-central already tries to do what your commit message says you're doing here. Something like this might be clearer: "Make nsCanvasFrame::GetWritingMode() more robustly defer to root element, to avoid truncating printed vertical-WM docs." (2) Please include add a reftest for this bug. You probably want something like this: <html class="reftest-print" style="writing-mode: vertical-lr"> <div style="background: teal; height: 50in; width: 50in"></div> </html> And then the reference case can probably be identical, but without the "writing-mode" set on the root element. Under the hood, in builds with your fix, the testcase will be fragmented horizontally and the reference will be fragmented vertically, but they should each produce a bunch of tiny pages, each of which is fully-teal. And in broken builds, the testcase would just produce 1 page, I think. (It might be trickier than I'm expecting, but hopefully something similar to the above will work.) ::: layout/generic/nsFrame.h:578 (Diff revision 3) > * @param aChildPseudo the child's pseudo type, if any. > */ > static nsIFrame* > CorrectStyleParentFrame(nsIFrame* aProspectiveParent, nsIAtom* aChildPseudo); > > + mozilla::WritingMode GetWritingModeDeferringToRootElem() const; Please move this to the "protected" section -- we probably don't want to expose this publicly. It's an implementation detail. ::: layout/generic/nsFrame.cpp:9135 (Diff revision 3) > + if (primaryFrame) > + return primaryFrame->GetWritingMode(); Please add curly braces for this innermost "if" statement.

(I've marked comment 21 as obsolete - per recent IRL discussions, the new function (GetWritingModeDeferringToRootElem) will have multiple callers now, so comment 21's request is no longer applicable.)

Comment on attachment 8828528 [details] Bug 1166147 - Part2: Convert physical co-ordinates to logical co-ordinates for nsSimplePageSequenceFrame. https://reviewboard.mozilla.org/r/105892/#review109442 The commit messag has some "hg fold" junk in it: MozReview-Commit-ID: FAETDfZOyfh *** Merge this with physical to logical MozReview-Commit-ID: 7kdhv0JFBt6 (That's from hg concatenating the two folded commits' commit-messages together, one after the other.) Please clean up that commit message, to drop the "***" and everything below it. ::: layout/generic/nsSimplePageSequenceFrame.h:132 (Diff revision 6) > const nsMargin& aChildPhysicalMargin); > > - > void DetermineWhetherToPrintPage(); This commit is touching this file only to remove this one blank line -- please revert that change, for brevity. (Maybe we should clean up that blank line as part of cleanup somewhere. But, better not to clutter this patch up with it, to reduce the number of files modified in this patch.) ::: layout/generic/nsSimplePageSequenceFrame.cpp:92 (Diff revision 6) > nscoord aHeight) > { > - // Aim to fill the whole size of the document, not only so we > + // Aim to fill the whole size of the document, not only so we > - // can act as a background in print preview but also handle overflow > + // can act as a background in print preview but also handle overflow > - // in child page frames correctly. > + // in child page frames correctly. > - // Use availableWidth so we don't cause a needless horizontal scrollbar. > + // Use availableWidth so we don't cause a needless horizontal scrollbar. This comment needs fixing, since you're rewriting the code that it's talking about. Probably just delete "availableWidth", and replace it with: "AvailableISize() (rather than ComputedISize())" (That's what this comment means to say, I'm pretty sure. BTW, I don't immediately see *why* we'd get a horizontal scrollbar [from using ComputedISize] as the comment is implying... but we don't have to sort that out here, necessarily.) ::: layout/generic/nsSimplePageSequenceFrame.cpp:164 (Diff revision 6) > // Return our desired size > SetDesiredSize(aDesiredSize, aReflowInput, mSize.width, mSize.height); > aDesiredSize.SetOverflowAreasToDesiredBounds(); > FinishAndStoreOverflow(&aDesiredSize); > > - if (GetRect().Width() != aDesiredSize.Width()) { > + if (GetRect().Width() != aDesiredSize.PhysicalSize().width) { I think you should revert these s/Width()/PhysicalSize().width/ sorts of conversions here. (That makes up a lot of this patch, actually.) As discussed in person -- if it's correct for us to use the width, then the old Width() accessor should do fine.) Similar for s/ComputedWidth()/ComputedPhysicalSize().width/ changes elsewhere in this patch -- those can be reverted since they're equivalent. (The width->isize / height->bsize changes elsewhere are *not* equivalent, of course -- those are making the code more generic -- so *those* ones are all likely worth keeping!) So I think that means you should probably just leave nsSimplePageSequence::Reflow untouched (aside from reindenting the args in the function-signature, which is fine as tangential cleanup in this patch.) ::: layout/generic/nsSimplePageSequenceFrame.cpp:169 (Diff revision 6) > nscoord centeringMargin = > - ComputeCenteringMargin(aReflowInput.ComputedWidth(), > + ComputeCenteringMargin(aReflowInput.ComputedPhysicalSize().width, (We generally indent by 2 spaces after an assignment, like the old version of this code does. So, you'd want to reindent this to match the old indentation. BUT, really, I think these changes can just be reverted altogether as noted above. :)) ::: layout/generic/nsSimplePageSequenceFrame.cpp:251 (Diff revision 6) > nsPageFrame * pf = static_cast<nsPageFrame*>(kidFrame); > pf->SetSharedPageData(mPageData); > > // Reflow the page > ReflowInput kidReflowInput(aPresContext, aReflowInput, kidFrame, > - LogicalSize(kidFrame->GetWritingMode(), > + LogicalSize(wm,pageSize)); While you're modifying this line, please fix its indentation as well (so that it's aligned under aPresContext). Also, please add a space before "pageSize". ::: layout/generic/nsSimplePageSequenceFrame.cpp:256 (Diff revision 6) > - PR_PL(("AV W: %d H: %d\n", kidReflowInput.AvailableWidth(), kidReflowInput.AvailableHeight())); > + PR_PL(("AV W: %d H: %d\n", kidReflowInput.AvailableISize(), > + kidReflowInput.AvailableBSize())); The "W" and "H" in this logging-statement's string here must refer to "Width" and "Height". (And AV means Available, I guess) I don't know if anyone uses this logging, but you should change it to "ISize:" and "BSize:" so that it continues to reflect reality. ::: layout/generic/nsSimplePageSequenceFrame.cpp:268 (Diff revision 6) > - x += ComputeCenteringMargin(aReflowInput.ComputedWidth(), > - kidSize.Width(), pageCSSMargin); > + x += ComputeCenteringMargin(aReflowInput.ComputedPhysicalSize().width, > + kidSize.PhysicalSize().width, pageCSSMargin); > As above, this can be reverted. (The comment above it indicates that it's an explicitly physical operation -- centering it *horizontally* -- so it's fine&good that we're dealing with explicit widths rather than ISizes.) ::: layout/generic/nsSimplePageSequenceFrame.cpp:271 (Diff revision 6) > + /* Cannot replace this call with the logical counterpart because page frames > + must be arranged horizontally irrespective of the writing mode > + */ This comment feels a bit out of place / overly specific (about "this call"), since really *most* of this file can't be replaced with its logical counterpart. I'd suggest replacing with a comment like this, either here or at the top of this function: "Note: we largely position/size out our children (page frames) using *physical* x/y/width/height values, because the print preview UI is always arranged in the same orientation, regardless of writing mode."

Comment on attachment 8828528 [details] Bug 1166147 - Part2: Convert physical co-ordinates to logical co-ordinates for nsSimplePageSequenceFrame. https://reviewboard.mozilla.org/r/105892/#review109472 Looks like the commit message still has Mercurial "fold" junk in it. (a "***" followed by some bonus commit message stuff). Please do edit the commit message to delete the *** and everything below it. r=me with that & the following: ::: layout/generic/nsSimplePageSequenceFrame.cpp:261 (Diff revision 7) > - PR_PL(("AV W: %d H: %d\n", kidReflowInput.AvailableWidth(), kidReflowInput.AvailableHeight())); > + PR_PL(("AV ISize: %d BSize: %d\n", kidReflowInput.AvailableISize(), > + kidReflowInput.AvailableBSize())); The indentation seems to be arbitrary on this second line here, so this needs some fixup. (at a minimum, deindenting that line back to align with the first arg, the open-quote of the format-string). I'd say this would be optimally readable with a linebreak after the format string, like so: PR_PL(("AV ISize: %d BSize: %d\n", kidReflowInput.AvailableISize(), kidReflowInput.AvailableBSize())); (And if you like, a linebreak between the latter two args, as well).

Comment on attachment 8831818 [details] Bug 1166147 - Part3: Override GetWritingMode() and make it more robustly defer to root element, to avoid truncating printed vertical-WM docs. https://reviewboard.mozilla.org/r/108336/#review109474 r=me with one change: ::: layout/generic/nsPageContentFrame.h:47 (Diff revision 3) > * > * @see nsGkAtoms::pageContentFrame > */ > virtual nsIAtom* GetType() const override; > - > + > + virtual mozilla::WritingMode GetWritingMode() const override Please remove the "virtual" keyword from all your added .h-file chunks in this patch. It's redundant given that these are already tagged as "override". See this (recent-ish) Coding Style guideline: "Method declarations must use at most one of the following keywords: virtual, override, or final." https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices We clearly haven't fixed up existing code to conform to this yet (e.g. in the contextual code here), but we should adhere to it in new code.

Comment on attachment 8831819 [details] Bug 1166147 - Part4: Add a reftest for this bug. https://reviewboard.mozilla.org/r/108338/#review109478 r=me with the reftest comment expanded upon a bit. (It's important to explain the test clearly, because it's depending very strongly on some very precise assumptions about page-sizing -- so we need to make sure those assumptions are clearly expressed, so that we can quickly update this test if we happen to modify the reftest harness down the line to use slightly-different page sizes, for example) ::: layout/reftests/printing/1166147.html:3 (Diff revision 3) > +This test checks if content is pagenated correctly in the horizontal > +direction when printing. The test passes if the pages generated > +are identical for both vertical-lr and horizontal-tb writing-modes. Three things: (1) Typo: s/pagenated/paginated/ (one "e" wants to be "i") (2) Right now, this comment seems to be saying that it's testing the *exact same* content, paginated vertically vs. paginated horizontally. I think that's originally what we'd envisioned, but it's not actually how this reftest ended up -- we've now got different sizing in the testcase vs. the reference case. So, please reword to avoid giving that mistaken impression. (3) You should mention that the block-sizes (width:12in here, height:6in in the reference) are specifically chosen to *exactly* fill however many pages, and hint at how that math works out for the 3x5in sizes of our reftests. (It's not actually clear to me quite how it works right now, since the pages are 3x5inches, which don't directly correspond to the sizes you've got here. I'm guessing that after subtracting page-margins, our reftest-print medium is 4in wide and 2in tall -- is that correct? If so, please be sure to mention that here.)

Comment on attachment 8831819 [details] Bug 1166147 - Part4: Add a reftest for this bug. https://reviewboard.mozilla.org/r/108338/#review109486 ::: layout/reftests/printing/1166147-ref.html:8 (Diff revision 4) > + <body style="margin:0;"> > + <div style="background: teal; width:50in; height:6in;"> Similarly: here, I think we want "width: 4in" probably (if that's exactly the right width to fill the page area here) ::: layout/reftests/printing/1166147.html:13 (Diff revision 4) > +harness, the background color will not show since we omit printing and > +previewing of background colors by default via the browser printing path. > +--> > +<html class="reftest-print" style="writing-mode: vertical-rl;"> > + <body style="margin:0;"> > + <div style="background: teal; width:12in; height:50in;"> Also, you should reduce the "50in" here, to just exactly the amount to fill one page. So this should be "height:2in" I think. (reasoning: if we have a bug that forced us to paginate this file vertically rather than horizontally, I'd like for us to just produce one page instead of tons-of-pages. That would make the reftest failure easier to interpret. (Because in the tons-of-pages scenario, the reftest screenshot probably would only show the first ~3 pages anyway -- so that's a less-desirable failure mode than a 1-page failure mode. I think originally we were envisioning the testcase & reference case being identical for simplicity, with a huge div like "height:50in;width:50in", which is probably where this came from - but IIRC that can't work due to scrollbar-sizing-differences depending on how we're paginating. So, there's no reason for us to be using 50in in the inline dimension now.) ::: layout/reftests/printing/reftest.list:32 (Diff revision 4) > == 115199-2a.html 115199-2-ref.html > == 115199-2b.html 115199-2-ref.html > == 652178-1.html 652178-1-ref2.html > fuzzy-if(cocoaWidget,1,5000) == 745025-1.html 745025-1-ref.html > == 820496-1.html 820496-1-ref.html > +== 1166147.html 1166147-ref.html I'm also just realizing that this is the wrong spot for this new line. This file isn't perfectly sorted, but it's mostly sorted -- and this new line should go at the correct sorted spot, which is further towards the bottom.

Comment on attachment 8831819 [details] Bug 1166147 - Part4: Add a reftest for this bug. https://reviewboard.mozilla.org/r/108338/#review109486 > Similarly: here, I think we want "width: 4in" probably (if that's exactly the right width to fill the page area here) (sorry, mozreview posted my comments out of order. By "similarly" I'm referring to my other comment about 50in, which [when I wrote the comments] was immediately before this one rather than immediately after it. Darned MozReview swapping the order. :))]

Marking this bug as 'checkin-needed' - A successful try run for these patches is at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ac15e83757b17f8ed5af74fce02fdbcea674b16&selectedJob=72944913 **The only related failure you see in this try run is for the new reftest I added for this bug (layout/reftests/printing/1166147.html). This is failing because the box shadow is not generated correctly and this bug was fixed in the blocking Bug 1334680.

Comment on attachment 8831819 [details] Bug 1166147 - Part4: Add a reftest for this bug. https://reviewboard.mozilla.org/r/108338/#review109528 Thanks for adding the extra comments here! Two quick nits on this reftest patch that I noticed before triggering autoland: ::: layout/reftests/printing/1166147.html:12 (Diff revision 7) > +of pages being generated in each writing mode. > +This sizing is calculated like so: > + > +1. It is important to note that irrespective of the writing-mode, the print > + UI always lays out printed pages vertically. Therefore, it is possible > + to equate the printed content of two differnet writing modes if both Typo: s/differnet/different/ (both testcase & reference case) ::: layout/reftests/printing/reftest.list:37 (Diff revision 7) > == 966419-2.html 966419-2-ref.html > + > +== 1166147.html 1166147-ref.html > + > # asserts(3) HTTP(..) fails 1108104.html 1108104-ref.html # bug 1067755, 1135556 This new test's correct sorted position is still one line further down than where it is in this current patch. (The line below it right now is a [commented-out] line for 1108104.html, which is a smaller bug number than this new test (1166147))

Canceling checkin-needed per comment 49 - but I think this is good to go after that's addressed. [I'll be happy to trigger autoland right away after those are fixed, if you'd like - feel free to ping me.]

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5bad29d0e87b Part1: Ignore writing mode while printing header and footer r=dholbert https://hg.mozilla.org/integration/autoland/rev/2ec1e071c802 Part2: Convert physical co-ordinates to logical co-ordinates for nsSimplePageSequenceFrame. r=dholbert https://hg.mozilla.org/integration/autoland/rev/409865c5f2dd Part3: Override GetWritingMode() and make it more robustly defer to root element, to avoid truncating printed vertical-WM docs. r=dholbert https://hg.mozilla.org/integration/autoland/rev/40900d7e5bad Part4: Add a reftest for this bug. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/5bad29d0e87b https://hg.mozilla.org/mozilla-central/rev/2ec1e071c802 https://hg.mozilla.org/mozilla-central/rev/409865c5f2dd https://hg.mozilla.org/mozilla-central/rev/40900d7e5bad

I guess it's too late to fix this in 52 now. Would uplifting to aurora53 make sense? Do we have verification of the fix in nightly by Japanese users?

(In reply to Julien Cristau [:jcristau] from comment #54) > Would uplifting to aurora53 make sense? I'm hesitant to recommend uplift here, since (a) it's not a regression (b) I don't know that there's a lot of content out there that was affected by this bug (content with vertical writing-mode on root node), (c) it's not a trivial fix (so nonzero risk of regressions) (d) any hypothetical regressions may be discovered later in the game, since printing is a less-common user action. So, as long as there's no known strong need to get this fix out quickly, it's probably best to just let this ride the trains. > Do we have verification of the fix in nightly by Japanese users? Alice0775 White, would you mind verifying that this is fixed for you in Nightlies now? (The 芥川龍之介 羅生門.html testcase seems fixed to me, locally in Print Preview.)

(In reply to Daniel Holbert [:dholbert] from comment #55) > (b) I don't know that there's a lot of content out there that was affected > by this bug (content with vertical writing-mode on root node), [...meant to say: s/content/content which is likely to be printed/]

FWIW, I agree with Daniel's comment 55; I don't think uplift is justified here (small reward because it's a rare use-case, and significant risk because this is non-trivial and not heavily tested).

(In reply to Daniel Holbert [:dholbert] from comment #55) x in nightly by Japanese users? > > Alice0775 White, would you mind verifying that this is fixed for you in > Nightlies now? (The 芥川龍之介 羅生門.html testcase seems fixed to me, locally in > Print Preview.) I can verify that the problem was fixed on Nightly54.0a1(2017-02-14). Print preview and print to "Microsoft Print to PDF" are successfully performed.