Closed Bug 752206 Opened 8 years ago Closed 7 years ago

event dialog is not non-modal anymore

Categories

(Calendar :: Dialogs, defect)

Lightning 1.4
x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lapsap7+mz, Assigned: alexandre.f.demers)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
(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.
(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.
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.
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?
Regression from Bug 657755.
Blocks: 657755
Keywords: regression
Summary: Wish: render event dialongs as non-modal → event dialog is not non-modal anymore
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.
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.
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?
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.
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.
(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?
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 ^_^"
(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.
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.
It fixes a regression introduced by patch for 657755 where the dialog is not non-modal anymore.
Attachment #624645 - Flags: review?(11010010)
Attachment #624645 - Flags: review?(11010010) → review?(philipp)
Comment on attachment 624645 [details] [diff] [review]
Fix regression where event dialogs are not non-modal anymore

If Richard has no objections, r=mmecca
Attachment #624645 - Flags: ui-review?(richard.marti)
Attachment #624645 - Flags: review?(philipp)
Attachment #624645 - Flags: review+
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
Attachment #624645 - Flags: ui-review?(richard.marti) → ui-review+
Assignee: nobody → alexandre.f.demers
Status: NEW → ASSIGNED
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";
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.
Attachment #624645 - Attachment is obsolete: true
Attachment #624715 - Flags: ui-review?(richard.marti)
Attachment #624715 - Flags: review?(matthew.mecca)
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
Attachment #624715 - Flags: ui-review?(richard.marti) → ui-review+
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.
Attachment #624715 - Flags: review?(matthew.mecca) → review+
Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/0094bc46084a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.7
Attachment #624715 - Flags: approval-calendar-beta?
Attachment #624715 - Flags: approval-calendar-aurora?
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.
Attachment #624715 - Flags: approval-calendar-beta?
Attachment #624715 - Flags: approval-calendar-beta+
Attachment #624715 - Flags: approval-calendar-aurora?
Attachment #624715 - Flags: approval-calendar-aurora+
You need to log in before you can comment on or make changes to this bug.