1666523 - Make the new print UI set nsIPrintSettings.paperSizeUnit whenever it sets a paper dimension

Status: RESOLVED FIXED

(Toolkit :: Printing, defect, P2)

Description

[Jonathan Watt [:jwatt]]

The frontend code is setting nsIPrintSettings.paperWidth/nsIPrintSettings.paperHeight without also setting nsIPrintSettings.paperSizeUnit:

https://searchfox.org/mozilla-central/rev/b58ca45005fe02077c92779483d1b60e9a49687c/toolkit/components/printing/content/print.js#1090-1091

It is critical that the unit be set whenever the width/height is set or else the settings can result in unusable paper dimensions. Worse yet, those settings are likely to be saved to prefs and so can end up breaking printing. I think this is at least part of the source of the broken printing reported in bug 1659928.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1666523 - Update paperSizeUnit whenever we update paper size settings. r?jwatt,mstriemer!

Comment 2

[Sam Foster [:sfoster] (he/him)]

I'm demoting this to a P2. Its an edge case resulting from bad preferences and a bad paperSizeUnit value - a state a user can only get in if they manually edit the pref value or copy prefs from one machine and platform to another. Without the incoming patch, the workaround is to manually change the relevant print.print_paper_size_unit value in about:config to either 1 or 0.

Comment 3

[Jonathan Watt [:jwatt]]

I'm pretty sure we're not doing this for v82, and since I filed this bug I'll take the liberty of punting it to v83.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fc2f6bba0a4
Update paperSizeUnit whenever we update paper size settings. r=mstriemer

Comment 5

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=SkDRJrTQRpavDhiRoHymqg.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=6fc2f6bba0a4d16f4ced72618b9e315c6c7cd2da&failure_classification_id=2

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318650287&repo=autoland&lineNumber=9038

Backout: https://hg.mozilla.org/integration/autoland/rev/d7dc1f284cd8ee807e3eaedae1d5957f25e62ae5

Comment 6

[Sam Foster [:sfoster] (he/him)]

(In reply to Razvan Maries from comment #5)

Backed out for perma failures.

Thanks, I'm investigating.

Comment 7

[Emma Malysz]

The problem seems to be that https://searchfox.org/mozilla-central/source/widget/cocoa/nsPrintSettingsX.mm#267 sometimes uses a stale value. We originally set the paper width to that value (like 11), viewSettings.paperWidth is initialized to that, and then our platform code later updates it again to the correct value (like 11.7). View settings won't register the change.

:nordzilla, is there a reason why our paperSize.width can start with the incorrect number in our mac platform code? This doesn't seem to be an issue with paperSize.height

Comment 8

[Erik Nordin [:nordzilla]]

Hey Emma,

Sorry it's taken me a sec to look at this. I'm looking into it now.

Leaving the NI active until I figure out what's going on.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Quick status update here: The patch was backed out for test failures in macos, addressed by :emalyz in comment 7. I've opened a bug 1672304 for that.
I'm trying to get this re-landed but in the meantime some changes to custom margins and the necessary rebase now causes the margins test to fail when I apply the patch. I'm investigating.

Comment 10

[Erik Nordin [:nordzilla]]

So I was able to diagnose the issue.

The macOS NSPrintInfo appears to maintain an invariant that Portrait ↔ width <= height and Landscape ↔ width >= height

The SetPaperWidth() and SetPaperHeight() functions in the macOS print settings [here] are aware of this. They preserve the original orientation, and then try to change it back if the orientation was switched.

But the invariant is maintained in both directions. Just like changing the width/height may implicitly change the orientation, changing the orientation may implicitly change the width/height.

So we have this specific edge case in trying to convert na_letter(8.5 x 11) into iso_a3(11.7 x 16.5):

1. Paper starts out as na_letter(8.5 x 11)
2. We change the width from 8.5 => 11.7.
3. Since the new width (11.7) is longer than the current height (11), the orientation implicitly changes to landscape.
   3a) We now have a paper size that is landscape(width(11.7) x height(11))
4. The front-end code now changes the orientation back to Portrait, but the width/height are implicitly swapped to maintain the invariant.
   4a) We now have a paper size that is portrait(width(11) x height(11.7))
5. Now the front-end code goes to change the height to 16.5
   5a) We now have a paper size that is portrait(width(11) x height(16.5))

The problem is that our print settings API has separate SetPaperWidth() and SetPaperHeight() functions, where as the native macOS print settings only has one conceptual SetPaperSize() method which encompasses the width and height. This is why the defect appeared to be specifically related to SetPaperWidth(), but it could really happen to either the width or the height depending on the order in which the parameters are set.

The good news is that this will be fixed by @jfkthame's patch for Bug 1667053, since that patch will make our print settings the source of truth, only updaing the macOS NSPrintInfo in one big sweep before printing, rather than trying to keep it up to date with each small change.

The part that is confusing me is that switching from US Letter to A3 appears to work in nightly when saving as PDF. According to these tests, and from stepping through the code myself, I would think that it shouldn't work correctly, unless we've fundamentally changed something about how we assign the width/height in this patch.

But I'm skimming through the code for this patch, and I don't see any major changes to the way assignments to width and height are done.
@sfoster any ideas?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46bb9fa2d70c
Update paperSizeUnit whenever we update paper size settings. r=mstriemer

Comment 12

[Sam Foster [:sfoster] (he/him)]

(In reply to Erik Nordin [:nordzilla] from comment #10)

But I'm skimming through the code for this patch, and I don't see any major changes to the way assignments to width and height are done.
@sfoster any ideas?

Possibly related, I found and fixed a copy-pasta bug in my patch which had the paperHeightInInches and paperWidthInInches used to check the custom margin values both using the paper.width. That was suspicious-looking, but I don't see how it would explain it at first glance...

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/46bb9fa2d70c

Comment 14

[Erik Nordin [:nordzilla]]

Just an update that Bug 1667053 was scheduled to land today, which both fixes and removes the skip-test for macOS.