1867261 - Pages can display "both portrait and landscape" if not all pages are being printed (Range, Even or Odd selected)

Status: NEW

(Core :: Print Preview, defect)

Description

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: test file both portrait:landscape.png

Found in

• Firefox 122.0a1 (20231129050021)

Affected versions

• Firefox 122.0a1 (20231129050021)

Tested platforms

• Affected platforms: Windows 11, macOS 13.6.1, Ubuntu 22.04

Steps to reproduce

1. Launch Firefox and open the following link
2. Open Print Preview
3. From Pages, select "Even"

Expected result

• The "even" pages are shown and the pages are in portrait mode

Actual result

• The "even" pages are shown but the first page is rotated in landscape modeand kept in portrait as well (see attachment)

Additional notes

• The issue can be seen in the attachment
• The issue occurs with actual print / save to pdf

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:csasca, if you think that's a regression, could you try to find a regression range using for example mozregression?

Comment 2

[Jonathan Watt [:jwatt]]

Interesting. Thanks for catching this. We won't block shipping support since mixed orientation is relatively uncommon, and previous telemetry that we've gathered also showed printing page ranges to be very uncommon.

Note: this also occurs when setting the "Pages" field to "Custom" and entering the range "2-5", and when scrolling to the second page (the landscape page) and selecting "Current". When the "Pages" field is set, those values are more commonly chosen than "Even" or "Odd".

Comment 3

[Jonathan Watt [:jwatt]]

Implementation note: this is an issue where the nsPageFrame orientation is not being matched with the PrintedSheetFrame orientation.

Comment 4

[Jonathan Watt [:jwatt]]

This is another manifestation of bug 1869211, and should be fixed by the fix for that bug.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: Bug 1867261. Fix pages to not simultaniously display portrait and landscape. r=alaskanemily

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: Bug 1867261 p2. Add a chrome mochitest for page-orientation in print preview with a page range. r=alaskanemily

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: Mochitest rendering before patch

Comment 8

[Daniel Holbert [:dholbert]]

I can still reproduce this in Firefox 131.0a1 (2024-08-09).

jwatt, it looks like you have a patch stack here with the code-change being r+ and the test having some minor review feedback from me. Probably worth addressing the feedback and getting that landed prior to gsuite-printing getting broadly enabled, if you can.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: print-to-pdf result showing dataloss from this bug

Hmm, this isn't just a cosmetic issue during print-preview; it results in dataloss during actual print operations. If I follow the STR and proceed to print-to-PDF, I get two portrait-oriented pages, with the first one having landscape-orientation page content printed onto it, and running off the right side (and getting clipped).

(I guess this was already mentioned here -- comment 0 mentioned "The issue occurs with actual print / save to pdf" -- so I'm just adding a bit more detail on what that actually means here.)

Comment 10

[Daniel Holbert [:dholbert]]

I just retested the STR with the patch applied (the main patch does still apply cleanly, though the test patch has bitrotted and needs a rebase).

Unfortunately when I follow the STR here (while signed in to my opted-in Google account), I get this fatal assertion-failure at the moment that I switch to "Pages: even":

[41294] Assertion failure: !aFirstRenderedPage->HasAnyStateBits(NS_PAGE_SKIPPED_BY_CUSTOM_RANGE) (Sheet size should be calculated from the first rendered page), at /scratch/work/builds/mozilla-central/mozilla/layout/generic/PrintedSheetFrame.cpp:264
#01: mozilla::PrintedSheetFrame::ComputeSheetSizeFromPage(nsPageFrame*, nsPresContext const*) const (/scratch/work/builds/mozilla-central/mozilla/layout/generic/PrintedSheetFrame.cpp:262)
#02: nsPageFrame::ReflowPageContent(nsPresContext*, mozilla::ReflowInput const&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsPageFrame.cpp:0)
#03: nsPageFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsPageFrame.cpp:245)
#04: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsContainerFrame.cpp:0)
#05: mozilla::PrintedSheetFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/PrintedSheetFrame.cpp:148)
#06: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsContainerFrame.cpp:0)
#07: nsPageSequenceFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsPageSequenceFrame.cpp:368)
#08: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsContainerFrame.cpp:0)
#09: nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsCanvasFrame.cpp:710)
#10: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsContainerFrame.cpp:0)
#11: mozilla::ScrollContainerFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput&, bool, bool, mozilla::ReflowOutput*) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/ScrollContainerFrame.cpp:915)
#12: mozilla::ScrollContainerFrame::ReflowContents(mozilla::ScrollReflowInput&, mozilla::ReflowOutput const&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/ScrollContainerFrame.cpp:1072)
#13: mozilla::ScrollContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/ScrollContainerFrame.cpp:1513)
#14: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/nsContainerFrame.cpp:0)
#15: mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) (/scratch/work/builds/mozilla-central/mozilla/layout/generic/ViewportFrame.cpp:358)
#16: mozilla::PresShell::DoReflow(nsIFrame*, bool, mozilla::OverflowChangedTracker*) (/scratch/work/builds/mozilla-central/mozilla/layout/base/PresShell.cpp:9935)
#17: mozilla::PresShell::ProcessReflowCommands(bool) (/scratch/work/builds/mozilla-central/mozilla/layout/base/PresShell.cpp:10102)
#18: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (/scratch/work/builds/mozilla-central/mozilla/layout/base/PresShell.cpp:4383)
#19: nsPrintJob::ReflowPrintObject(mozilla::UniquePtr<nsPrintObject, mozilla::DefaultDelete<nsPrintObject> > const&) (/scratch/work/builds/mozilla-central/mozilla/layout/printing/nsPrintJob.cpp:1425)
#20: nsPrintJob::ReflowDocList(mozilla::UniquePtr<nsPrintObject, mozilla::DefaultDelete<nsPrintObject> > const&) (/scratch/work/builds/mozilla-central/mozilla/layout/printing/nsPrintJob.cpp:959)
#21: nsPrintJob::InitPrintDocConstruction(bool) (/scratch/work/builds/mozilla-central/mozilla/layout/printing/nsPrintJob.cpp:1000)
#22: nsPrintJob::DoCommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, mozilla::dom::Document&) (/scratch/work/builds/mozilla-central/mozilla/layout/printing/nsPrintJob.cpp:0)
#23: nsPrintJob::PrintPreview(mozilla::dom::Document&, nsIPrintSettings*, nsIWebProgressListener*, std::function<void (mozilla::dom::PrintPreviewResultInfo const&)>&&) (/scratch/work/builds/mozilla-central/mozilla/layout/printing/nsPrintJob.cpp:467)

The assertion that's failing is a newly-added assertion in this patch. So the main patch here might not be quite ready/complete after all. I'll mark it as request-changes with an inline nit about that assertion-failure, to reflect this in phab.

(Having said that: if I disable that assertion and rebuild, I end up getting EXPECTED RESULTS, so the assertion apparently isn't mission-critical at least for this bug's scenario. But presumably it might be problematic if the first and second pages differed in their actual page size (not just orientation) and the first page was skipped -- I think that's the thing that this assertion is trying to catch, which would run into trouble if the assertion fails.)