431190 - New Linux print & page-setup dialogs should be modal, but they aren't

Status: RESOLVED FIXED

Description

[Daniel Holbert [:dholbert]]

Our new Linux print dialog should be modal, but it isn't.

Steps to reproduce:
 1. Load any web page, e.g. http://ftp.mozilla.org/
 2. Select a chunk of text
 3. Hit Ctrl-P to open print dialog.
 4. Move the print dialog out of the way.
 5. Click somewhere in the text of the original document
    --> RESULT: Selection disappears. (click has an effect
 6. Try to select a different chunk of text
    --> RESULT: New selection is effective
 7. Try to open the menus (File, Edit, etc.) on the original firefox Window.
    --> RESULT: Menus are fully usable
 8. Use Ctrl-T to open a new blank tab
 9. Close the original tab (*the tab that you're printing!!*)
    --> RESULT: The original tab closes!!

The results listed above all demonstrate that the new print dialog is *not* modal on Linux.

However, I think it *should* be modal, for the following reasons:
 - FF2's print dialog (under linux) is modal.
 - FF3's print dialog under *windows* is modal
 - Gedit's print dialog (under linux) is modal.  (and Gedit uses the same GTK print dialog that FF3 does)


This is an issue for these reasons:
 * Steps 5 and 6 have weird implications for what the expected output of "Print Selection" would be.
 * Step 7 lets you edit the page setup and attempt to print-preview (though print-preview just does nothing)
 * Steps 8-9 is SCARY. (though it seems to have the expected output -- the original document still prints ok)

Comment 1

[Daniel Holbert [:dholbert]]

Actually, the new Linux page-setup dialog is similarly non-modal, and should be, for all of the same reasons mentioned in comment 0:
  - FF2's page-setup dialog (under linux) is modal.
  - FF3's page-setup dialog under *windows* is modal
  - Gedit's print dialog (under linux) is modal.  (and Gedit uses the same GTK page-setup dialog that FF3 does)

Comment 2

[Daniel Holbert [:dholbert]]

This is happening because we don't pass the parent windows to the dialog-creating code.

In particular, we're passing NULL for the "GtkWindow *parent" parameter at both of these lines in nsPrintDialogGTK.cpp:

162  dialog = gtk_print_unix_dialog_new(GetUTF8FromBundle("printTitle").get(), NULL);

555 GtkPageSetup* newPageSetup = gtk_print_run_page_setup_dialog(NULL, oldPageSetup, gtkSettings);

Links to source:
http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsPrintDialogGTK.cpp#162
http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsPrintDialogGTK.cpp#555

Comment 3

[Daniel Holbert [:dholbert]]

FWIW, to make our "built-in" print dialog and page-setup dialog (used in FF2/All and in FF3/Windows), we pass it the parent window from within nsPrintingPromptService.

http://mxr.mozilla.org/seamonkey/source/embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp

e.g. ShowPrintDialog and ShowPageSetup both have a "nsIDOMWindow *parent"

However, notably, the "dlgPrint->ShowXXX" funcitons (for native print dialogs) don't use the parent window argument.

We probably want to modify these functions to take that argument, and also convert it from its current type (nsGlobalChromeWindow * in my GDB session) into a GtkWindow* somehow, if that's possible.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> and also
> convert it from its current type (nsGlobalChromeWindow * in my GDB session)
> into a GtkWindow* somehow, if that's possible.

It looks like two functions already exist for doing this conversion. (though I'm not sure if they're accessible to this area of code)

In particular, we have:
  /mozilla/embedding/browser/gtk/src/EmbedGtkTools.h
    GetGtkWidgetForDOMWindow
and
  /mozilla/embedding/browser/gtk/src/GtkPromptService.h
    GtkPromptService::GetGtkWindowForDOMWindow

The GtkPromptService method is private, so we probably won't be able to use that one.

These functions' implementations are here:
http://mxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/EmbedGtkTools.cpp#46
http://mxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/GtkPromptService.cpp#284

Comment 5

[Damon Sicore (:damons)]

Won't block on this as there's not a big negative with regard to user experience.  The only issue I see is that we might need an API change.  So, I'm minusing, but please request approval if a patch surfaces. 

Comment 6

[Daniel Holbert [:dholbert]]

Created attachment 318493 [details] [diff] [review]
patch v1 (draft)

This draft patch fixes the bug, making print & page-setup windows modal.

The main trouble I've had is finding the best way to convert a nsIDOMWindow* into a GtkWindow*.  I couldn't easily reference or copy the functions mentioned in Comment 4, so this patch copies similar functions from the File-Picker code.

In making a final patch, these functions should at least be cleaned up, and possibly even moved into a generic utility class where they could be called from both places. (both file-dialog and printing code).

Comment 7

[Daniel Holbert [:dholbert]]

FWIW: Even with patch v1, I can still cause some mischief like this:
 1. Start Firefox
 2. Ctrl-N to make a second window
 3. Ctrl-P to open print dialog on current window.
 4. Alt-Tab, to switch to other (non-modally-disabled) window
 5. Ctrl-Q to quit firefox

This closes the Firefox windows, while still leaving the Print dialog open, which seems a little broken.

(More complicated situations like this can be created by opening many windows, with multiple page-setup and print dialogs, each associated with a window.)

However, AFAICT, we still handle things pretty gracefully.  If i choose to print from the dialog, the output is printed correctly.  If I cancel, it just closes, without crashing or anything, and the Firefox process ends.

Comment 8

[Daniel Holbert [:dholbert]]

Actually, I can do the exact same thing described in Comment #7 using the File-Open and Save-As dialogs instead of the Print dialog.

So, I'm not going to worry about that sort of thing -- if it's an issue, we can file another bug on it.

Comment 9

[Daniel Holbert [:dholbert]]

Created attachment 318705 [details] [diff] [review]
patch v2

(In reply to comment #6)
> In making a final patch, these functions should at least be cleaned up, and
> possibly even moved into a generic utility class where they could be called
> from both places. (both file-dialog and printing code).

Per vlad's suggestion, I'm just adding an XXX comment in this version indicating that we should unify the duplicated code in 1.9.0.x.  For now, the duplicated code is minimal enough that it's not a big issue, and it's more important that we get this fixed.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 318705 [details] [diff] [review]
patch v2

Looks fine here

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 318705 [details] [diff] [review]
patch v2

(In reply to comment #5)
> Won't block on this as there's not a big negative with regard to user
> experience.  The only issue I see is that we might need an API change.  So, I'm
> minusing, but please request approval if a patch surfaces. 

Requesting approval.
 - Benefit: Restores important modal behavior of print/page-setup dialogs
 - Risk: small

Comment 12

[Damon Sicore (:damons)]

Comment on attachment 318705 [details] [diff] [review]
patch v2

a1.9+=damons

Comment 13

[Daniel Holbert [:dholbert]]

Patch v2 checked in.

Checking in embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp;
/cvsroot/mozilla/embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp,v  <--  nsPrintingPromptService.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in widget/public/nsIPrintDialogService.h;
/cvsroot/mozilla/widget/public/nsIPrintDialogService.h,v  <--  nsIPrintDialogService.h
new revision: 3.4; previous revision: 3.3
done
Checking in widget/src/gtk2/Makefile.in;
/cvsroot/mozilla/widget/src/gtk2/Makefile.in,v  <--  Makefile.in
new revision: 1.74; previous revision: 1.73
done
Checking in widget/src/gtk2/nsFilePicker.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsFilePicker.cpp,v  <--  nsFilePicker.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in widget/src/gtk2/nsPrintDialogGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.cpp,v  <--  nsPrintDialogGTK.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in widget/src/gtk2/nsPrintDialogGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.h,v  <--  nsPrintDialogGTK.h
new revision: 1.4; previous revision: 1.3
done
Checking in widget/src/xpwidgets/nsBaseFilePicker.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsBaseFilePicker.cpp,v  <--  nsBaseFilePicker.cpp
new revision: 1.32; previous revision: 1.31
done

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> I'm just adding an XXX comment in this version
> indicating that we should unify the duplicated code in 1.9.0.x.

Filed bug 431596 for the unification.