1674106 - Print modal turns inaccessible when toggling between paper size using static custom values

Status: VERIFIED FIXED

(Toolkit :: Printing, defect, P1)

Description

[Anca Soncutean, Desktop QA]

Attachment: screencast issue.gif

Affected versions

• 83.0b5
• 84.0a1 (2020-10-28)

Affected platforms

• Windows 10
• Windows 7
• macOS 10.15
• Ubuntu 20.04

Steps to reproduce

1. Launch Firefox
2. Make sure print.tab_modal.enabled is set on true
3. Hit CTRL + P on any page (eg. https://en.wikipedia.org/wiki/Facebook))
4. Set paper size to A4
5. Set valid custom value
6. Select another paper size option that turns the custom values invalid
7. Set paper size back to A4
   .
   Expected result

• All the elements inside the modal can be accessed and they are functional

Actual result

• Most of the element inside the Print modal are inaccessible

Regression range

• Not a regression, present since custom margins implementation

Additional notes

• The modal is once again active if it is closed and reopened again

Suggested severity

• S2

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Attachment: UnresponsiveTab.gif

The symptoms seem to get worse if reproducing this issue and exiting the print preview while the print preview is in a loading state. The tab is no longer responding to actions like (tab refresh button, back navigation button,etc).
Note: This behavior seems to be encountered with both Fission enabled & disabled.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

After discussion, we're going to try to get this fixed in 84…

Comment 3

[Emma Malysz]

Custom margins should reset to default values if there is an invalid value detected.

The problem is that we are not taking into account the unwriteable margins in getSettingsToUpdate, but we are when setting the maxHeight and maxWidth within the element. These values are async properties and are resolved within fetchPaperMargins. I think the best route forward would be to move those calculations directly into updateSettings underneath this call (https://searchfox.org/mozilla-central/rev/16d30bafd4e5276d6d3c632fb52a6c71e739cc44/toolkit/components/printing/content/print.js#544-546), return the paperWrapper, and then modify changedSettings if necessary.

I also had made a typo here: https://searchfox.org/mozilla-central/rev/16d30bafd4e5276d6d3c632fb52a6c71e739cc44/toolkit/components/printing/content/print.js#477. That should read customMarginBottom.

Comment 4

[Emma Malysz]

Attachment: Bug 1674106: validate custom margin values when paper size changes and change if necessary

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58572979c431
validate custom margin values when paper size changes and change if necessary r=sfoster

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/58572979c431

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emalysz, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

[Anca Soncutean, Desktop QA]

Attachment: affected paper sized.gif

I can still reproduce the issue for some of the paper size on latest Nightly across platforms

Comment 9

[Emma Malysz]

Attachment: Bug 1674106, delete custom margins from userChangedSettings when updating an invalid margin after a paper size switch

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36f51561645e
delete custom margins from userChangedSettings when updating an invalid margin after a paper size switch r=mstriemer

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/36f51561645e

Comment 12

[Emma Malysz]

Comment on attachment 9190369 [details]
Bug 1674106, delete custom margins from userChangedSettings when updating an invalid margin after a paper size switch

Beta/Release Uplift Approval Request

• User impact if declined: We can lock the print UI for a user with custom margins that switches between paper sizes multiple times.

This is an edge case, but still is an issue that we should address

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Open the new print preview dialog

2. Set paper size to us letter
3. Set right/left margins to 4 each
4. Switch to A5 paper sizes. Notice margins reset to 0.5
5. Switch to A6 paper size. Notice margins are still 0.5 and the UI is usable

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): no strings, just two lines of code that address an edge case
• String changes made/needed: n/a

Comment 13

[Emma Malysz]

Comment on attachment 9185337 [details]
Bug 1674106: validate custom margin values when paper size changes and change if necessary

Sorry, I believe the first patch would not require beta uplift, as it should have made the cutoff for 84

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9190369 [details]
Bug 1674106, delete custom margins from userChangedSettings when updating an invalid margin after a paper size switch

Thanks for including automated tests. Approved for 84.0b8.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Anca Soncutean, Desktop QA]

Verified fixed with Fx 84.0b8 and Fx 85.0a1(2020-12-03) on Windows 10, macOS 10.15 and Ubuntu 18.04.