1668478 - Ctrl+Enter to save and close New Event dialog window creates unwanted duplicate calendar event

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[tobias.thommes]

Attachment: Screenshot 2020-10-01 113331.JPG

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

• double-click day in calender view / press Ctrl-i to enter a new event
• confirm / save event by pressing Ctrl-Enter

Actual results:

The event is added twice in the calender!

Expected results:

The event should have only be added once (as it happens correctly when pressing the Save-Button in the Dialog)

Comment 1

[WaltS48 [:walts48]]

Double clicking on a a day in the Calendar opens an Event creation dialog for me, so does pressing Ctrl+I.

So now I have two creation dialogs open or did you mean use one or the other method to open the dialog?

I enter some text in the Title field and save the event by pressing Control+Enter. It closes leaving me with one event in the calendar and the other dialog is still open. Only one event appears in the calendar.

I do use the edit events in a tab method.

I can create duplicates using the edit in a dialog window method and using only one of the open the dialog methods.

Comment 2

[tobias.thommes]

I meant using one or the other method to open the dialog.
When I close the dialog by Ctrl+Enter I get a double event. When I close it with Enter only it creates a single event.
For me it does not matter wether I have opened two dialogs or only one in advance.

Comment 3

[WaltS48 [:walts48]]

(In reply to tobias.thommes from comment #2)

I meant using one or the other method to open the dialog.
When I close the dialog by Ctrl+Enter I get a double event. When I close it with Enter only it creates a single event.
For me it does not matter wether I have opened two dialogs or only one in advance.

Thanks for the clarification.

Does it happen if you enable "Edit events and tasks in a tab instead of in a dialog window" in Calendar preferences?

Comment 4

[tobias.thommes]

(In reply to WaltS48 [:walts48] from comment #3)

(In reply to tobias.thommes from comment #2)

I meant using one or the other method to open the dialog.
When I close the dialog by Ctrl+Enter I get a double event. When I close it with Enter only it creates a single event.
For me it does not matter wether I have opened two dialogs or only one in advance.

Thanks for the clarification.

Does it happen if you enable "Edit events and tasks in a tab instead of in a dialog window" in Calendar preferences?

No, then only single events are created. Also with this option set I can only save the event by pressing Crtl+Enter (or clicking the button), not by pressing Enter alone.

Comment 5

[WaltS48 [:walts48]]

Thanks again.
I've used edit in a tab ever since it became available.

Okay, it works properly in version 68.12.0 on Ubuntu 18.04 LTS.

Comment 6

[Alice0775 White]

I can reproduce the issue when pressing Crtl+Enter in "New Event:" dialog.

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=b12420c26104dd203307856e5086cb3139493c89&tochange=4a37dadf68195123ad4e38b39487a64c15f28d18
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21d22b2f541258d3d1cf96c7ba5ad73e96e616b5&tochange=155a7e2117e575ff6de6caa3dfe5b076cb455ae1

Comment 7

[Alice0775 White]

Suspect Bug 1544916

Comment 9

[Lasana Murray]

Attachment: 1668478.patch

The MozDialog element installs a keypress listener that triggers the "accept dialog" when the enter key is pressed. See here https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/dialog.js#23

Having a Ctrl+Enter shortcut in dialog mode executes both our shortcut command and triggers the code path linked to above.

Comment 10

[Lasana Murray]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3c93675e18247c78ddf7f79986e4db1042061412

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9183267 [details] [diff] [review]
1668478.patch

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

Looks ok to me, but let's have someone calendar/ review

Comment 13

[Alex Willis]

I'm also experiencing this. The trouble is I like to edit in a window not a tab so I can have the event details on my second screen while I type notes from it on my primary screen.

Comment 15

[Thomas D. (:thomas8)]

Comment on attachment 9183267 [details] [diff] [review]
1668478.patch

Requested calendar reviewer (:Fallen) seems busy, no response for 25 days.

Let's land this so that keyboard users stop suffering, and to avoid getting more duplicates and wasting triage time on that. This bug significantly affects calendar productivity/ux-efficiency.

The code change looks correct, the inbuilt dialog handler checks for (event.keyCode == KeyEvent.DOM_VK_RETURN), so that clearly triggers for Ctrl+Enter (probably intentional, for exactly this use case of multi-line inputs where enter creates a new line, so Ctrl+Enter is needed to accept the dialog).

I'm not an expert on tests, but the test LGTM, too, and I trust Lansana's coding skills.
If there's anything wrong with the test, it will fail and we can look at it again.
If we want certainty on that, let's have a try build.
But let's move on!

Comment 16

[Thomas D. (:thomas8)]

(In reply to Lasana Murray from comment #10)

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3c93675e18247c78ddf7f79986e4db1042061412

Try run LGTM, no related failures. If I overlooked something, please educate me.

Comment 17

[Magnus Melin [:mkmelin]]

Thomas, you're not a reviewer. Let's follow protocol.

Comment 18

[Geoff Lankow (:darktrojan)]

Comment on attachment 9183267 [details] [diff] [review]
1668478.patch

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

This makes sense to me but I'd like the test to be a bit more robust.

::: calendar/test/browser/eventDialog/browser_eventDialog.js
@@ +372,5 @@
> +/**
> + * Test that using CTRL+Enter does not result in two events being created.
> + * This only happens in the dialog window. See bug 1668478.
> + */
> +add_task(async function testCtlEnterShortcut() {

Missing an 'r' in the function name?

@@ +377,5 @@
> +  createCalendar(controller, CALENDARNAME);
> +  switchToView(controller, "day");
> +  goToDate(controller, 2020, 9, 1);
> +
> +  let createBox = lookupEventBox("day", CANVAS_BOX, null, 1, 8);

Why do this in the day view if you're going to check the result in the month view? If it's a way to work around the issue I mention below, it's clever but I'm not convinced.

@@ +393,5 @@
> +  Assert.equal(
> +    document.querySelectorAll("calendar-month-day-box-item").length,
> +    1,
> +    "event was created once"
> +  );

We might get problems here if the element hasn't appeared by the time the test code looks for it. Or if the second event turns up after this check happens. The elements appear instantly most of the time but it isn't synchronous. If I were writing this test I'd probably call `controller.sleep(2000)` before checking (with a comment explaining what we're waiting for, of course).

Also, you should delete the event after testing.

Comment 20

[Lasana Murray]

Why do this in the day view if you're going to check the result in the month view?

That's mostly copied from other tests. I can't say I understand lookupEventBox well enough to use it how I'd like. Last time I tried using it for months it did not work well.

The elements appear instantly most of the time but it isn't synchronous.
Why is that? Is that under our control?

Comment 21

[Lasana Murray]

Attachment: bug1668478v2.patch

Included the controller.sleep(2000) call. Fixed typo, delete event.

Comment 22

[Lasana Murray]

Lots going on here but this patch appears to pass:

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c7cb82d3929b03b2100b033a86e0e231ff0ffaf5

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c0572b3b6798
Remove Ctrl-Enter shortcut from new calendar event dialog. r=darktrojan

Comment 25

[Martin Schröder [:mschroeder]]

Comment on attachment 9188475 [details] [diff] [review]
bug1668478v2.patch

Considering the multiple duplicate bug reports, this fix should be uplifted to beta and esr78.

[Approval Request Comment]
Regression caused by (bug #): bug 1544916
User impact if declined: Creating unwanted duplicates of events
Testing completed (on c-c, etc.): ?
Risk to taking this patch (and alternatives if risky): ?

Comment 26

[Wayne Mery (:wsmwk)]

Comment on attachment 9188475 [details] [diff] [review]
bug1668478v2.patch

[Triage Comment]
Approved for beta

Comment 27

[Dave Miller [:justdave]]

Thunderbird 84.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/78af282f7657

Comment 28

[WaltS48 [:walts48]]

Using Ctrl+Enter to save and close an event no longer creates a duplicate in the Day view in my testing of the 84.0b2 release candidate on Ubuntu 18.04 LTS Linux.

Comment 29

[Wayne Mery (:wsmwk)]

Comment on attachment 9188475 [details] [diff] [review]
bug1668478v2.patch

[Triage Comment]
Approved for esr78

Comment 30

[Rob Lemley [:rjl]]

Thunderbird 78.5.1:
https://hg.mozilla.org/releases/comm-esr78/rev/5ebd01edafb4

Comment 31

[Rob Lemley [:rjl]]

Attachment: 1668478_esr78_testfix.patch

This is to fix the test on c-esr78.

Comment 32

[WaltS48 [:walts48]]

Using Ctrl+Enter to save and close an event no longer creates a duplicate in the Day view in my testing of the 78.5.1 release candidate on Ubuntu 18.04 LTS Linux or Windows 10.

Comment 33

[Magnus Melin [:mkmelin]]

Thanks for testing, Walt! Once verified, please set the relevant status flag to verified.