317477 - Lightning: Canceling 'Edit Event' dialog opens 'New Event' dialog

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Stefan Sitter [:ssitter]]

Canceling 'Edit Event' dialog opens 'New Event' dialog.

Steps to reproduce:
1) double click on existing event in day/week view to open 'Edit Event' dialog
2) dismiss dialog with 'cancel'

Works with 20051121 Lightning build.
Fails with 20051122 Lightning build.
Regression from bug 309734.

Comment 1

[Stefan Sitter [:ssitter]]

Assigning to myself as I'm willing to work on it.

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: patch

The problem in this case is:

Double click on entry triggers the following events:

calendar-event-box: 1st 'click' event --> start single click timeout (350msec)

calendar-event-box: 2nd 'click' event --> last click within 350msec check
                                      --> double click --> Edit Event dialog

calendar-event-box: 'dblclick' event --> not handled
calendar-event-column: 'dblclick' event --> New Event dialog

I solved the problem by adding a 'dblclick' event handler to calendar-event-box and moving the double click check stuff from the 'click' to 'dblclick' event handler.

I found no regression during testing (only on windows); double clicking and inline editing still work after this changes.

Comments please.

Comment 3

[Joey Minta]

Comment on attachment 204582 [details] [diff] [review]
patch

Nice work!  Now for the annoying nits:
+        // stop 'single click edit' timeout
+        if (this.editingTimer) {
Is it possible to arrive here and have there not be an editingTimer?  If not, we should probably remove this check.


+            var self = this;
+            if (this.editingTimer) clearTimeout(this.editingTimer);
+            this.editingTimer = setTimeout(function () { self.startEditing(); }, 350);

I know you didn't write this, but if we're going to move it around, we should probably make it look proper.  (clearTimeout in {} on a new line)

I apologize for taking so long to get to this review.  I should be able to get to them faster starting next week.

Comment 4

[Stefan Sitter [:ssitter]]

Attachment: patch v2

(In reply to comment #3)
> +        // stop 'single click edit' timeout
> +        if (this.editingTimer) {
> Is it possible to arrive here and have there not be an editingTimer?  If not,
> we should probably remove this check.

There are several code places where this.editingTimer is set to null. Also I don't know for sure if the order of events is always guaranteed to be the same. To be on the safe side we should keep that check.

> +            var self = this;
> +            if (this.editingTimer) clearTimeout(this.editingTimer);
> +            this.editingTimer = setTimeout(function () { self.startEditing();
> }, 350);
> 
> I know you didn't write this, but if we're going to move it around, we should
> probably make it look proper.  (clearTimeout in {} on a new line)

Done.

Comment 5

[Joey Minta]

Comment on attachment 205157 [details] [diff] [review]
patch v2

r=jminta

Comment 6

[Joey Minta]

patch checked in