Closed Bug 678600 Opened 10 years ago Closed 10 years ago

Don't add print frames options to Windows print dialog when they would be disabled anyway because there are no frames

Categories

(Toolkit :: Printing, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: steffen.wilberg, Assigned: steffen.wilberg)

References

(Blocks 1 open bug)

Details

(Keywords: ux-minimalism)

Attachments

(3 files, 1 obsolete file)

Gecko extends the native Windows print dialog with frame options:
- As laid out on screen
- the selected frame
- each frame separately.

Most of the time however, these options are disabled because the document doesn't contain frames. This affects most websites and pretty much every single email in Thunderbird/Seamonkey.

The attached screenshot shows the current print dialog. The locale mismatch is a result of using an English Firefox on a German Windows.
This is the print dialog without the frame options.
Attachment #552752 - Flags: ui-review?(limi)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → steffen.wilberg
Status: NEW → ASSIGNED
Attachment #552755 - Flags: review?(roc)
Summary: Hide print frames options from Windows print dialog when they would be disabled anyway because there are no frames → Don't add print frames options to Windows print dialog when they would be disabled anyway because there are no frames
Design comment: Doesn't discoverability dictate leaving the options present, but disabling them, just as is done now?

Technical comment: Your patch almost certainly makes code later in the same function dead; you should find that code and remove it.

Tangential request: Do you think you would be able to implement the "missing piece" of bug 629500 (the native Page Setup dialog box)?
(In reply to Zack Weinberg (:zwol) from comment #3)
> Design comment: Doesn't discoverability dictate leaving the options present,
> but disabling them, just as is done now?
You'll discover the frame options when printing a frameset by looking at a print dialog which is larger than usual because it contains those options.
On normal webpages (or emails), i.e. almost always, you simply don't need these options.

> Technical comment: Your patch almost certainly makes code later in the same
> function dead; you should find that code and remove it.
The rest of the function only deals with extending the dialog by the frame options, and calls InitializeExtendedDialog. In the nsIPrintSettings::kFrameEnableNone case, I don't want to add these options and initialize them, so I just return true.
But in the other cases, kFrameEnableAll and kFrameEnableAsIsAndEach, the function proceeds as before.

> Tangential request: Do you think you would be able to implement the "missing
> piece" of bug 629500 (the native Page Setup dialog box)?
I'm afraid not. I usually hack XUL+JS+CSS...
(In reply to Steffen Wilberg from comment #4)
> > Technical comment: Your patch almost certainly makes code later in the same
> > function dead; you should find that code and remove it.
> The rest of the function only deals with extending the dialog by the frame
> options, and calls InitializeExtendedDialog. In the
> nsIPrintSettings::kFrameEnableNone case, I don't want to add these options
> and initialize them, so I just return true.

Looking at the code, InitializeExtendedDialog can no longer be called with kFrameEnableNone, which means this block should be removed:

579   } else {  // nsIPrintSettings::kFrameEnableNone
580     // we are using this function to disabe the group box
581     SetRadio(hdlg, grp3, PR_FALSE, PR_FALSE); 
582     // now disable radiobuttons
583     SetRadio(hdlg, rad4, PR_FALSE, PR_FALSE); 
584     SetRadio(hdlg, rad5, PR_FALSE, PR_FALSE); 
585     SetRadio(hdlg, rad6, PR_FALSE, PR_FALSE); 
586   }

and replaced with an NS_ABORT_IF_FALSE.  There may be other such blocks elsewhere in the file; I didn't see any, but I'm not certain.
Attached patch patch v1.1Splinter Review
OK, I've updated the other caller of InitializeExtendedDialog as well, so I could remove the code block you mentioned.

That other caller is PropSheetCallBack, which is dead code, inside #ifdef MOZ_REQUIRE_CURRENT_SDK, only used in this file and defined nowhere. I think it's about the more modern Windows print dialog.
Attachment #552755 - Attachment is obsolete: true
Attachment #552755 - Flags: review?(roc)
Attachment #552790 - Flags: feedback?(zackw)
Comment on attachment 552790 [details] [diff] [review]
patch v1.1

(In reply to Steffen Wilberg from comment #6)
> Created attachment 552790 [details] [diff] [review]
> patch v1.1
> 
> OK, I've updated the other caller of InitializeExtendedDialog as well, so I
> could remove the code block you mentioned.

Thanks.  Looks good.

> That other caller is PropSheetCallBack, which is dead code, inside #ifdef
> MOZ_REQUIRE_CURRENT_SDK, only used in this file and defined nowhere.

... and it's been that way since the beginning of _CVS_ history.  Oh my aching head.

I am under the impression that we have required a fairly recent Windows SDK for some time, so it might be safe to enable that code unconditionally, but we probably need to have a conversation about whether it's worth doing, and it should happen in a new bug.

> I think it's about the more modern Windows print dialog.

That does appear to be what it is about, to my limited understanding of Windows.
Attachment #552790 - Flags: review?(roc)
Attachment #552790 - Flags: feedback?(zackw)
Attachment #552790 - Flags: feedback+
Attachment #552752 - Flags: ui-review?(limi) → ui-review?(faaborg)
Attachment #552752 - Flags: ui-review?(faaborg) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/8f064c1c1d40
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment on attachment 552790 [details] [diff] [review]
patch v1.1

> // Special Hook Procedure for handling the print dialog messages
> static UINT CALLBACK PrintHookProc(HWND hdlg, UINT uiMsg, WPARAM wParam, LPARAM lParam) 
> {
>+      return TRUE;

TRUE is a UINT now?
(In reply to Ms2ger from comment #11)
I followed the existing code. "return TRUE" was added at the end of PrintHookProc in bug 628157. See bug 628157 comment 9:
> The documentation for WM_INITDIALOG says that true should be returned
> for focus to be set.
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.