1657550 - nsDocumentViewer::PrintPreviewScrollToPage is broken

Status: RESOLVED FIXED

(Core :: Print Preview, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

The function uses BaseRect::Contains to tell which printed sheet frame is including the current scroll position, but there are gaps between each sheet frame in the print preview content, so if the scroll position is in one of the gaps, it fails.

Also since bug 1651947, the content is scaled, we need to factor the scale value there.

I suppose this should block bug 1631440.

Comment 1

[Jonathan Watt [:jwatt]]

Are you able to take this, hiro?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Yep, I will figure out a reasonable way to scroll a given position once after we agreed on bug 1657763.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

As discussed in #print Matrix, we should keep the old behavior for the old print preview UI, so I am going to copy the current behavior and use it if print.tab_modal.enabled is false.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6f1c00484e9264ffa73e369d3a4c78718e8b6fc

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657550 - Copy nsDocumentViewer::PrintPreviewScrollToPage for the old print preview UI. r?emilio

So that we can keep using the logic in the old print preview UI.

For the new print preview UI, the original PrintPreviewScrollToPage will be
modified gradually in subsequent changes.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657550 - Use `scrollTopMax` position for PRINTPREVIEW_END. r?emilio

I suppose PRINTPREVIEW_END doesn't mean the last page.

Depends on D87543

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657550 - Preserve x-axis scroll position on printPreviewScrollToPage call in any cases. r?emilio

That's what we've done for PRINTPREVIEW_PREV_PAGE, PRINTPREVIEW_NEXT_PAGE
and PRINTPREVIEW_GOTO_PAGENUM.

Depends on D87544

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657550 - Factor out the function to get the current page number in the print preview. r?emilio

We have to use the logic in the function for
nsDocumentViewer::PrintPreviewScrollToPage in subsequent changes.

Depends on D87545

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657550 - Make GetCurrentPageNumberInPrintPreview return the current sheet frame along with the current page. r?emilio

We need the current sheet frame for PRINTPREVIEW_NEXT_PAGE and
PRINTPREVIEW_PREV_PAGE cases.

Depends on D87546

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657550 - Rewrite nsDocumentViewer::PrintPreviewScrollToPage. r?emilio

This change consists of;

1. Use switch statement for the given PrintPreview Navigation type.

2. Simply iterate over the children of nsPageSequenceFrame for
   PRINTPREVIEW_GOTO_PAGENUM

3. Use GetCurrentSheetFrameAndPageNumber for PRINTPREVIEW_PREV_PAGE and
   PRINTPREVIEW_NEXT_PAGE so that it should now match
   printPreviewCurrentPageNumber
   (that means the edge case where the current scroll position is in
   the gap between pages has been fixed by this change)

4. Scroll to the position where the target frame is positioned at the center of
   the print preview scroll port in the cases of PRINTPREVIEW_PREV_PAGE,
   PRINTPREVIEW_NEXT_PAGE and PRINTPREVIEW_GOTO_PAGENUM

5. is a bit debatable but it can be now easily modified by changing
   ComputeScrollPositionFrameAtCenter later if it turns out the current way is not
   reasonable.

Depends on D87547

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d7aa93b9b82
Copy nsDocumentViewer::PrintPreviewScrollToPage for the old print preview UI. r=emilio
https://hg.mozilla.org/integration/autoland/rev/07766c2a11d0
Use `scrollTopMax` position for PRINTPREVIEW_END. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1a7fb3e1e8e8
Preserve x-axis scroll position on printPreviewScrollToPage call in any cases. r=emilio
https://hg.mozilla.org/integration/autoland/rev/364e2943979b
Factor out the function to get the current page number in the print preview. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b818ca5a2fed
Make GetCurrentPageNumberInPrintPreview return the current sheet frame along with the current page. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1b228ca5d679
Rewrite nsDocumentViewer::PrintPreviewScrollToPage. r=emilio

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4d7aa93b9b82
https://hg.mozilla.org/mozilla-central/rev/07766c2a11d0
https://hg.mozilla.org/mozilla-central/rev/1a7fb3e1e8e8
https://hg.mozilla.org/mozilla-central/rev/364e2943979b
https://hg.mozilla.org/mozilla-central/rev/b818ca5a2fed
https://hg.mozilla.org/mozilla-central/rev/1b228ca5d679