1664415 - Unexpected title for error-message dialog on a Print operation

Status: ASSIGNED

(Toolkit :: Printing, defect, P3)

Description

[Jonathan Kew [:jfkthame]]

This is a pretty minor (cosmetic) issue, but feels wrong as it stands.

STR (on Windows, probably similar on other platforms but not tested):

• Ensure print.tab_modal.enabled is set to true
• Choose the Print command to open the tab-modal print UI
• Select a destination printer (e.g. my Samsung laser device; also tried same STR with the Microsoft OneNote destination)
• Go to Start menu > Settings > Devices > Printers & scanners
• Choose the printer that has been chosen as destination in Firefox, and click Remove Device to uninstall it from Windows
• Return to Firefox and see that the (now-removed) printer is still the chosen destination
• Click the Print button

Expected:
Printing should presumably fail, ideally with a suitable error message.

Actual:
Printing does indeed fail, with the message "The selected printer could not be found." This is as expected (I was curious how well we'd handle this edge case). However, one jarring note is that the error dialog that appears is titled Print Preview Error.

This seems wrong because the print preview in the tab-modal dialog was fine; the error occurred when attempting to actually print. So I'd have expected the _displayPrintingError function to use the print_error_dialog_title string here, not the printpreview_error_dialog_title.

Presumably we're getting false for the isPrinting parameter because this operation is initiated from a preview context in the tab-modal UI. But at the point where the user clicks Print to initiate an actual print job, isPrinting for errors that result from this operation ought now to be true.

Comment 1

[Jonathan Kew [:jfkthame]]

Actually, this might simply be a platform bug: looks to me like nsDocumentViewer::GetDoingPrint may be broken. I'll check...

Comment 2

[Jonathan Kew [:jfkthame]]

FWIW, it appears that method has been bogus forever, or as good as..... probably a copy/paste bug from when it was created. Roc added the XXX comment questioning it back in bug 288873, but it's never been addressed.

I'll try fixing that (when my build catches up) and see if it has any visible effect.

Comment 3

[Jonathan Kew [:jfkthame]]

So the obvious fix to nsDocumentViewer::GetDoingPrint (to return IsDoingPrint() from the job), while probably a good thing to do, doesn't resolve the problem here, because at the point where this error occurs, the mIsDoingPrinting flag has not yet been set. So it turns out that when PrintingChild.jsm sends the error message, both wbp.doingPrint and wbp.doingPrintPreview are false.

To improve the behavior here, then, I suggest we switch the sense of the flag that's stored in the "Printing:Error" message: instead of trying to set an isPrinting flag, we can make it an isPreviewing flag, which we can more reliably set from wbp.doingPrintPreview.

With this change, the error dialog I get when trying to use a removed printer is properly titled Printer Error rather than Print Preview Error.

(An alternative fix would be to simply pass !wbp.doingPrintPreview as the isPrinting flag in the message, instead of trying to use wbp.doingPrint but it seems more transparent to rename the flag itself.)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1664415 - patch 1 - Fix nsDocumentViewer::GetDoingPrint to return the correct answer. r=jwatt

Note that doingPrint only becomes true once the job actually gets started;
initially, both doingPrint and doingPrintPreview may be false.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1664415 - patch 2 - Use an isPreviewing rather than isPrinting flag in printing error messages to determine which form of messages to use. r=jwatt

Depends on D89906

Comment 6

[Jonathan Kew [:jfkthame]]

On further poking around, it turns out the error here is being fired by nsPrintJob::CleanupOnFailure. At this point, whichever of the printing or print-previewing flags was set on the nsPrintJob has already been cleared, and so neither doingPrint nor doingPrintPreview is going to be true.

In effect, then, the PrintingError event handler is flying blind: neither the previewing nor printing attribute will be true at this point.

(The same would be true for errors fired from nsPrintJob::CommonPrint, if we run into that case somewhere.)

So I think that patch 2 here makes sense: the "printer error" title is the more generic label to use for some kind of failure in the printing subsystem, and the "print preview error" title should only be used when we specifically know that the failure happened during a previewing operation. Patch 2 replaces the isPrinting flag with isPreviewing, so that "preview" is the marked case for dialog-title selection, and "printing" is the fallback, which IMO is less surprising to the user.

Comment 7

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

(Bumping to 84, because it's fairly minor and one of the patches still needs review.)

Comment 8

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

(Whoops, sorry, I missed that this was assigned. But while I'm here, jfkthame, did you want to pick a different reviewer, and we can try to get this landed in 84? 🙂)

Comment 9

[Jonathan Kew [:jfkthame]]

The original "patch 2" here has bitrotted, and didn't work well in all cases anyhow.

I took another look at this, and I think the root of the problem is that in nsPrintJob::CommonPrint and similarly in CleanupOnFailure, we reset the isPrinting or isPreview flag in the print job before we fire the error event.

This means that if we catch an error here, by the time the event is fired, there's no chance of nsIWebBrowserPrint.doingPrint returning true, and so the error dialog will always get the "Print Preview Error" title.

AFAICT if we move the clearing of these flags to follow the (possible) call to FirePrintingErrorEvent, then we get the expected results. I hacked the code in nsPrintJob to randomly "fail" when processing some pages, and after making this change, I get a dialog with the desired "Print Preview Error" if the failure occurs while presenting the preview UI, and "Printer Error" if it occurs during sending the actual job to the printer.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1664415 - patch 2 - Don't clear the isDoingPrinting or isPrintPreview flags until after firing a potential error message. r=jwatt

Depends on D89906

Comment 11

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

(Moving bugs to 86, part 2.)