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

RESOLVED FIXED in mozilla9

Status

()

Toolkit
Printing
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Steffen Wilberg, Assigned: Steffen Wilberg)

Tracking

({ux-minimalism})

Trunk
mozilla9
All
Windows 7
ux-minimalism
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 552749 [details]
screenshot of current dialog

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.
(Assignee)

Comment 1

7 years ago
Created attachment 552752 [details]
screenshot of proposed reduced dialog

This is the print dialog without the frame options.
(Assignee)

Updated

7 years ago
Attachment #552752 - Flags: ui-review?(limi)
(Assignee)

Comment 2

7 years ago
Created attachment 552755 [details] [diff] [review]
patch
Assignee: nobody → steffen.wilberg
Status: NEW → ASSIGNED
Attachment #552755 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
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)?
(Assignee)

Comment 4

7 years ago
(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.
(Assignee)

Comment 6

7 years ago
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.

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+
(Assignee)

Updated

7 years ago
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
Last Resolved: 7 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?
(Assignee)

Comment 12

7 years ago
(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.
(Assignee)

Updated

7 years ago
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.