7372 - Dialogs on Windows don't look like dialog windows

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Michael Lowe]

Something needs to be done to make Mozilla dialog windows look more like real
Windows dialogs.   At present it seems that nsAppShellService creates a
nsWebShellWindow, and nsWebShellWindow creates a nsWindow.   However, the
nsWindow is created with a border style eBorderStyle_window.   Instead, in
this case the nsWindow should be created with a border style
eBorderStyle_dialog, or else nsWebShellWindow should create a nsDialog instead
of a nsWindow.

Comment 1

[Michael Lowe]

*** Bug 7371 has been marked as a duplicate of this bug. ***

Comment 2

[davidm]

reassign to XPToolkit/Widgets. Its not just windows. All platforms need the
ability to adjust window chrome.

Comment 3

[Peter Trudelle]

reassigning to danm as p3 for m9

Comment 4

[David Hyatt]

I fixed this one just now.  Merry Christmas, Dan.

Comment 5

[cpratt]

Reassigning to reporter for verification (it looks fine to me, just wanted to
make sure it looks fine to them as well).

Comment 6

[Michael Lowe]

Looking better.   Now a Windows only widget issue: nsWindow is not creating
dialog windows with the correct attributes.   Dialog windows in Windows should
not have minimise/maxamise buttons nor a system menu/icon at the far left of the
titlebar.

Comment 7

[Michael Lowe]

At this time don't know exactly what windows styles should be added/removed, but
WS_EX_DLGMODALFRAME looks like it is missing.

Comment 8

[Michael Lowe]

Are you the man for the job Rod?

Comment 9

[Peter Trudelle]

mass-moving all m13 bugs to m14

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Patch to remove minimise and maximise buttons from dialogs

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch modifies nsAppShellService so that resizable windows do not get 
maximise/minimise boxes or a system menu if they happen to be dialogs.

The remaining problem is, resizable dialogs still DO get a system menu even 
though eBorderStyle_menu is not specified. In fact, nsWindow will not remove the 
system menu unless you ask for no system menu AND no close box. (The max and min 
boxes do go away though, which is a definite improvement.)

After lots of experimentation, I think it's because if you leave out WS_SYSMENU, 
the close box goes away too and there is no way to get it back. If you look at 
standard Windows dialogs, they actually have WS_SYSMENU specified and there is a 
system menu around --- try pressing Alt-space. My guess is that the windowproc 
for the built in Dialog class does some fancy stuff with WM_NCxxx to make the 
system menu disappear. Hacking that into nsWindow would be tough. The system 
menu isn't so bad, anyway :-).

By the way, in a vain attempt to fix this problem in nsWindow, I significantly 
cleaned up the code for assigning window styles. It's kind of ugly right now 
with unused functions, misnamed functions, and logic scattered all around. I'll 
submit the patches in case you want them.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Patch to nsWindow.cpp to clean up WS_ style computations

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch to nsWindow.h to clean up WS_ style computations

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks for checking in my patches! Things seem to be working fine, including the 
Viewer (which I had some concerns about).

I have a new patch that eliminates the system menu for dialog boxes. It works by 
creating dialog windows using CreateDialogIndirectParam instead of 
CreateWindowEx; thus, dialog windows get the "Dialog" window class and, as I 
suspected, this takes care of hiding the system menu. Everything looks good and 
seems to work fine!

Note: in order to make this work, I had to adjust the way that we discover the 
nsWindow pointer from an HWND. We can't use GWL_USERDATA anymore because the 
Dialog class uses that space. We could do something fancy like extend 
NetscapeWindowClass with DLGWNDEXTRA + 4 bytes of user data, and store the 
pointer at GWL_USERDATA+DLGWNDEXTRA, but then we'd also have to make our own 
copy of the Dialog class to make space for the extra data. Instead I'm storing 
the nsWindow pointer as a window property (accessed via GetProp) under the atom 
"nsWindowPtr". Could be a minor performance hit there but I don't think it's 
significant. I wrapped all getting and setting of the nsWindow pointer behind 
two functions, so this can easily be revisited.

Another note: because of the bogus way dialog coordinates are specified in 
"dialog units", it's possible that the actual window size and position might be 
off the specified values by a few pixels. This doesn't seem to affect the app in 
any visible way. I have made sure the size is always rounded up.

This needs testing with the AIMM input method stuff. NetscapeWindowClass has 
some code for IMM support which I am now bypassing for dialogs; I hope this is 
already works by default for the Dialog class, but it must be tested, and I 
don't have the IMM tools.

If this patch is acceptable, I think we can declare victory over this bug.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Patch to give dialog boxes the Dialog window class

Comment 16

[rods (gone)]

I just added the patches and so far it looks fine, but dan should do some 
testing.

Comment 17

[rods (gone)]

something is wrong with the clean up of widgets. I am now getting asserts in 
nsBrowserWindow.cpp that there is too many ref counts on the Back, Froward, et.c 
widgets. Someone needs to look into this.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can't reproduce this particular problem, but I did find one additional change 
that needed to be made, in Viewer. I will attach the patch.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Patch to Viewer's text widget to use new GetNSWindowPtr

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: IMPROVED Patch to Viewer's text widget to use new GetNSWindowPtr

Comment 21

[Peter Trudelle]

Mass moving M17 bugs to M18

Comment 22

[Dan M]

All of roc's patches up to here are now checked in. Thanks to his nontrivial 
latest set, dialog windows have lost that inappropriate application icon in their 
title bar. I don't see the refcount assertions in viewer that rods mentioned 
above. Things seem good. Are we done with this one yet?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I hope you tested with AIMM...

Thanks!!

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, there is a problem with the "dialog class" stuff which I just noticed, but 
was probably always there. Pressing Alt-Space in a resizable dialog brings up 
the "system menu" as it should, but that menu contains Maximise and Minimise 
options even when the dialog is not showing the Minimise and Maximise buttons. 
This lets you do some stupid things like minimise the dialog box. However, this 
may well be just a bug in the Windows Dialog class handler itself ... I can't 
find a resizable, non-minimisable dialog anywhere else in the system to check!

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This problem is almost certainly not worth worrying about, because only crazy 
power users even know that you can pull up a system menu in a dialog box.

Comment 26

[Dan M]

I demonstrated just last night that I could sleep with this problem in the code 
base. If it even is a problem. This bug seems fixed to me, and wanting to be 
closed. Obliging. Thanks again for your patches!