1657763 - Refine the way to choose the current page

Status: RESOLVED FIXED

(Core :: Print Preview, enhancement, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

In bug 1657515 we are going to add nsIWebBrowserPrint.printPreviewCurrentPageNumber, but the initial implementation to choose the current page number is primitive. The way is;

1. Enumerate all pages from the first one to the last one
2. If the page intersects with the print preview scroll port, it's chosen as the current one, even if the intersection area is the bottom 1px height of the page

Emilio suggested to choose the page where a half of the page is visible (that's said, I am suspecting this way doesn't work for heterogeneous page size case, it's possible there is no page where half of it is visible)

An alternative way what I have in my mind is to choose the closest page from the center of the scroll port.

Also, TYLin noticed me that once after we supported pages per a sheet option (bug 1631452), what should the current page number be?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

mstriemer, could you please give us your insights on this issue from the frontend perspective?

Comment 2

[Jonathan Watt [:jwatt]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)

Also, TYLin noticed me that once after we supported pages per a sheet option (bug 1631452), what should the current page number be?

I think for scrolling to a page, the general expectation would be that the "page" number would be physical page (sheet) number. Actually, I've just checked what Chrome does and that is the case there.

Also I think it's shorlander that would be the person to ask about the behaviour of what page (i.e. sheet) number to choose. I also just checked what Chrome does there, and actually they seem to do what you suggest (the current page number displayed is the page that is interceted by or closest to the center of the print preview viewport). I guess that's contrary to my suggestion in bug 1657515 comment 2 (choosing the page intersected by/closest to the top edge of the viewport). But shorlander should decide this.

Comment 3

[Mark Striemer [:mstriemer]]

The middle of the viewport makes sense to me, but deferring to Stephen sounds good to me.

I put together a WIP patch in bug 1654684 to pull this value and when it's in landscape currently it won't update to the last page being current even at the bottom. I'd imagine that could happen with either of these cases though.

That patch also has a FIXME (with a stack trace) where this seems to throw an error when called immediately after the preview is updated.

Comment 4

[Jonathan Watt [:jwatt]]

Stephen seems super busy, but when I pinged him he said:

Yeah, the bottom page that crosses the 50% mark would be the most visible page “current” page.

Hopefully that allows you to proceed, hiro?

Comment 5

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

Attachment: Current Page

(In reply to Jonathan Watt [:jwatt] from comment #2)

Also I think it's shorlander that would be the person to ask about the behaviour of what page (i.e. sheet) number to choose. I also just checked what Chrome does there, and actually they seem to do what you suggest (the current page number displayed is the page that is interceted by or closest to the center of the print preview viewport). I guess that's contrary to my suggestion in bug 1657515 comment 2 (choosing the page intersected by/closest to the top edge of the viewport). But shorlander should decide this.

Yeah that sounds right to me, whichever page is the most visible page should be considered the current page.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thank you, both, I am going to take this.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1657763 - Choose the closest page to the center of the scroll port as the "current" page. r?emilio

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I've uploaded the fix for this issue, but I haven't had any great idea to write automated tests for this..

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8091fc7428c4
Choose the closest page to the center of the scroll port as the "current" page. r=emilio

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8091fc7428c4