Closed Bug 507392 Opened 12 years ago Closed 12 years ago
To Alert Position() usage
Got tired of seeing "Trying to position a sizeless window; caller should have called sizeToContent() or sizeTo(). See bug 75649." in the (native) console all the time. We could fix it by calling sizeToContent() before very call to moveToAlertPosition(), but I don't see the point of the method call at all. If i remove it completely dialogs are properly centered relative to their opener. With the call, they are a bit over center on the screen it seems. Besides, the moveToAlertPosition method is in toolkit/obsolete/content/dialogOverlay.js since forever. I guess it was needed some time pre 2003? http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/obsolete/content/dialogOverlay.js&rev=HEAD&mark=1.1 Some affected dialogs: - new folder - rename folder - folder properties - address book card/list properties - ...
Well assuming these are all dialogs, then a different version of moveToAlertPosition is used and called within the dialog set up code anyway: http://hg.mozilla.org/releases/mozilla-1.9.1/diff/9b2a99adc05e/toolkit/content/widgets/dialog.xml Which means you're doing some of what bug 113722 was asking for :-)
Yep, all dialogs, except customheaders.js - but i've another cleanup patch coming that will make that a dialog too. So would seem this is indeed the comm-central part of what's left of bug 113722.
This seems to work on some dialogs but not others, viz: abAddressBookNameDialog renameFolderDialog askViewZoom abCardOverlay I guess the ones that it does work on are opened including the modal and centerscreen styles in their window features.
Attachment #391608 - Flags: review?(bugzilla) → review-
Comment on attachment 391608 [details] [diff] [review] proposed fix Per Neil's comment, looks like this needs another revision - modal dialogs are treated differently on mac, so I can't test easily.
Surprisingly, can't repro those problems on linux. Maybe it's platform dependent? I've added centerscreen where needed, but i don't see anything to do for abAddressBookNameDialog nor for askViewZoom. grep -r -C 5 --exclude=*~ --exclude=*.mn abAddressBookNameDialog.xul mail mailnews suite calendar grep -r -C 5 --exclude=*~ --exclude=*.mn askViewZoom.xul mail mailnews suite calendar
(In reply to comment #5) > I've added centerscreen where needed, but i don't see anything to do for > abAddressBookNameDialog nor for askViewZoom. I see the new patch does fix askViewZoom, but I can't see a change for rename folder or address book name which still open in the top left of my screen :-( In fact, you look as if you fixed rename folder for Thunderbird only - for suite you added centerscreen to a dialog that already has centerscreen...
third time's a charm? titlebar is on by default, and resizable isn't applicable to modal dialogs, so i removed those.
I discovered that my testing methodology was suspect - some of the dialogs have multiple entry points and I hadn't tried them all. Latest results: New contact - pass (but see below) Contact properties - pass New list - pass List properties - pass New address book - pass Address book properties - FAIL New LDAP directory - pass LDAP directory properties - FAIL Folder properties - pass New (sub)folder - FAIL Rename folder - pass New filter - pass Custom headers - pass Send progress - did not test Shutdown - did not test Zoom - pass Also, the second parameter to openDialog should always be blank, otherwise you get problems should you happen to open the same dialog from more than one parent window, which is a particular problem for the new contact dialog, because that can be opened from any of the main Suite windows (although the addressbook version must have a different caller because it isn't affected by the bug).
The failures should now be fixed. I don't know about Send progress and Shutdown - they do work for me but that doesn't say much...
Attachment #395084 - Flags: superreview?(neil) → superreview+
Comment on attachment 395084 [details] [diff] [review] proposed fix, v4 :) Thx a log neil! Requesting r? from philipp for a few lines in calendar/
(In reply to comment #7) > titlebar is on by default, and resizable isn't applicable to modal dialogs, so > i removed those. Except on mac where resizable and titlebar both seem to have the effect of making the modal dialog not resizable.
Comment on attachment 395084 [details] [diff] [review] proposed fix, v4 r- due to the previously mentioned mac issue. I've not tested on Linux or Windows yet, but I'm guessing they probably fine.
Attachment #395084 - Flags: review?(bugzilla) → review-
(In reply to comment #12) > (In reply to comment #7) > > titlebar is on by default, and resizable isn't applicable to modal dialogs, so > > i removed those. > > Except on mac where resizable and titlebar both seem to have the effect of > making the modal dialog not resizable. Since I *removed* resizable=no:s i'm really surprised if that made them *not* resizable. Were they really resizable before? And if so, should they? When i wrote resizable isn't applicable to modal dialogs i mean whatever you set it to there's no difference - at least on windows/linux.
Sorry I got that wrong. The modal dialogs are currently *not* resizable, with your patch they are resizable. So resizable is applicable to modal dialogs on Mac which is what I was trying to say.
So, are there any rules/guidelines for which mac modal dialogs should be resizable and which shouldn't? What so applications usually do? Re titlebar, did mac need/want that too?
(In reply to comment #16) > So, are there any rules/guidelines for which mac modal dialogs should be > resizable and which shouldn't? What so applications usually do? > > Re titlebar, did mac need/want that too? So from what I've seen so far, titlebar and resizable seem to do the same thing - they make the dialog not resizable. Mac never displays a title bar on a modal dialog afaik.
My vague memory (it's always hard to force modal dialogs in random programs when you want to see them) is that the only modal dialogs that are supposed to be resizeable are things like filepickers (or the New Filter dialog) that have listboxes in them.
Status quo regarding resizable=no then. Carrying fwd sr=neil Though if they aren't supposed to normally be resizable that sounds like a mac bug.
Assuming it's because I've still got the patch applied locally, Send Progress didn't work for me, it opened at the top left :-( (In reply to comment #15) > Sorry I got that wrong. The modal dialogs are currently *not* resizable, with > your patch they are resizable. Odd, since resizable=no is supposed to be the default...
Progress dialogs now working too.
Attachment #396242 - Flags: review?(philipp)
OK, so I've trawled through window watcher, app shell and widget, and what happens if you have a dialog but fail to specify (either positively or negatively) any of the chrome features (including titlebar or close, which default to on, and can therefore be safely used to disable this behaviour), then you get a widget-defined set of default flags. And in Windows widget that's no features, while in Mac widget that's all features. (I'm not sure about GTK2; it passes -1 which may not even be valid?)
neil: did you file a bug for it? I assume if you already tracked down where it's happening it might be too hard to fix.
Yes, please file a bug for making the chrome feature handling consistent.
Comment on attachment 396242 [details] [diff] [review] proposed fix, v6 Sorry for the delay in getting back to this r+a=Standard8
Comment on attachment 396242 [details] [diff] [review] proposed fix, v6 Sorry for the delay, r=philipp
Attachment #396242 - Flags: review?(philipp) → review+
changeset: 4007:a7ee2565bfa6 http://hg.mozilla.org/comm-central/rev/a7ee2565bfa6 Thanks to all! ->FIXED I filed bug 520026 to make the feature handling consistent.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.0b4 → Thunderbird 3.0rc1
Version: unspecified → Trunk
Comment on attachment 396242 [details] [diff] [review] proposed fix, v6 >+ "chrome,modal,centerscreen,modal", Oops! (Both times, I think.)
Yeah, one time. Code blindness :( I'll check in a removal of the duplicate modal once the tree is happier.
changeset: 4017:2bc754381c2e http://hg.mozilla.org/comm-central/rev/2bc754381c2e
You need to log in before you can comment on or make changes to this bug.