507392 - remove moveToAlertPosition() usage

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

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

Comment 1

[Mark Banner (:standard8)]

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 :-)

Comment 2

[Magnus Melin [:mkmelin]]

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.

Comment 3

[[e-mail]]

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.

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

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

Comment 6

[[e-mail]]

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

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

third time's a charm?

titlebar is on by default, and resizable isn't applicable to modal dialogs, so i removed those.

Comment 8

[[e-mail]]

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

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

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

Comment 10

[[e-mail]]

Comment on attachment 395084 [details] [diff] [review]
proposed fix, v4

Phew! ;-)

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 395084 [details] [diff] [review]
proposed fix, v4

:) Thx a log neil!

Requesting r? from philipp for a few lines in calendar/

Comment 12

[Mark Banner (:standard8)]

(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 13

[Mark Banner (:standard8)]

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.

Comment 14

[Magnus Melin [:mkmelin]]

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

Comment 15

[Mark Banner (:standard8)]

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.

Comment 16

[Magnus Melin [:mkmelin]]

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?

Comment 17

[Mark Banner (:standard8)]

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

Comment 18

[Phil Ringnalda (:philor)]

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.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v5

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.

Comment 20

[[e-mail]]

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

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v6

Progress dialogs now working too.

Comment 22

[[e-mail]]

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?)

Comment 23

[Magnus Melin [:mkmelin]]

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.

Comment 24

[Markus Stange [:mstange]]

Yes, please file a bug for making the chrome feature handling consistent.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 396242 [details] [diff] [review]
proposed fix, v6

Sorry for the delay in getting back to this r+a=Standard8

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 396242 [details] [diff] [review]
proposed fix, v6

Sorry for the delay, r=philipp

Comment 27

[Magnus Melin [:mkmelin]]

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.

Comment 28

[[e-mail]]

Comment on attachment 396242 [details] [diff] [review]
proposed fix, v6

>+                      "chrome,modal,centerscreen,modal",
Oops! (Both times, I think.)

Comment 29

[Magnus Melin [:mkmelin]]

Yeah, one time. Code blindness :(
I'll check in a removal of the duplicate modal once the tree is happier.

Comment 30

[Magnus Melin [:mkmelin]]

changeset:   4017:2bc754381c2e
http://hg.mozilla.org/comm-central/rev/2bc754381c2e