1865172 - 'page-orientation' is also applying rotation to the page after the one it is set on in Google Docs

Status: VERIFIED FIXED

(Core :: Print Preview, defect, P2)

Description

[Jonathan Watt [:jwatt]]

I'm not sure when this started happening, but `page-orientation' rotations have started additionally applying to the page after the page that they are set on in Google Docs.

Note: Google account must have been opted in to native Firefox printing to test properly.

Examples:
The page with the text "Page 3" should not be rotated to landscape:
https://docs.google.com/document/d/1SetfZ2cx7J0BJanXnWTABzKexzwP4p-dQ--XSEAeiqs/edit

The page with "p2" should not be rotated to landscape:
https://docs.google.com/document/d/1N9kDNCdB-6YLW-Y0fLt_--9LyaN-hvFX4t-B6KoEOe0/edit

Comment 1

[Daniel Holbert [:dholbert]]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cf925ae475f57ab952364974520a3bcded6c50af&tochange=d171871eb9d27852fb8a80b30a78ff9150c649da

--> Regression from bug 1819335

(Note for release managers etc. who might be concerned about this regression: this won't affect users on the live google docs site at this point, since this is related to a google docs feature that's disabled-for-Firefox-by-default at the google account level except for a few select google accounts of engineers working on the feature.)

Comment 2

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

Set release status flags based on info from the regressing bug 1819335

Comment 3

[Daniel Holbert [:dholbert]]

Here's a simpler testcase (in that the issue happens slightly earlier in the doc):
https://docs.google.com/document/d/1oYQF2Bc2eUgMe7OQxPTaAmJxcVQgZRCay109P5BKdB0/edit

In Firefox Nightly print-preview for that^ document (properly opted in to native google docs printing), Page 2 is unexpectedly rotated to be displayed in landscape mode here, when it should be shown in portrait mode.

Comment 4

[Jonathan Watt [:jwatt]]

Dumping the frame tree shows that the following page's PageContentFrame is (erroneously) being assigned the page name of the preceeding page. Hence the 'landscape' page's style is also applying to the page after the page that it was supposed to apply to.

The following PageContentFrame is erroneously being given the previous page's page name if the frame for the previous page has an empty PrincipalChildList(). So if the div only contains an absolutely positioned children, for example, as we have when printing the pages of a Google doc.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

comment 4 inspired me to write this reduced testcase.

In Firefox, this renders with all the content on page 1. In Chrome, "page 2" ends up on page 2, as it should.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (further reduced)

Here's a testcase where the page:a div is literally empty, just to simplify things a bit further (empty empty PrincipalChildList() as jwatt mentioned).

We still put all the content on page 1, when in fact the content that follows that div should go on page 2.

Comment 7

[Jonathan Watt [:jwatt]]

Yeah, that seems to be a different bug, I think. I'll attach a reduced testcase that reproduces the bug that we're getting on Google docs.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: Reduced testcase from google docs

Comment 9

[Jonathan Watt [:jwatt]]

I talked with Emily about this, and since this is a page name issue as opposed to a page-orientation issue, she'll work on this.

Comment 10

[Jonathan Watt [:jwatt]]

FWIW, since I got it, the page name gets assigned to the following page when its frame is created under this stack:

 nsIFrame::GetStylePageName
 nsIFrame::ComputePageValue
 nsCSSFrameConstructor::ConstructPageFrame
 nsCSSFrameConstructor::CreateContinuingFrame
 mozilla::PrintedSheetFrame::Reflow
 nsContainerFrame::ReflowChild
 nsPageSequenceFrame::Reflow
 nsContainerFrame::ReflowChild
 nsCanvasFrame::Reflow
 nsContainerFrame::ReflowChild
 nsHTMLScrollFrame::ReflowScrolledFrame
 nsHTMLScrollFrame::ReflowContents
 nsHTMLScrollFrame::Reflow
 nsContainerFrame::ReflowChild
 mozilla::ViewportFrame::Reflow
 mozilla::PresShell::DoReflow
 mozilla::PresShell::ProcessReflowCommands
 mozilla::PresShell::DoFlushLayout(bool
 mozilla::PresShell::DoFlushPendingNotifications
 mozilla::PresShell::FlushPendingNotifications
 mozilla::PresShell::DoFlushPendingNotifications
 mozilla::PresShell::FlushPendingNotifications
 nsPrintJob::ReflowPrintObject

The ComputePageValue() call seems weird to me, since we're calling it on aPrevPageFrame to decide the following page's page name... ComputePageValue finds the most deeply nested first in-flow descendant, and returns its page name. In the gdocs case it iterates down the tree to the nsBlockFrame that corresponds to the div that has the page name set on it, and returns that page name up to nsCSSFrameConstructor::ConstructPageFrame which sets it on the next page frame, unfortunately.

If the PrincipalChildList() of the previous page frame is not empty, then presumably what happens is that the PrincipalChildList check in nsIFrame::ComputePageValue results in different behavior.

Comment 12

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1865172 Part 1 - Always store a page name value when a breakpoint is first found during block reflow.

Comment 13

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1865172 Part 2 - Add a test which includes the same structure as the documents that cause this issue.

This is more complex than what is strictly necessary to reproduce this issue,
but is reflective of a situation we keep having problems with.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/087e1d3a393f
Part 1 - Use computed page value on first reflow for all pages that don't have a pushed page name r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ab9a94d4c0bf
Part 2 - Add a test which includes the same structure as the documents that cause this issue. r=dholbert

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44081 for changes under testing/web-platform/tests

Comment 16

[Iulian Moraru]

Backed out for causing wpt failures on page-size-007-print.html.

• Push with failures

• Failure log

• Backout link

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17019e03e97c
Part 1 - Always store a page name value when a breakpoint is first found during block reflow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fc1f91bbfd85
Part 2 - Add a test which includes the same structure as the documents that cause this issue. r=dholbert

Comment 19

[Serban Stanca [:SerbanS]]

Backed out for causing wpt failures in page-name-unnamed-trailing-001-print.html

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | /css/css-page/page-name-unnamed-trailing-001-print.html | Testing http://web-platform.test:8000/css/css-page/page-name-unnamed-trailing-001-print.html == http://web-platform.test:8000/css/css-page/page-name-unnamed-trailing-001-print-ref.html

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7176374b72f
Part 1 - Always store a page name value when a breakpoint is first found during block reflow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/20489367d1be
Part 2 - Add a test which includes the same structure as the documents that cause this issue. r=dholbert

Comment 22

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c7176374b72f
https://hg.mozilla.org/mozilla-central/rev/20489367d1be

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 24

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

The patch landed in nightly and beta is affected.
:alaskanemily, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox124 to wontfix.

For more information, please visit BugBot documentation.

Comment 25

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

Reproduced the page orientation issue on Firefox 121.0a1 (2023-11-16) on macOS 14.4 by using the provided TCs (Comment 0 and Comment 8).

The issue is fixed and the pages are shown as expected on latest Firefox Nightly (2024-03-17). Tests were performed on macOS 14.4, Ubuntu 23.10 and Windows 11.