752206 - event dialog is not non-modal anymore

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[石庭豐 (Seak, Teng-Fong)]

In Sunbird, event dialogs are simple windows.

But in Lightning 1.4, event dialogs are modal windows which have become a pain: whenever I need to see event content, the whole Thunderbird window is shown up and covers the other windows (of other applications) on which I was working on.

Please make it back as non modal.

Comment 1

[Pete Riley]

(In reply to 石庭豐 (Seak, Teng-Fong) from comment #0)
> But in Lightning 1.4, event dialogs are modal windows
I can't reproduce it in Lightning 1.2 in Linux.  I don't think you should be seeing model dialogs with 1.4 in XP either.  A patch was implemented a long time ago to make event dialogs non-modal so if they're modal on your computer then perhaps it's a bug.

Just to be clear, you have to show the Thunderbird window to open the event dialog but then you can minimize Thunderbird and the event dialog should remain in front.

Comment 2

[石庭豐 (Seak, Teng-Fong)]

(In reply to Pete Riley from comment #1)
> I can't reproduce it in Lightning 1.2 in Linux.

I've just tried Lightning 1.2 (in XP and TB 10) and I can confirm that event dialogs are NOT modal.  So there's certainly a bug in Lightning 1.4.
> 
> Just to be clear, you have to show the Thunderbird window to open the event
> dialog but then you can minimize Thunderbird and the event dialog should
> remain in front.

When I minimize TB, event dialog does not remain.

Comment 3

[石庭豐 (Seak, Teng-Fong)]

Lightning 1.3 did not have this modal dialog problem either.

I've just found where the error is.

In chrome\calendar\content\calendar\calendar-item-editing.js, at line 369, we have

features = "chrome,titlebar,resizable,dependent";

If we remove ",dependent" from the string, the problem will go away.

The same probably applies to "other targets" at line 374.

Comment 4

[石庭豐 (Seak, Teng-Fong)]

I've just tested TB 11 + LT 1.4 inside Debian 6 but the event dialog is NOT modal.  So this problem is only restricted to Windows (not only XP, but other Windows as well).

Is there a little hope to have Lightning 1.4.1 as a quick fix?

Comment 5

[Stefan Sitter [:ssitter]]

Regression from Bug 657755.

Comment 6

[石庭豐 (Seak, Teng-Fong)]

OK, I see.

It's at the introduction of attachment 578495 [details] [diff] [review] that "dependent" was added into "features" variable for WinNT by error.

Comment 7

[Alexandre Demers]

Yes, you could remove it. The patch I had proposed for bug 657755 was based on code in a different area of Lightning (as it had been suggested). I understand your need and how you use it. However, I'm wondering if this behavior was only available on Windows before the patch (I've personally never tested it). I'm not against removing "dependent", but if we do so, I think we should make sure we have the same behaviour on all OSes.

Comment 8

[石庭豐 (Seak, Teng-Fong)]

I could only test inside Win and Linux (Debian or Ubuntu).  I have no Mac :)

As far as I could see, event dialogs have always been non-modal for the latest versions of Sunbird and Lightning.

So now the question is basically like this:
"What is the best behaviour for event dialog? Modal or non-modal?"

If there's a vote, I would of course vote NON-modal.

Furthermore, in GUI convention, modal windows are only used for properties/options.  Now, event dialog should be considered at the same importance level as a mail window, so it should not be modal.

I have not yet read through the whole doc on Mozilla development, so I'm not sure how to check that code-change into Moz source code.  Could someone do it please?

Comment 9

[Alexandre Demers]

Attach a patch and ask for a review. You could think about philipp as a reviewer, or there is a small utility to help you find out who else could be a potential reviewer.

Comment 10

[Pete Riley]

Bug 122671(In reply to Alexandre Demers from comment #7)
> I'm wondering if this
> behavior was only available on Windows before the patch
I think bug 122671 makes it clear that the event dialog has been non-modal in all OSes for several years and is the intended behavior.

Comment 11

[Alexandre Demers]

(In reply to Pete Riley from comment #10)
> Bug 122671(In reply to Alexandre Demers from comment #7)
> > I'm wondering if this
> > behavior was only available on Windows before the patch
> I think bug 122671 makes it clear that the event dialog has been non-modal
> in all OSes for several years and is the intended behavior.

Then non-modal shall it be: release the PATCH! ;) Teng, do you want to propose a patch or should I do it?

Comment 12

[石庭豐 (Seak, Teng-Fong)]

I'm reading the long document to see how to make the patch :D

If you are patient enough, we might have the patch... let's see, two months later ^_^"

Comment 13

[Alexandre Demers]

(In reply to 石庭豐 (Seak, Teng-Fong) from comment #12)
> I'm reading the long document to see how to make the patch :D
> 
> If you are patient enough, we might have the patch... let's see, two months
> later ^_^"

I could do it for you if you need help since you already pointed out where and what to change in the code. Let me know.

Comment 14

[石庭豐 (Seak, Teng-Fong)]

For the moment I'm still pretty lost... maybe I need more time to read docs carefully...?

It seems that I have to check it's working with the latest source code?  That means I have to test it with TB 15 and Lightning (or Aurora or Earlybird??) 1.something?

So, yes, please do.

Comment 15

[Alexandre Demers]

Attachment: Fix regression where event dialogs are not non-modal anymore

It fixes a regression introduced by patch for 657755 where the dialog is not non-modal anymore.

Comment 16

[Matthew Mecca [:mmecca]]

Comment on attachment 624645 [details] [diff] [review]
Fix regression where event dialogs are not non-modal anymore

If Richard has no objections, r=mmecca

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 624645 [details] [diff] [review]
Fix regression where event dialogs are not non-modal anymore

Review of attachment 624645 [details] [diff] [review]:
-----------------------------------------------------------------

On OSX the dialog is also non modal and this change makes the behavior consistent over all systems.

ui-r=me

Comment 18

[石庭豐 (Seak, Teng-Fong)]

As per my comment 4, the behaviour in Debian was already correct, ie the keyword "dependent" seems to have no effect and its presence doesn't make the dialog modal.  I don't know why.  So I don't know if we need to remove "dependent" or not. Let's hope its disappearance won't lead to another regression :)

On the other hand, let me suggest to add some comments in the code, like this:

     // reminder: event dialog should not be modal (cf bug 122671)
     var features; 
     if (Services.appinfo.OS == "WINNT") {
         // keyword "dependent" should not be used in WINNT (cf bug 752206)
         features = "chrome,titlebar,resizable";

Comment 19

[Alexandre Demers]

Attachment: Same as previous patch, but commented so we don't forget

v1: Removed "dependent" to make the event dialog non-modal.

v2: Reminder added, so we don't forget that event dialogs should be non-modal.

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 624715 [details] [diff] [review]
Same as previous patch, but commented so we don't forget

Tried also under Linux and saw no difference.

ui-r=me

Comment 21

[石庭豐 (Seak, Teng-Fong)]

Thanks to all :)

(Partially off-topic)
I tried to dig a little deeper into this "dependent" topic (correct me if I'm wrong):
I found that openDialog() shares the same "features" as window.open():
https://developer.mozilla.org/en/DOM/window.open#Window_functionality_features

It says, amongst others, that
1. "dependent" is not implemented on MacOS X. That explains why it's absent in the code :)
2. it will probably be removed in the future

I've found several appearances of this "dependent" elsewhere in the source code, like in calendar-event-dialog.js & calApplicationUtils.js  IMO, it's better to remove them now.

Comment 22

[Matthew Mecca [:mmecca]]

Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/0094bc46084a

Comment 23

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

Comment on attachment 624715 [details] [diff] [review]
Same as previous patch, but commented so we don't forget

Approved for aurora/beta. I'll be spinning (or piecing together) the beta builds tomorrow or thursday, so I'd appreciate if someone could push this.

Comment 24

[Stefan Sitter [:ssitter]]

Pushed to https://hg.mozilla.org/releases/comm-aurora/rev/5533c6ed0ecd

Comment 25

[Stefan Sitter [:ssitter]]

Pushed to https://hg.mozilla.org/releases/comm-beta/rev/5869020d2976