1663303 - Calendar New Event window should require a title ("null" events are saved)

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect, P2)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1659380 +++

I notice it's also possible to save with no title at all entered which will create a "null" event.

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-0.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9174181 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-0.patch

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

::: calendar/lightning/content/lightning-item-iframe.js
@@ +1778,5 @@
>    let newTitle = cal.l10n.getCalString(strName) + ": " + getElementValue("item-title");
> +  sendMessage({
> +    command: "updateTitle",
> +    argument: newTitle,
> +    disable: !document.getElementById("item-title").value.length,

just .value is enough. But you already send the title, so you can let updateTitle check the new title instead

::: calendar/lightning/content/lightning-item-panel.js
@@ +439,5 @@
>  /**
>   * Update the title of the tab or window.
>   *
> + * @param {string} newTitle  The new title
> + * @param {boolean} disable Disable the "Save and Close" button

nit: - after disable

@@ +444,3 @@
>   */
> +function updateTitle(newTitle, disable) {
> +  document.getElementById("button-saveandclose").setAttribute("disabled", disable);

would use .disabled = disabled so we don't need to fix this for HTML compat later

Comment 3

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #2)

just .value is enough. But you already send the title, so you can let
updateTitle check the new title instead

It will be "New Event: event_name" or "New Task: task_name"(It appends prefix), so it's hard to check and "New Event" or "New Task" will be changed according to locale.

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-1.patch

Comment 5

[Paul Morris [:pmorris]]

Comment on attachment 9174191 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-1.patch

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

Looks like a good start, but we need to also disable the "Save" and "Save and Close" items in the "Event" or "Task" menu.  Also when the user hits the enter key to close the dialog, that saves the item.  So at minimum that should be disabled too, although showing some kind of notification that the event needs a title might be more helpful to the user.  Showing a notification could be deferred to a follow-up.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-1.patch

I have tried to cover all the test cases.
What I do here:
If a new event is created, all the buttons are disabled and menu items are hidden for both dialog and tab.
If user tried to create an event with drag and drop and then blurs it, dialog and tab will open up. If event name is still empty while he cancels it, we ask to delete that event.
Pressing enter in the dailog if event title is empty will no do anything.

Comment 7

[Paul Morris [:pmorris]]

Comment on attachment 9175196 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-1.patch

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

Getting closer, but I think this needs some more work and probably some UX discussion (see review comment with some thoughts on that).  Also, I'm getting some unexpected behavior.  When I do:
- drag to create an event, but don't enter a title for it
- un-focus/blur the new event, the dialog opens
- enter a title
- click close button on tab or dialog window
- prompt asks if I want to save the item, click to save
- prompt asks if I want to delete the item

The existing code here is already quite complex and I had trouble following the logic around the `gEventDelete` global variable.   If possible it would be good to avoid introducing a new global variable in this file, or if not then document it some more?

::: calendar/lightning/content/lightning-item-iframe.js
@@ +475,4 @@
>      document.documentElement.setAttribute("systemcolors", "true");
>    }
>  
> +  updateTitle();

I'm curious why this is needed now when it wasn't before?  Is it because we are no longer setting the value of the title field, so that no longer triggers the update of the window title?  

There's also an updateTitle call in the `loadDialog` function.  I wonder if we can avoid having it called in both places?

@@ +569,5 @@
>  }
>  
>  /**
> + * Util function to check if event should be deleted or not on basis
> + * of empty title.

I'd add a sentence to say more, like that it may also delete the event.

@@ +571,5 @@
>  /**
> + * Util function to check if event should be deleted or not on basis
> + * of empty title.
> + *
> + * @param  {object} calendarItem - Selected event or task item.

nit: Capital O for object type: `{Object}`

@@ +572,5 @@
> + * Util function to check if event should be deleted or not on basis
> + * of empty title.
> + *
> + * @param  {object} calendarItem - Selected event or task item.
> + * @param  {string} iframeId - (optional) iframe id of the tab to be closed

nit: `[iframeId]` to signal that it's optional.  I don't mind leaving the "(optional)" in the description for extra clarity as well.

@@ +573,5 @@
> + * of empty title.
> + *
> + * @param  {object} calendarItem - Selected event or task item.
> + * @param  {string} iframeId - (optional) iframe id of the tab to be closed
> + * @return {boolean} - True if the event should be deleted.

nit: the hyphen is not needed on @return lines.

@@ +580,5 @@
> +  // When user creates the event with drag operation, event gets added
> +  // in the calendar without title and title input gets focused. If title
> +  // input is empty and it gets blurred, the edit event window/tab opens up.
> +  // When user tries to cancel such events, we should ask to delete the item
> +  // from the calendar.

I would move this comment up into the description in the doc comment for the function.

Also, I'm not sure what the best UX would be here.  Maybe worth bringing Alex into the discussion.  Below are a couple other possible ways to handle when the user creates an event by dragging, and title is empty when the event is blurred. 

1. delete the event (don't open the dialog)
2. give the event a placeholder title like "Untitled" (don't open the dialog)
3. prompt the user to enter a title or delete the event (don't open the dialog)

I think something along these lines would be better than opening the dialog.

@@ +627,5 @@
> +      if (calendarItemDeleteOnCancel(calendarItem)) {
> +        return true;
> +      }
> +    }
> +  }

I think this section might be clearer if you just used the `if (iframe)` conditional to set the itemTitle and calendarItem variables, and then just did one `if (!itemTitle.value) {` block, that used `iframe ? iframe.id : null` for the second argument to `calendarItemDeleteOnCancel` (as you do below).

@@ +1832,5 @@
>    let newTitle = cal.l10n.getCalString(strName) + ": " + getElementValue("item-title");
> +  sendMessage({
> +    command: "updateTitle",
> +    argument: newTitle,
> +    disable: !document.getElementById("item-title").value,

nit: disable -> disableSaving or similar to make it clear what is being disabled

::: calendar/lightning/content/lightning-item-panel.js
@@ +459,5 @@
> +  }
> +  let saveMenu =
> +    document.getElementById("item-save-menuitem") ||
> +    document.getElementById("calendar-save-menuitem");
> +  saveMenu.hidden = disable;

Why not 'disabled' instead of 'hidden' for these menu items?  

But actually, instead of the individual buttons, menu items, etc., it looks like there are `<command>`s that you can disable that should cover all the buttons, menu items, keyboard shortcuts, etc.  See `cmd_save` and `cmd_accept` in `lightning-item-panel.inc.xhtml` and `calendar-event-dialog.xhtml`.  If you disable those I think that should cover it.

Comment 8

[Paul Morris [:pmorris]]

Alex, when you get a chance, I'm curious about your UX take on this. When a user drags to create an event in the day or week view, it is no longer given a "New Event" title, instead the title starts out empty, and we want to prevent events from being saved with a missing/blank title. Currently if the user doesn't enter a title and blurs the new event by clicking somewhere else, the event dialog opens to allow adding a title. Then if the user closes the dialog without adding a title, what do we do? One option is ask if they want to delete the event. (The current patch does this.) Or we might want to do something instead of opening the dialog when an event without a title is blurred:

1. delete the event
2. give the event a placeholder title like "Untitled" (similar to previous behavior)
3. prompt the user to enter a title or else delete the event

I think 2 or something like it makes sense to me. It doesn't disrupt the user's flow by opening a dialog and it avoids saving an event without a title value.

A new glitch: I just noticed that if I click an existing event to edit the title "inline" (no dialog) and leave the title empty/blank, and then click to blur, the dialog opens, but with the title intact, like I hadn't changed it.

Another glitch: if I open an existing event (that has a title) in the dialog and delete the title then close the dialog without saving, it asks if I want to save the changes. I save them, then the event in the week view shows "Untitled". So I was able to (re-)save an existing event without a title. Then if I then open the event again in the dialog, and close it again, it asks if I want to delete the item. (This is with Khushil's current patch.)

Comment 9

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #8)

Another glitch: if I open an existing event (that has a title) in the dialog and delete the title then close the dialog without saving, it asks if I want to save the changes. I save them, then the event in the week view shows "Untitled". So I was able to (re-)save an existing event without a title. Then if I then open the event again in the dialog, and close it again, it asks if I want to delete the item. (This is with Khushil's current patch.)

Ohh, I missed this case. I will disable the save button here then.

Comment 10

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #7)

The existing code here is already quite complex and I had trouble following
the logic around the gEventDelete global variable. If possible it would
be good to avoid introducing a new global variable in this file, or if not
then document it some more?

gEventDelete is needed when we prompt the delete dialog box (we will open this delete dialog when the user tries to cancel the event with an empty title) and the user clicks on the delete button in the delete dialog box. Clicking on the delete button will again trigger the close window or tab event and the delete dialog will reappear because the event title will be still empty, it will end up in the loop. We want to avoid that.

Comment 11

[Alessandro Castellani [:aleca]]

Currently if the user doesn't enter a title and blurs the new event by clicking somewhere else, the event dialog opens to allow adding a title.

I think this shouldn't happen and we should always have a default title for events.
I think we have the solution right now since we have an already translated placeholder (New Event), so we should grab that and save it as title if the user saves the event without a written title.

Also on blur, we shouldn't open the dialog for edits, that's a bit jarring and unexpected.
Maybe the user wants to quickly reserve a slot in the calendar without writing anything, so we shouldn't force the user to edit an event if he/she doesn't want it.

My suggestion is to use the placeholder.

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-2.patch

Comment 13

[Paul Morris [:pmorris]]

I was just discussing some of the details of this with Khushil on chat. I agree with Alex in comment 11, but I think "Untitled" would be a better placeholder to use, and we already have it as a translated string. (It is what is shown in an event in the calendar views when its title is null.)

Reason: "New Event" as a placeholder can be inaccurate and misleading because all events become old in the course of time. Also an event might already be an old already existing one that the user just deleted the title. So "Untitled" is better because it never becomes inaccurate/misleading.

Comment 14

[Paul Morris [:pmorris]]

Comment on attachment 9176268 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-2.patch

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

You may already be working on this after our chat conversation, but I thought I'd go ahead and review.  Just a few comments, this is looking good overall and working well.  Like we discussed it would be good to default to "Untitled" as a title when the user closes an edit dialog with an empty title field, and then chooses to save it in the save prompt.

::: calendar/base/content/calendar-editable-item.js
@@ +429,4 @@
>            this.mOccurrence,
>            null,
>            null,
> +          this.eventNameTextbox.value || cal.l10n.getCalString("newEvent")

Like we discussed (after you did this patch) let's use the "Untitled" string here.

::: calendar/test/browser/recurrence/browser_biweekly.js
@@ +19,4 @@
>    menulistSelect,
>    switchToView,
>    viewForward,
> +  inputValue,

Needed? Seems unused.

::: calendar/test/modules/CalendarUtils.jsm
@@ +46,4 @@
>    "openLightningPrefs",
>    "closeLightningPrefs",
>    "menulistSelect",
> +  "inputValue",

Is this needed here?  Seems like it's not defined in this file?

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-3.patch

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

Comment 16

[Paul Morris [:pmorris]]

Comment on attachment 9176459 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-3.patch

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

This is working well.  I definitely like "Untitled".  Just a few thoughts on how we could refactor the code a bit while we're here, on reading it again.  (Aside: "New Event" for the title placeholder seems odd to me, I wonder if we could eventually improve on that, something like "Event Title" or "Task Title" would seem more appropriate, but that's out of scope for this bug.)

::: calendar/lightning/content/lightning-item-iframe.js
@@ +557,5 @@
>    );
> +  let itemTitle = document.getElementById("item-title");
> +  if (choice == 0 && !itemTitle.value) {
> +    itemTitle.value = cal.l10n.getCalString("eventUntitled");
> +  }

I think it would be simpler to put this below in the switch branch for case 0.  No need to get the title unless the user clicked save.  You can wrap it in a block like this:

case 0: { // Save
  let itemTitle = ...
  ...
  return true;
}
case 1:

@@ +1778,3 @@
>      throw Components.Exception("", Cr.NS_ERROR_NOT_IMPLEMENTED);
>    }
>    let newTitle = cal.l10n.getCalString(strName) + ": " + getElementValue("item-title");

Looking at this again, I think it would be better to not assemble the two parts of the string here, but send them as two separate strings: the "prefix" and the "title".

@@ +1780,5 @@
>    let newTitle = cal.l10n.getCalString(strName) + ": " + getElementValue("item-title");
> +  sendMessage({
> +    command: "updateTitle",
> +    argument: newTitle,
> +    disableSaving: !document.getElementById("item-title").value,

So this could be as follows, and then on the receiving end of the message we can disable saving or not based on whether the title is an empty string.

sendMessage({
    command: "updateTitle",
    prefix: cal.l10n.getCalString(strName),
    title: getElementValue("item-title")
})

::: calendar/lightning/content/lightning-item-panel.js
@@ +450,2 @@
>   */
> +function updateTitle(newTitle, disableSaving) {

I think this would be better if it were two separate functions "updateTitle" and "disableSaving", one to update the title and one to disable saving.  That makes it clear what the functions are doing, and make it possible to reuse the disable saving one.  

The updateTitle function could call the disableSaving function, passing a boolean determined by whether the title was an empty string.

@@ +455,5 @@
> +  }
> +  let cmdAccept = document.getElementById("cmd_accept");
> +  if (cmdAccept) {
> +    cmdAccept.setAttribute("disabled", disableSaving);
> +  }

The prefix and title could be combined here in the updateTitle function, with ": " separating them

Comment 17

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-4.patch

Comment 18

[Paul Morris [:pmorris]]

Comment on attachment 9176528 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-4.patch

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

This looks good and works great!  Just one minor suggestion.  Thanks for making late-breaking changes.

::: calendar/lightning/content/lightning-item-panel.js
@@ +446,5 @@
> + * Disable the saving options according to the item title.
> + *
> + * @param {string} title - The item title.
> + */
> +function disableSaving(title) {

Nit: we might want to use this function in other ways not related to the title, so it would be better to make it more independent by reworking the argument, naming it something like `disabled`, then passing `!title` to it.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event-5.patch

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d4b09b3b74e
Fix calendar new event window should require a title. r=pmorris DONTBUILD

Comment 21

[Khushil Mistry [:khushil324]]

Comment on attachment 9176563 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-5.patch

[Approval Request Comment]
Regression caused by (bug #): 1659380
User impact if declined: UX related to the event title needs to be improved.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low

Comment 22

[Wayne Mery (:wsmwk)]

Comment on attachment 9176563 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-5.patch

[Triage Comment]
This will be on 82 beta without uplift

[Triage Comment]

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9176563 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event-5.patch

[Triage Comment]
Approved for esr78

Comment 24

[Rob Lemley [:rjl]]

Uplifting failed. It looks like this bug depends on another one that renamed the test function from "invokeEventDialog" to "invokeNewEventDialog".
Khushil?
I will try look for it as well.

Comment 25

[Rob Lemley [:rjl]]

Bug 1575195 changed invokeEventDialog to invokeNewEventDialog as part of the work to split up to split the dialog into viewing and editing dialogs. That is not going to be uplifted. Khushil, could you make a version of this patch for 78?

Comment 26

[Khushil Mistry [:khushil324]]

Sure, will be uploading a patch for ESR 78 ASAP.

Comment 27

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663303_disable-save-close-no-title-event_ESR78.patch

Patch for ESR 78.

Comment 28

[Rob Lemley [:rjl]]

Comment on attachment 9179826 [details] [diff] [review]
Bug-1663303_disable-save-close-no-title-event_ESR78.patch

[Triage Comment]
Moved approval to ESR version of patch.
This will probably be held until 78.4.0 unless build2 of 78.3.2 becomes necessary.

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 78.3.3:
https://hg.mozilla.org/releases/comm-esr78/rev/fd10970ce284