Closed
Bug 752206
Opened 9 years ago
Closed 9 years ago
event dialog is not non-modal anymore
Categories
(Calendar :: Dialogs, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: lapsap7+mz, Assigned: alexandre.f.demers)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
1.46 KB,
patch
|
mmecca
:
review+
Paenglab
:
ui-review+
Fallen
:
approval-calendar-aurora+
Fallen
:
approval-calendar-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
(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.
Reporter | ||
Comment 2•9 years ago
|
||
(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.
Reporter | ||
Comment 3•9 years ago
|
||
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.
Reporter | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
Regression from Bug 657755.
Blocks: 657755
Keywords: regression
Summary: Wish: render event dialongs as non-modal → event dialog is not non-modal anymore
Reporter | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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.
Reporter | ||
Comment 8•9 years ago
|
||
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?
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
(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?
Reporter | ||
Comment 12•9 years ago
|
||
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 ^_^"
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Reporter | ||
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
It fixes a regression introduced by patch for 657755 where the dialog is not non-modal anymore.
Assignee | ||
Updated•9 years ago
|
Attachment #624645 -
Flags: review?(11010010)
Assignee | ||
Updated•9 years ago
|
Attachment #624645 -
Flags: review?(11010010) → review?(philipp)
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Updated•9 years ago
|
Assignee: nobody → alexandre.f.demers
Status: NEW → ASSIGNED
Reporter | ||
Comment 18•9 years ago
|
||
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";
Assignee | ||
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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+
Reporter | ||
Comment 21•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #624715 -
Flags: review?(matthew.mecca) → review+
Comment 22•9 years ago
|
||
Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/0094bc46084a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.7
Updated•9 years ago
|
Attachment #624715 -
Flags: approval-calendar-beta?
Attachment #624715 -
Flags: approval-calendar-aurora?
Comment 23•9 years ago
|
||
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+
Comment 24•9 years ago
|
||
Pushed to https://hg.mozilla.org/releases/comm-aurora/rev/5533c6ed0ecd
Target Milestone: 1.7 → 1.6
Comment 25•9 years ago
|
||
Pushed to https://hg.mozilla.org/releases/comm-beta/rev/5869020d2976
Target Milestone: 1.6 → 1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•