401029 - Event editing dialog asks for confirmation on Ctrl-W, not on Enter

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[David Ascher (:davida)]

Steps to reproduce:

  1) Double click on an event to edit it.

  2) Make a change (e.g. change from having an alarm not not having one, or vice versa)

  3a) If you hit Ctrl-W (or Cmd-X on OS X) you get a confirmation dialog asking if you want to save the changes.

  3b) If you hit "Enter", the changes are saved, but there's no confirmation dialog.

The difference between 3a and 3b is a bug.

(FWIW, I think a confirmation dialog is not necessary in this case).

Comment 1

[David Ascher (:davida)]

Note that clicking the button "Save and Close" also doesn't ask for permission, so I think the close handler is the buggy bit.

Comment 2

[Stefan Sitter [:ssitter]]

The toolbar button "Save and Close" saves the event before closing the window. The menu command "Close Window" doesn't save the event before closing the window. Therefore the confirmation dialog is shown if the event changed. See related Bug 392737.

Comment 3

[Marc Weidner]

Bug still occurs with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre) Gecko/20080218 Calendar/0.8pre

Comment 4

[Pete Riley]

I also think that this is not a bug but is actually correct and well established behavior in computer programs.  I should only see the confirmation prompt if I'm about to lose changes that I made.  I wouldn't want to see the confirmation prompt if I press Enter or click 'Save and Close'.  Perhaps I don't understand the problem.

Comment 5

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

Just pressing "Enter" shouldn't be closing the dialog. Does this still happen with 1.0b1 or later?

Comment 6

[Pete Riley]

> Just pressing "Enter" shouldn't be closing the dialog.

Isn't that how all dialogs work:  you press Enter for the OK button or Escape for the Cancel button.  For example in Thunderbird's "Tools > Options" after you make changes, you press Enter and your changes are saved with no further prompts.

I know those two buttons aren't in the event dialog but why should it behave differently than all other Windows dialogs?  I see now that this is a Mac bug so maybe things work differently on that platform.

Plus there's the annoyance factor of the second dialog.  95% of the time when you press Enter you really want to save the event without having to press Enter a second time or moving the mouse to the OK button.  The 5% of the time when someone accidentally presses Enter he can simply open the dialog again and continue where he left off (no data was lost).

Comment 7

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

I'd rather expect Ctrl+Enter to save and close the dialog. This is not a standard dialog, its not the typical OK/Cancel dialog, it has very rich content and is more comparable to a standard window (has toolbar, etc).

Comment 8

[Pete Riley]

Ctrl-Enter makes sense.  That would be consistent with sending an email in Thunderbird's composition window.

Comment 9

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

Attachment: Fix - v1

I see what you mean. I never noticed this before, but you are right. Pressing Enter triggers the "accept" button in the dialog (even though the button is not really shown).

I don't think we should be reacting to Enter, instead we should use Ctrl+Enter. The fix isn't as clean as I had imagined. Toolkit's dialog.xml in some places makes me think they support defaultButton="none" to make sure no command is executed when pressing enter, but this causes a bunch of strict warnings.

Then I tried just adding a
<key id="prevent-enter" keycode="VK_ENTER" oncommand="event.preventDefault()"/>
but no matter which phase I set, the dialog accept is called before the key is triggered.

I realize this doesn't really fix the original issue fully though. What we could do is offer a 
---------------------------------
Are you read to save this event?

[ ] Don't show this message again
                   [ Yes ] [ No ]
---------------------------------

type message, similar to Thunderbird's compose dialog, but since it requires more work and the bug is quite minor, I think we should postpone that to a different bug.

For now, with this patch you won't accidentally be pressing enter and having your event saved.

Comment 10

[Markus Adrario [:Taraman]]

I personally lived well with ENTER only to save & close.

For the confirmation dialog I would like to point to comment #2. Please don't make a confirmation on an operation that causes no data loss.
[exaggeration on]
        "You pressed the <shift> key, did you really want to press this key?"
        ;-)
[exaggeration off]
When closing without saving the confirmation is a good thing, but not when the event is saved.

Comment 11

[Markus Adrario [:Taraman]]

Comment on attachment 429743 [details] [diff] [review]
Fix - v1

Patch looks good.

Two little nits:
I got some rejects on applying:
>-    <keyset>
>+    <keyset id="calendar-event-dialog-keyset">
>         <key id="new-event-key"
>              modifiers="control"
>              key="&event.dialog.new.event.key;"
>              command="cmd_file_new_event"/>
>         <key id="new-message-key"
>              modifiers="accel"
>              key="&event.dialog.new.message.key;"
>              command="cmd_file_new_message"/>
>         <key id="close-key"
>              modifiers="accel"
>              key="&event.dialog.close.key;"
>              command="cmd_file_close"/>
>         <key id="save-key"
>              modifiers="accel"
>              key="&event.dialog.save.key;"
>              command="cmd_save"/>
>+        <key id="accept-key"
>+             modifiers="&event.dialog.accept.modifier;"
>+             keycode="&event.dialog.accept.keycode;"
>+             command="cmd_accept"/>

And Scondly:
>+<!-- LOCALIZATON NOTE
>+     The key and its modifier to close the dialog. This can not be VK_RETURN by
>+     itself, since this key is prevented in the javascript code. Any modifier
>+     together with VK_RETURN is valid though. -->
>+<!ENTITY event.dialog.accept.modifier             "accel">
>+<!ENTITY event.dialog.accept.keycode              "VK_RETURN">
Does this really have o be localizable?
modifiers are hardcoded in all the other keys here and the VK_ENTER should be the same in all languages.
So why should localizer have to bother with that.

With at least the rejects fixed
r=markus

Comment 12

[Markus Adrario [:Taraman]]

meant VK_RETURN in the second comment of course.

Comment 13

[Stefan Sitter [:ssitter]]

Comment on attachment 429743 [details] [diff] [review]
Fix - v1

With this patch applied it is no longer possible to press Return to create multi-line description comments.

Comment 14

[Markus Adrario [:Taraman]]

Comment on attachment 429743 [details] [diff] [review]
Fix - v1

Thanks stefan for noticing that.

changing to r- in that case.

Comment 15

[Stefan Sitter [:ssitter]]

Current behavior (pressing Return does Save & Close) is specified by the dialog itself. You could change onAccept() to show different behavior.
 
Or you could fix Bug 392737 and assign Ctrl-W / Cmd-X to the new Save & Close command to unify behavior. This would get rid of the unnecessary confirmation dialog as mentioned in comment #0 and comment #4.

Comment 16

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

Attachment: Fix - v2

Different approach here. Override the _hitEnter function of the dialog to avoid Enter causing cmd_accept.

I think that now we have a key for Save and Close, leaving Ctrl+W for close (with confirmation) is reasonable.

Comment 17

[Markus Adrario [:Taraman]]

Comment on attachment 433816 [details] [diff] [review]
Fix - v2

This patch works.

The only thing that bugs me is that we have two keys for the same thing now (accel + L from the menuitem & accel + Enter)
SInce we cannot assign VK_RETURN as accesskey in the menu, I fear that we have to live with this.

Since this patch is bitrotten, it gets an r-
I'll affix a fixed version after writing this.

Comment 18

[Markus Adrario [:Taraman]]

Attachment: Patch V2 - de-bitrotten -

The fixed version of the patch as mentioned above

Comment 19

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/f0450df04eb9>
-> FIXED

Comment 20

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

Redundancy is fine, this gives the user two possibilities, one of which he may prefer. I checked in your de-bitrotted patch. Thanks!