1666473 - [macOS] Paper size settings unexpectedly revert to default on actually submitting a print job

Status: VERIFIED FIXED

(Core :: Printing: Setup, defect, P2)

Description

[Jonathan Kew [:jfkthame]]

STR:

• Open the Print UI and choose a real printer as destination
• Click More Settings to reveal the Paper Size selector
• Choose a non-default paper size (e.g. A5)
• Click Cancel to dismiss the UI
• Re-open the Print UI and open More Settings again
• Observe that the chosen paper size is still selected
  • all good :)
• Click Print to submit an actual print job
• Re-open the Print UI and open More Settings again
• Observe that the paper size has reverted to a default (e.g. A4 or Letter)
  • unexpected! :(

It's unclear to me if this is a regression or has always been broken (as long as the paper size selector has been available in the new UI).

Comment 1

[Jonathan Kew [:jfkthame]]

AFAICT this has been broken ever since the paper size control was added. It looks like choosing a paper size "works" inasmuch as the preview is updated to reflect the new size, and when the print job is submitted, it will have that layout; but at the same time, the saved size (which will be the initial setting when the UI is reopened) goes back to the printer's default as recorded in the print.macosx.pagesetup-2 pref.

This also results in the wrong paper size being reported by the (pending) new telemetry in bug 1666110.

Comment 2

[Jonathan Kew [:jfkthame]]

It seems that what happens here is that the paper size we really wanted to use, which has been recorded in the base nsPrintSettings::mPaperWidth and mPaperHeight fields, is getting overridden by values retrieved from the Cocoa NSPrintInfo in nsPrintSettingsServiceX::SerializeToPrintDataParent here. If I remove that chunk of code, the issue here seems to be fixed, though I'm concerned that may result in other issues somewhere.... it probably had a purpose at some point in its life!

The paper size we get from the NSPrintInfo there is the result of calling ReadPageFormatFromPrefs() here, which retrieves a serialized PMPageFormat and applies it. That PMPageFormat in the prefs does not reflect what the user has most recently chosen in the UI.

So maybe this means we're failing to call nsIPrintSettingsService.savePrintSettingsToPrefs at the right time, or it's failing for some reason? Guess I'll poke some more...

Comment 3

[Jonathan Watt [:jwatt]]

Hah. See bug 1659928 comment 30. nsPrintDialogServiceX::Show only stores the paper size in the NSPrintInfo. So we need that serialization code currently in order to pass the correct paper size to the content process. This code is a mess though...

Comment 4

[Jonathan Watt [:jwatt]]

For now, we should probably override Get/SetPaperWidth/Height on nsPrintSettingsX and remove the chunk of code you linked to in nsPrintSettingsServiceX::SerializeToPrintDataParent so that we have one source of truth for paper width/height instead of two. That doesn't solve the issue of mPaperSizeUnit not being set by nsPrintDialogServiceX::Show that I mentioned in bug 1659928 comment 30, but we can fix that separately.

Comment 5

[Jonathan Kew [:jfkthame]]

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

For now, we should probably override Get/SetPaperWidth/Height on nsPrintSettingsX

So nsPrintSettingsX already has overrides of SetPaperWidth/Height, because it maintains "adjusted" versions as well as the base mPaperWidth/Height fields. But it doesn't update the NSPrintInfo when they change. AFAICT, if we extend these methods to update the NSPrintInfo in sync with the base dimensions, the bug here appears to be resolved. That may be the minimal-risk approach here, I suspect.

With that change, removing the chunk in nsPrintSettingsServiceX::SerializeToPrintDataParent that reads from the NSPrintInfo doesn't seem to be necessary, though it may be harmless. I don't think I understand all the interdependencies here, so I'm inclined to touch as little as possible. :)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1666473 - Update mPrintInfo in SetPaperWidth/Height overrides. r=jwatt

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d8ce79d27a4
Update mPrintInfo in SetPaperWidth/Height overrides. r=jwatt

Comment 8

[Alexandru Michis [:malexandru]]

Backed out changeset 7d8ce79d27a4 (Bug 1666473) for causing wpt failures in infrastructure/reftest/*

Backout link: https://hg.mozilla.org/integration/autoland/rev/b9a0af58863c5c8c49f50d8fbfefbed1e7142d3b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&fromchange=6c48acbfa45a105336b935ce25f9a062633f8c6a&searchStr=os%2Cx%2C10.14%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-macosx1014-64%2Fopt-web-platform-tests-print-reftest-e10s%2Cwp&tochange=b9a0af58863c5c8c49f50d8fbfefbed1e7142d3b&selectedTaskRun=f0Ot0VDURD2EqqnTyAoo0g.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316712184&repo=autoland&lineNumber=1007

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee8039174ab7
Update mPrintInfo in SetPaperWidth/Height overrides. r=jwatt

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ee8039174ab7

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 9177598 [details]
Bug 1666473 - Update mPrintInfo in SetPaperWidth/Height overrides. r=jwatt

Beta/Release Uplift Approval Request

• User impact if declined: After a user selects a paper size to print to on macOS, it will be reverted the next print. Since the paper selector is hidden by default in the new UI, this has particular potential to upset people.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 12

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9177598 [details]
Bug 1666473 - Update mPrintInfo in SetPaperWidth/Height overrides. r=jwatt

macos printing fix, approved for 82.0b5

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e434910dd720

Comment 14

[Vlad Lucaci, Desktop QA (:vlucaci)]

I have managed to reproduce this 83.0a1(20200922154306).

Confirming this issue as verified fixed with 83.0b6 and 84.0a1(20201101092255) on macOS 10.15.7