538487 - With taskbar previews enabled, print preview gets its "own tab". Switching tabs causes the print preview toolbar to persist and won't close

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[u88484]

STR: 
1) Open two or more tabs
2) Open print preview on the second tab
3) a new tab is created (older version of Firefox basically overwrite this tab)
2) Move mouse down to the taskbar and switch to the first tab

The print preview toolbar persist across tabs and now you can't close it unless you go back to the tab that you opened the print preview.

The following error appears in the error console when trying to close this toolbar:

Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPrint.exitPrintPreview]
Source file: chrome://global/content/printUtils.js
Line: 289

Comment 1

[Rob Arnold [:robarnold]]

Setting uiwanted as I'm not sure what we should do for the print preview case.

Comment 2

[Jim Mathies [:jimm]]

I'd say this is a bug in the way we implement print preview. It should open in it's own window, or like we're doing with other stand alone dialog nows, have it fall into it's own separate tab.

Comment 3

[Rob Arnold [:robarnold]]

There's a project to do this listed on https://wiki.mozilla.org/Firefox/Projects but there's no bug filed or project page for it.

Comment 4

[Rob Arnold [:robarnold]]

Attachment: Special case the print preview tab

I don't know much about the print preview code so I'd like some feedback on the approach.

We special case the print preview tab - it is never made visible or part of the ordering. It also does not count against the "max tab previews" check.

Since the print preview code also hides all the chrome so it can make a toolbar, I decided to exit print preview mode if the user wants a live preview of their tabs.

Unfortunately, the obvious check of gInPrintPreview fails when trying to decide what to do with a new tab - it's never set when the tab is created and doing so leads to unfortunate unbounded recursion. Hence the odd and unabstract check in TabWindow.newTab. I'd like to get rid of it but I'd also just rather that the print preview feature didn't go make a new tab in the first place!

Comment 5

[Rob Arnold [:robarnold]]

Comment on attachment 440709 [details] [diff] [review]
Special case the print preview tab

After talking with Gavin, it's probably a better idea to try to fix up Firefox's implementation of the print preview code to not be so hacky. I don't have a plan yet but somehow we have to conjure up a nsIDocShell for the toolkit code. My faith in the test coverage here is low so this would be nice to have for an alpha.

Comment 6

[Tan Wei Lin Jason [:Ryuji]]

Attachment: Error cause by the print preview

STR:
1.Open Minefield
2.Open some tabs and go some web site
3.Go to Print Preview
4.Open other programs (i.e Thunderbird) and use it to open a link in a new tab in Firefox
5. Go to new tab with the new website, close Print Preview
6. Tab bar, Navigation and Bookmark Toolbar will disappear leaving only the Firefox Menu Button

Comment 7

[Johnathan Nightingale [:johnath]]

Rob - is this still something you're working on? It's been dormant for a while, but if you can't do it, I'd appreciate any suggestions of alternates, since it's a release blocker for FF4.

Comment 8

[Rob Arnold [:robarnold]]

No, I have not worked on this for some time. If it happens to work out that we can make the print preview replace the current tab contents, then we should do that instead and this bug won't occur. I don't know if the bugs that halted the earlier print preview project/sprint have been fixed or do not occur. I will hopefully have some time in about 2 weeks to figure this out.

Comment 9

[Jim Mathies [:jimm]]

Attachment: hide tab previews v.1

Simple fix - hide tab previews for the print preview window.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 495866 [details] [diff] [review]
hide tab previews v.1

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -196,16 +196,19 @@ XPCOMUtils.defineLazyGetter(this, "Win7F
>     Cu.import("resource://gre/modules/WindowsPreviewPerTab.jsm", temp);
>     let AeroPeek = temp.AeroPeek;
>     return {
>       onOpenWindow: function () {
>         AeroPeek.onOpenWindow(window);
>       },
>       onCloseWindow: function () {
>         AeroPeek.onCloseWindow(window);
>+      },
>+      enableForWindow: function (flag) {
>+        AeroPeek.enableForWindow(window, flag);
>       }
>     };
>   }
> #endif
> #endif
>   return null;
> });
> 
>@@ -2676,24 +2679,28 @@ var PrintPreviewListener = {
>       this._tabBeforePrintPreview.linkedBrowser : gBrowser.selectedBrowser;
>   },
>   getNavToolbox: function () {
>     return gNavToolbox;
>   },
>   onEnter: function () {
>     gInPrintPreviewMode = true;
>     this._toggleAffectedChrome();
>+    if (Win7Features)
>+      Win7Features.enableForWindow(false);
>   },

Does this actually make sense API-wise, i.e. would we really want to disable all potential "win 7 features" in print preview mode?

Comment 11

[Jim Mathies [:jimm]]

> Does this actually make sense API-wise, i.e. would we really want to disable
> all potential "win 7 features" in print preview mode?

The only win7 features tied to this are our custom tab / live previews. Since there are no tabs with print preview up, seemed like the simplest fix. When our custom implementation is disabled, Windows reverts back to it's default behavior for the window, so we don't loose the thumbnail or the live preview feature.

Comment 12

[Dão Gottwald [:dao]]

I realize this is the only actual feature. It still seems to me that you really want something like Win7Features.AeroPeek.enableForWindow...

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #12)
> I realize this is the only actual feature. It still seems to me that you really
> want something like Win7Features.AeroPeek.enableForWindow...

Ah, sure, sounds good to me.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

As long as we block on bug 605338 we don't need to continue to block on this bug.

Comment 15

[Rob Arnold [:robarnold]]

(In reply to comment #14)
> As long as we block on bug 605338 we don't need to continue to block on this
> bug.

My understanding (and maybe this has changed) is that we are just flipping the pref in bug 605338 but leaving the GUI option visible. Shouldn't we still get this fixed for final since having a GUI option implies some sort of quality for this feature?

Comment 16

[Jim Mathies [:jimm]]

Attachment: hide tab previews v.2

Dao, how about |Win7Features.enableTabPreviewsForWindow(flag)|?

That seems to get the point across.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

I would imagine that having a gui option would depend on if bugs like this one are fixed.  Up to drivers to make the blocking call.

Comment 18

[u88484]

(In reply to comment #17)
> Up to drivers to make the blocking call.
When I filed the bug the print preview toolbar would not close leaving the user with a broken browsing session but now it seems to close fine so maybe it shouldn't block even though it is a nuisance.

Comment 19

[Jim Mathies [:jimm]]

This isn't blocking now that we've disabled previews by default. We do have a fix though for people who enable previews on their own. Hopefully it'll land before we release.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 497805 [details] [diff] [review]
hide tab previews v.2

Why is Win7Features.enableTabPreviewsForWindow better than Win7Features.AeroPeek.enableForWindow?

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> Comment on attachment 497805 [details] [diff] [review]
> hide tab previews v.2
> 
> Why is Win7Features.enableTabPreviewsForWindow better than
> Win7Features.AeroPeek.enableForWindow?

Honestly I don't understand how the scope works out on this - 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#189

AeroPeek seems temporary to me. I can call Win7Features but I don't see how I can access AeroPeek directly.

Comment 22

[Dão Gottwald [:dao]]

You would set AeroPeek as a property on the returned object:

     return {
       AeroPeek: temp.AeroPeek,
       ...

Comment 23

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 27

[Robert Strong (they/them - no direct email)]

Shell Integration is for default browser code so moving to general.

Comment 28

[:Gijs (he/him)]

We changed print preview so I don't think this is an issue now.