Closed Bug 1951724 Opened 9 months ago Closed 4 months ago

Print Preview UI doesn't update the print scaling value (despite using it for the rendering) after switching to a print target that has a saved `.print_scaling` value

Categories

(Toolkit :: Printing, defect, P3)

defect

Tracking

()

RESOLVED FIXED
142 Branch
Tracking Status
firefox142 --- fixed

People

(Reporter: dholbert, Assigned: jtech3029, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [recomp] [lang=js])

Attachments

(2 files)

STR (pref-twiddling version):

  1. Set these 3 prefs (either manually, or by copypasting them into an about:config
user_pref("print.more-settings.open", true);
user_pref("print.printer_Mozilla_Save_to_PDF.print_scaling", "0.5");
user_pref("print.printer_Mozilla_Save_to_PDF.print_shrink_to_fit", false);
  1. Visit https://example.org/ (or any page)
  2. Ctrl+P / Cmd+P to open Print Preview dialog
  3. Choose the "Save to PDF" print-target, and look at the scale in the preview rendering vs. the scale shown in the print UI

ACTUAL RESULTS:
The preview rendering is at 50% scale (using the value from prefs), but the UI says we're using 100% scaling.

EXPECTED RESULTS:
The "Scale:" box in the UI should reflect the actual scaling value that we're using.

Aha -- so really, the issue here is that the Scale value that's shown in the UI does not get updated when you change print targets.

So if you have multiple print targets, and you've got a nondefault saved Scale for any (or all) of them in about:config (from a previous print operation, whether it was one that you acted on or canceled out of), then: we'll show the correct Scale value when the print-preview dialog comes up, but we show the wrong one (i.e. we fail to update the value that we're reporting) when you change your print target to pick a different printer.

STR (user-facing version that illustrates comment 1, no manual about:config interaction required):

PREREQUISITE: You need to have more than 1 print target.

  1. Visit https://example.org/ (or any page)
  2. Ctrl+P / Cmd+P to open Print Preview dialog. Let's call the chosen print target "Print Target A"
  3. Expand "More Settings"
  4. Under "Scale", choose the radio button for "Scale" (not Fit to Page Width)
  5. Set the numeric value to 50% (the lowest possible value).
  6. Now choose some other print target (which I'll call Print Target B)
  7. Adjust the Scale numeric value to 200% (the highest possible value).
  8. Click "Cancel".
  9. Ctrl+P / Cmd+P to open Print Preview dialog again. (It should be showing Print Target A with restored Scale:50% that matches the rendering).
  10. Change the print target to Print Target B.

ACTUAL RESULTS:
The rendering updates to show 200% scaling, but the UI still shows "Scale: 50%"

EXPECTED RESULTS:
The Scale shown in the UI and in the rendering should be consistent.

Summary: Print Preview UI doesn't show the saved print scaling value (despite using it for the rendering) → Print Preview UI doesn't update the print scaling value (despite using it for the rendering) after switching to a print target that has a saved `.print_scaling` value
Attachment #9469768 - Attachment description: screencast of bug (starting with fresh profile) → screencast of bug (starting with fresh profile) using Comment 1 STR; buggy rendering is visible after t=33s

I can reproduce this bug (with both sets of STR) in Nightly 2020-12-01 (which is close to as far back as we have our tab-modal print dialog enabled)

So: not a recent regression.

I can confirm the STR. It looks like checking this._percentScale.value != parseInt(scaling * 100, 10) and going through the setter code should fix this https://searchfox.org/mozilla-central/rev/cb46268bc26b0cd9e91e625aa92aaa5a6f047b9d/toolkit/components/printing/content/print.js#2047

This could also use a test that should be able to be implemented similar to the STR listed in comment 2 (or by manually setting prefs as is done in comment 1) https://searchfox.org/mozilla-central/source/toolkit/components/printing/tests/browser_print_scaling.js

Mentor: tgiles
Severity: -- → S3

To help Mozilla out with this bug, here's the steps:

  1. Comment here on the bug that you want to volunteer to help.
    This will tell others that you're working on the next steps.
  2. Download and build the Firefox source code
    • If you have any problems, please ask on Element/Matrix in the #introduction channel. They're there to help you get started.
  3. Start working on this bug.
    • You will need to change this._percentScale.value to `this._percentScale.value != parseInt(scaling * 100, 10) on line 2047 in print.js
    • You will also need to add a test to verify this new change. This new test will be a new task that is added to browser_print_scaling.js and will have a similar setup to the existing task in that file. You won't need to assert that the settings changed like in line 16-22, but the percentScale variable will be helpful as well as the await helper.text(percentScale, "9") line (you will need to change the value of "9" to the appropriate value).
      • You should set the default printer's scale value to 50% and the "Fake Printer" scale value to 200%. After doing this, you can use this closing function snippet to force the print dialog to close via the cancel button. Note, you don't need to include the helper.doc.l10n lines for your case.
      • After closing the dialog via the cancel button, you will need to verify that the default printer's scale value is 50% and the "Fake Printer" scale value is 200%. This is where the helper.assertSettingsNotChanged() function will be needed.
    • If you have any problems with this bug, please comment on this bug and set the needinfo flag for me by using the "Request information from" checkbox and setting the dropdown select to "mentor". Also, you can find me and my teammates on the #reusable-components channel on Element/Matrix most hours of most days.
  4. Build your change with mach build and test your change with mach test toolkit/components/printing/tests --headless. Also check your changes for adherence to our style guidelines by using mach lint.
  5. Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
  6. After a series of reviews and changes to your patch, I'll push it to autoland.
  7. ...now you get to think about what kind of bug you'd like to work on next.
    Let me know what you're interested in and I can help you find your next contribution.
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [recomp] [lang=js]

hey im interested in working on this bug as my first bug. Thank you for the comprehensive guide!

Hi! I could use some help on creating the test case for the bug. I have already made changes to print.js and have looked through head.js to try and create the test case, but I am a bit confused on how I can check the values of the default and fake printer after closing it. I would assume I reopen the print dialogue after closing it like the video demonstration did, but what would be the proper command to do so? Afterwards, I would switch from the default to the fake printer using helper.dispatchSettingChange and assert something like is(percentScale.value, 2050", "Scale value of fake printer is 200"); correct?

Flags: needinfo?(tgiles)

(In reply to jtech3029 from comment #9)

Hi! I could use some help on creating the test case for the bug. I have already made changes to print.js and have looked through head.js to try and create the test case, but I am a bit confused on how I can check the values of the default and fake printer after closing it. I would assume I reopen the print dialogue after closing it like the video demonstration did, but what would be the proper command to do so? Afterwards, I would switch from the default to the fake printer using helper.dispatchSettingChange and assert something like is(percentScale.value, 2050", "Scale value of fake printer is 200"); correct?

meant to do 200 instead of 2050

(In reply to jtech3029 from comment #9)

Hi! I could use some help on creating the test case for the bug. I have already made changes to print.js and have looked through head.js to try and create the test case, but I am a bit confused on how I can check the values of the default and fake printer after closing it.

Thanks for the needinfo, and thanks for taking this on! Let's see how we can solve this test issue.

I would assume I reopen the print dialogue after closing it like the video demonstration did, but what would be the proper command to do so?

It seems like using await helper.startPrint(); will open the dialog again. Went through the stack of startPrint() and that seems like the best option for us here.

Afterwards, I would switch from the default to the fake printer using helper.dispatchSettingChange and assert something like is(percentScale.value, 2050", "Scale value of fake printer is 200"); correct?

I believe you're on the right track. You may need to use await helper.openMoreSettings() again to get access to the percent scale value in the new dialog.

If you get stuck trying to figure out the test, you can also submit a work-in-progress (WIP) patch to phabricator by running moz-phab submit --wip. This will submit a draft patch to phabricator, but you'll need to needinfo me that you've done this as phabricator doesn't send notifications for draft patches. More than happy to help debug and figure out what we need for the test!

(setting the needinfo just to make sure you get this notification)

Flags: needinfo?(tgiles) → needinfo?(jtech3029)

fixed an issue where print preview ui wouldn't update scaling value after
exiting, reentering, and switching printers with two different scaling values.
Partially finished a test case to check for this edge case.

Assignee: nobody → jtech3029
Status: NEW → ASSIGNED

Hi thanks for responding and helping me out! I could use a bit more help though since I am facing some trouble. The printer isn't switching because I think helper.startPrint() isn't working. When I ran mach test, one of the exceptions thrown was Error: Tab-modal print UI already open at _openTabModalPrint@chrome://global/content/printUtils.js:184:13 which I believe means that the print dialog is already open(correct me if I am wrong). And, I have a test checking whether the destinationPicker element was available which it is not which I believe is why the test is not switching from one printer to the other and failing. Perhaps the dialog is hidden rather than fully closed and requires something to reveal it?

I submitted a WIP patch to phabricator and have left comments on the code I created to explain my thought process for the test case. I also had another question since I noticed on your original guide you suggested using assertSettingsNotChanged(), but I was a bit confused on how to implement it and whether or not assertSettingsNotChanged() would be the best option. Looking forward to hearing from you!

Flags: needinfo?(jtech3029) → needinfo?(tgiles)

Hey jtech, I just left a detailed comment on your patch in Phabricator. When the patch is in WIP, I don't think anyone get notified of comments on the patch so setting the needinfo to make sure you get the notification. Please feel free to reach out to me with more questions and I'll be happy to assist!

The printer isn't switching because I think helper.startPrint() isn't working. When I ran mach test, one of the exceptions thrown was Error: Tab-modal print UI already open at _openTabModalPrint@chrome://global/content/printUtils.js:184:13 which I believe means that the print dialog is already open(correct me if I am wrong).

Your analysis is correct, the print dialog was still open when you called await helper.startPrint(). This is because we didn't move the document focus to the cancel button before sending the "Return" key to the print dialog window. I believe the focus is set on the primary button by default, so sending "Return" will cause a file picker to appear if we were on the "Save to PDF" printer and probably cause no UI to appear if we're on the mock printer. I left a comment on Phabricator explaining this as well.

I also had another question since I noticed on your original guide you suggested using assertSettingsNotChanged(), but I was a bit confused on how to implement it and whether or not assertSettingsNotChanged() would be the best option

I believe your approach of checking the percent scale values by using is() works just fine, so I would stick with that and not worry about how to use assertSettingsNotChanged() at the moment.

Thanks for the questions!

Flags: needinfo?(tgiles) → needinfo?(jtech3029)

Hi thank you for your detailed response! I've managed to get passed the issues I faced initially but I've run into two new problems: the percentScale value isn't being stored when closing the dialog except under some weird conditions, and the fake printer is consistently gone when exiting and reopening the dialog. Could you please help me try and diagnose why the tests are running incorrectly?
For the percentScale value not being stored, it appears that the print dialog is resetting the value back to 100 after exiting. I amended my commit with the new changes you suggested and some other ones as well. I added a while loop to act as a quasi breakpoint to allow me to try and debug why this was occurring. The best piece of information I was able to obtain was that doing this "breakpoint" before the switch to the fake printer and exiting would save the value. However, deleting the code that involves switching to the fake printer and setting it to 200 yields the same reset back to 100 which rules out the possibility of it causing the issue. For further information, I would run the tests without the --headless argument and manually input the steps of the tests.
As for the fake printer not occurring after the switch, it consistently would disappear after exiting the printer dialog no matter what I attempted to do. One of the errors thrown is that printerInfo is undefined which lines up with this assertion. Every quasi breakpoint I attempted to try always lead to the same result of it disappearing.
I hope that was comprehensive enough to help you (apologies if it wasn't) and I hope to hear from you soon!

TLDR: fake printer keeps disappearing and percentScale keeps resetting during the tests, both after exiting the print dialog.

Flags: needinfo?(jtech3029) → needinfo?(tgiles)
Attachment #9497446 - Attachment description: WIP: Bug 1951724 Fixed Print Preview UI not updating scale value r=tgiles → Bug 1951724 Fixed Print Preview UI not updating scale value r=jwatt
Attachment #9497446 - Attachment description: Bug 1951724 Fixed Print Preview UI not updating scale value r=jwatt → WIP: Bug 1951724 Fixed Print Preview UI not updating scale value r=tgiles,jwatt
Attachment #9497446 - Attachment description: WIP: Bug 1951724 Fixed Print Preview UI not updating scale value r=tgiles,jwatt → Bug 1951724 Fixed Print Preview UI not updating scale value r=tgiles,jwatt
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → INVALID

clearing need info

Flags: needinfo?(tgiles)
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Status: REOPENED → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 142 Branch
QA Whiteboard: [qa-triage-done-c143/b142]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: