Open Bug 1697226 Opened 1 year ago Updated 6 hours ago

Please add an option to print the current page only to the new printing UI

Categories

(Toolkit :: Printing, defect, P3)

defect

Tracking

()

ASSIGNED

People

(Reporter: julienw, Assigned: ramya, Mentored)

Details

(Keywords: regression, Whiteboard: [print2020_later] [lang=js])

Attachments

(4 files)

Attached image Current options

The new printing UI has 2 options for the "Pages" option currently: "All" and "Custom". It would benefit greatly from adding a new option "Current page", like we have on the native print dialog (at least on Linux).
Also please note that it doesn't seem to work in the native print dialog anymore (the option is disabled), so I don't see any way to do it currently. This seems like a regression to me.

Attachment #9207811 - Attachment description: image.png → Current options

Seems like a good idea. I'll throw it in the backlog, and we'll see when we can get to it.

Severity: -- → S4
Type: defect → enhancement
Priority: -- → P3
Whiteboard: [print2020_later]
Type: enhancement → defect

I would be happy to mentor if someone wanted to put a patch together to add this.

Mentor: sfoster

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

I would be happy to mentor if someone wanted to put a patch together to add this.

Cool :-) Do you know where the change should be done? Is that difficult to do cross-platform?

(In reply to Julien Wajsberg [:julienw] from comment #4)

Cool :-) Do you know where the change should be done? Is that difficult to do cross-platform?

We'd need to add the option in print.html and then something to handle it in print.js.

I don't think we currently expose the current page number, so we would need a way to get that (its in a current-page attribute on the preview <browser> element).

Whiteboard: [print2020_later] → [print2020_later] [lang=js]

please add an option to print the current page only to the new printing UI.

Assignee: nobody → tawahpeggy98
Status: NEW → ASSIGNED

Hello [:sfoster] I have submitted a work in progress patch for this.

please I searched for the FTL file to add the translation but I didn't see, I will appreciate if you can lead me to it.
also I don't quite know how to go about adding the JavaScript logic of the said functionality, please expantiate.
PS: I am new to contributing to this organization.

Hello Sam, I have made updates on the patch by adding a key to reference (Current) in printui.ftl . please check it out.
I am still trying to figure out the logic for printing the current page with respect to the way (all) and (custom) has been done, please help me with some insights.

Hi there,
Thanks for working on this. Currently the page-range control in the print UI has 2 possible values: "All" and "custom". We'd like to add a 3rd option: "Current". So, the first part is adding the new <option>. which you'll already got in your patch.

We currently represent "all pages" as an empty array on the front end, and a custom page range as an array of page numbers. I think probably the simplest solution for "current page" is to simply populate this array with the current page number. Its not very obvious right now how to get the current page number, or how that should work so let me make some suggestions:

First, poking around at the js state in the print dialog window is a bit awkward, so this is a helpful snippet to run in the browser console:

let win = PrintUtils.getPreviewBrowser(gBrowser.selectedBrowser).closest(".dialogBox").querySelector(".dialogFrame").contentWindow

that lets you more easily examine e.g. win.PrintEventHandler, win.document.querySelector("#page-range-input") etc.

The <browser> we use to display the print preview knows what the current page number is - we use that to update the preview pagination values. You can reach that via PrintEventHandler.printPreviewEl.querySelector(".printPreviewBrowser") (PrintEventHandler is global in the print.html context). To make this value available to the PageRangeInput instance, I would suggest adding it as a "virtual" property on the PrintSettingsViewProxy object. This is the settings object passed to each custom element's update() method. I.e.

diff --git a/toolkit/components/printing/content/print.js b/toolkit/components/printing/content/print.js
--- a/toolkit/components/printing/content/print.js
+++ b/toolkit/components/printing/content/print.js
@@ -1252,6 +1252,12 @@ var PrintSettingsViewProxy = {
 
   get(target, name) {
     switch (name) {
+      case "currentPageNumber": {
+        let previewBrowser = PrintEventHandler.printPreviewEl.querySelector(".printPreviewBrowser");
+        let pageNum = parseInt(previewBrowser.getAttribute("current-page"));
+        return isNaN(pageNum) ? 1 : pageNum;
+      }
+

Then, in PageRangeInput's update method:

    let { pageRanges, printerName, currentPageNumber } = settings;
    this._currentPage = currentPageNumber;

Now, when ._rangePicker's value changes to "current" you can simply set this._rangeInput.value = this._currentPage. It might look like a change to updatePageRange like this:

-    } else {
+    } else if (this._rangePicker.value == "current") {
+      this._rangeInput.value = this._currentPage;
+      this.validateRangeInput();
+    }
+    else {

I'd suggest we not show the range input when the range picker is "current". But, if the user does then select "custom", the value will be there to edit. This way we don't need to change any of the validation or any of the rest of the logic for how these range values are handled. And as the range is a single page, the user will not be able to scroll or paginate - which would change the current page number and confuse everything.

Hopefully that gets you unstuck. Eventually we'll want a new test to cover this functionality itself, but I would suggest working through getting it working first, in case we've missed something.

Flags: needinfo?(tawahpeggy98)

(In reply to Sam Foster [:sfoster] (he/him) from comment #9)

Hi there,
Thanks for working on this. Currently the page-range control in the print UI has 2 possible values: "All" and "custom". We'd like to add a 3rd option: "Current". So, the first part is adding the new <option>. which you'll already got in your patch.

We currently represent "all pages" as an empty array on the front end, and a custom page range as an array of page numbers. I think probably the simplest solution for "current page" is to simply populate this array with the current page number. Its not very obvious right now how to get the current page number, or how that should work so let me make some suggestions:

First, poking around at the js state in the print dialog window is a bit awkward, so this is a helpful snippet to run in the browser console:

let win = PrintUtils.getPreviewBrowser(gBrowser.selectedBrowser).closest(".dialogBox").querySelector(".dialogFrame").contentWindow

that lets you more easily examine e.g. win.PrintEventHandler, win.document.querySelector("#page-range-input") etc.

The <browser> we use to display the print preview knows what the current page number is - we use that to update the preview pagination values. You can reach that via PrintEventHandler.printPreviewEl.querySelector(".printPreviewBrowser") (PrintEventHandler is global in the print.html context). To make this value available to the PageRangeInput instance, I would suggest adding it as a "virtual" property on the PrintSettingsViewProxy object. This is the settings object passed to each custom element's update() method. I.e.

diff --git a/toolkit/components/printing/content/print.js b/toolkit/components/printing/content/print.js
--- a/toolkit/components/printing/content/print.js
+++ b/toolkit/components/printing/content/print.js
@@ -1252,6 +1252,12 @@ var PrintSettingsViewProxy = {
 
   get(target, name) {
     switch (name) {
+      case "currentPageNumber": {
+        let previewBrowser = PrintEventHandler.printPreviewEl.querySelector(".printPreviewBrowser");
+        let pageNum = parseInt(previewBrowser.getAttribute("current-page"));
+        return isNaN(pageNum) ? 1 : pageNum;
+      }
+

Then, in PageRangeInput's update method:

    let { pageRanges, printerName, currentPageNumber } = settings;
    this._currentPage = currentPageNumber;

Now, when ._rangePicker's value changes to "current" you can simply set this._rangeInput.value = this._currentPage. It might look like a change to updatePageRange like this:

-    } else {
+    } else if (this._rangePicker.value == "current") {
+      this._rangeInput.value = this._currentPage;
+      this.validateRangeInput();
+    }
+    else {

I'd suggest we not show the range input when the range picker is "current". But, if the user does then select "custom", the value will be there to edit. This way we don't need to change any of the validation or any of the rest of the logic for how these range values are handled. And as the range is a single page, the user will not be able to scroll or paginate - which would change the current page number and confuse everything.

Hopefully that gets you unstuck. Eventually we'll want a new test to cover this functionality itself, but I would suggest working through getting it working first, in case we've missed something.
Wow!! Thank you very much Sam.
This makes a lot of sense, let me get to it straight away and if I fall into any issues I will let you know.

Flags: needinfo?(tawahpeggy98)

Hey Julien, I also noticed the current page option is now disabled in the native dialog and looking around I'm not sure what exactly the behavior should be. I prototyped something to be able to write comment 9, where we use the current page from the print preview. As the tab modal sits on top of the content document the preview is both more visible and a more accurate representation of what the current page might look like once printed. But, setting the current page as the page range has the effect of recalculating the preview to only show that page. If you want to navigate the whole document again, you need to go back to all pages or custom range, scroll or hit next/back in the paginator and then select custom page again.

How would you expect this to work? We don't really have a way of previewing the whole document but only printing a single page at the moment, but if that's really the best answer we can file some more bugs and see if its doable. Or we can pursue the approach from comment #9 as an incremental improvement and necessary step to find the optimal behaviour. Or maybe there is a better well-defined answer already out there?

Flags: needinfo?(felash)
Type: defect → enhancement
Type: enhancement → defect
Type: defect → enhancement
Type: enhancement → defect

hello Sam i have made updates to our patch, its almost there.
i have a couple of debugging issues though, one of which is logging things to the console.

let win = >PrintUtils.getPreviewBrowser(gBrowser.selectedBrowser).closest(".dialogBox").querySelector(".dialogFra>>me").contentWindow
this line returns printUtils is not known.

Also when I try to add some code in print.js so it gets printed to the console i don't see it printed there, this has made me quite unable to debug properly. please give me heads up.

hey Sam,

sorry for the delay as I was away for a few days :-)

(In reply to Sam Foster [:sfoster] (he/him) from comment #11)

Hey Julien, I also noticed the current page option is now disabled in the native dialog and looking around I'm not sure what exactly the behavior should be. I prototyped something to be able to write comment 9, where we use the current page from the print preview. As the tab modal sits on top of the content document the preview is both more visible and a more accurate representation of what the current page might look like once printed. But, setting the current page as the page range has the effect of recalculating the preview to only show that page. If you want to navigate the whole document again, you need to go back to all pages or custom range, scroll or hit next/back in the paginator and then select custom page again.

How would you expect this to work? We don't really have a way of previewing the whole document but only printing a single page at the moment, but if that's really the best answer we can file some more bugs and see if its doable. Or we can pursue the approach from comment #9 as an incremental improvement and necessary step to find the optimal behaviour. Or maybe there is a better well-defined answer already out there?

I think that what you propose in comment 9 makes sense. I believe that if in that case only the specified page is displayed, it also makes sense. I'm looking forward to it :-)

Thanks!

Flags: needinfo?(felash)

(In reply to Tawah Peggy from comment #12)

hello Sam i have made updates to our patch, its almost there.
i have a couple of debugging issues though, one of which is logging things to the console.

let win = >PrintUtils.getPreviewBrowser(gBrowser.selectedBrowser).closest(".dialogBox").querySelector(".dialogFra>>me").contentWindow
this line returns printUtils is not known.

I suspect you are not in the right console. Make sure you are using the Browser Console not the regular devtools console. PrintUtils may need to be lazy-loaded, and certainly the preview browser will not exist until the print UI is open.

Also when I try to add some code in print.js so it gets printed to the console i don't see it printed there, this has made me quite unable to debug properly. please give me heads up.

Yeah, definitely sounds like you are looking at the wrong console.

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: tawahpeggy98 → nobody
Status: ASSIGNED → NEW

Added the option to print the current page to print UI.
Added test to check if current page is selected correctly.

Assignee: nobody → ramya.praneetha04
Status: NEW → ASSIGNED
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9f534734c7b
Added option to print current page. r=sfoster,fluent-reviewers,flod,mstriemer
You need to log in before you can comment on or make changes to this bug.