1651779 - Hide reminder dropdown in event summary dialog when the event is not an invitation

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Lasana Murray]

This will come up once bug 1575195 lands. Except from that bug:

For regular non-read-only events, reminders seems to be editable because there is a drop down menu for them, but there is no way to save a change. We should either make editing and saving reminders possible from the summary dialog, or don't offer the menu.

Comment 1

[Lasana Murray]

What would be the desired behavior here? Personally, I like being able to quickly set a reminder from the summary view.

Comment 2

[Magnus Melin [:mkmelin]]

Since it's editing the event, I'd lean towards having it only in the edit dialog. Being able to do this and that edit quickly will just give the full editing dialog in the end...

Comment 3

[Lasana Murray]

Attachment: hide-reminder-dropdown.patch

Made the event-summary widget only display the reminder dropdown if the event is not read-only and is an invitation. Updated a previous test to also ensure it remains hidden.

Comment 4

[Paul Morris [:pmorris]]

Comment on attachment 9171481 [details] [diff] [review]
hide-reminder-dropdown.patch

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

After a quick look these changes look good, but... rather than hiding the reminder menu, I think it would be better to show the value of the reminder property as read-only text.  That way you can see if you have a reminder set, and what kind, from right there in the summary dialog.  Otherwise you have to click through to the edit dialog.  Sorry I didn't say this when you asked about the desired behavior here earlier.

Previously we didn't show a reminder menu for read-only items, only for items that were invitations.  I think we can do the following going forward, treating read-only and editable (non-invitation) items the same:

- invitation items: show reminder menu to allow setting reminders (since you cannot edit an invitation via edit dialog)
- editable items: show reminder as read-only text, if the reminder property is set
- read-only items: show reminder as read-only text, if the reminder property is set

For custom invitations there should be existing code that can express those as text, as we do in the edit dialog.

Comment 5

[Lasana Murray]

Attachment: bug1651779.patch

Includes feedback from previous patch. Reminder details will show as read-only or a dropdown depending on the event.

Comment 6

[Paul Morris [:pmorris]]

Comment on attachment 9175580 [details] [diff] [review]
bug1651779.patch

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

This works mostly as expected when I tried it, except I think we should just not show the reminders row if there are no reminders.  Just a few implementation style comments.

Unfortunately, our current code does not really help us do what we want in an elegant way.  Having to have a hidden menu in the DOM in order to get a text representation of a reminder is not ideal, but it will work for now.  It would be nice if we just had a function that took an array of reminders/alarms and returned a string representation of them, but maybe that's a task for another day (easier once strings have moved to fluent).  

Great to have tests included!  Would be even better if they were in the mochitest rather than mozmill style, but fine to stick with mozmill for now if you like, to speed things up.  Eventually we'll want to migrate towards mochitests, so the more we can do new tests in mochitest style, the better.

::: calendar/base/content/widgets/calendar-item-summary.js
@@ +222,5 @@
> +                  &read.only.reminder.label;
> +                </html:th>
> +                <html:td>
> +                  <hbox class="reminder-read-only-row-details" align="center">
> +                    

Here's an unneeded extra empty line.

@@ +225,5 @@
> +                  <hbox class="reminder-read-only-row-details" align="center">
> +                    
> +                  </hbox>
> +                </html:td>
> +              </html:tr>

Instead of having two rows for reminders, and hiding or showing one or the other, can we re-use the same row?  Like, if it turns out we only need to show a textual representation of the reminders, then remove the menu elements and replace it with the text.

Ideally we could just set up the common outer container elements here and not even render the menu elements unless they are needed.  Once we know what kind of item we're dealing with, and whether it has any reminders, we could determine which kind of reminder row content we need (menu, text, nothing/hidden), and add the content to the row then.  

Unfortunately, without some significant refactoring, the current code requires us to have a menu to get the reminder string, so it may or may not be worth doing this at this point.  Up to you.  (It might help with performance of the import dialog where you may have many many events all rendered at once, each with a reminder menu that typically will stay hidden.)

@@ +433,3 @@
>  
> +      let supportsReminders =
> +        argCalendar.getProperty("capabilities.alarms.oninvitations.supported") !== false;

Looks like we can just inline this and drop the `argCalendar` variable: `item.calendar.getProperty(...)`

@@ +436,2 @@
>  
> +      if (supportsReminders) {

Seems like we should also have a check somewhere here to see if the item even has any reminders set, since in that case we don't even need to show a reminders row.

@@ +439,5 @@
> +          item.getAlarms(),
> +          this.mAlarmsMenu,
> +          this.mItem.calendar
> +        );
> +

Maybe add a comment here like:
// For invitations where the reminders can be edited,
// show a menu to allow setting the reminder,
// because you can't edit an invitation in the edit item dialog.
// For all other cases, show a plain text representation of the reminders.

@@ +521,5 @@
> +      this.updateReminder();
> +
> +      let hbox = this.querySelector(".reminder-read-only-row-details");
> +      let node = document.createTextNode(
> +        this.mAlarmsMenu.getItemAtIndex(this.mAlarmsMenu.selectedIndex).label

This works for non-custom reminders, but for custom reminders, there's additional text we can display to give more details.  The current code puts this text in the `reminder-details` element, so we can just get it from there and append it so we show something like "Custom: First Thursday of the Month".

Also, currently we see the "..." like "Custom...".  It would be better without the "...".

::: calendar/test/browser/eventDialog/browser_eventDialog.js
@@ +283,5 @@
> +      Assert.ok(row.hidden, "reminder dropdown is not displayed");
> +      Assert.ok(!readOnlyRow.hidden, "read-only row is displayed.");
> +      Assert.ok(
> +        hbox.textContent.includes("No reminder"),
> +        '"No reminder" is shown when there is no reminder'

I think it would be better to just not show any reminder row if there are no reminders.

Comment 7

[Lasana Murray]

This works mostly as expected when I tried it, except I think we should just not show the reminders row if there are no reminders.

I did the patch like that to be consistent with the "No reminder" default in the dropdown. My reasoning was, it explicitly tells the user no reminder is set rather than nothing at all while the "No reminder" value shows in the editing dialog.

Seems like we should also have a check somewhere here to see if the item even has any reminders set.

loadReminders exits early if no reminders are set. See here:
https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-dialog-utils.js#303

Comment 8

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #7)

This works mostly as expected when I tried it, except I think we should just not show the reminders row if there are no reminders.

I did the patch like that to be consistent with the "No reminder" default in the dropdown. My reasoning was, it explicitly tells the user no reminder is set rather than nothing at all while the "No reminder" value shows in the editing dialog.

I see. I think it would be better to keep it consistent with the way the rest of the event properties are handled, where if no value is set, the row is not shown at all. (That also allows us to improve efficiency in the import dialog which may have many events by not rendering the menu at all when there are no reminders.)

Seems like we should also have a check somewhere here to see if the item even has any reminders set.

loadReminders exits early if no reminders are set. See here:
https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-dialog-utils.js#303

Ah, that's true, but my thought was to avoid rendering the menu at all if there are no reminders, and I think that would involve an earlier check for whether reminders exist. In that case we wouldn't need the check in loadReminders anymore.

Comment 9

[Lasana Murray]

Alright.

The current code puts this text in the reminder-details element, so we can just get it from there

You mean this? I notice it displays Multiple Reminders... if there are more than one. Is that ok? (Edited)

https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-item-summary.js#201

Comment 10

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #9)

The current code puts this text in the reminder-details element, so we can just get it from there

You mean this? I notice it displays Multiple Reminders... if there are more than one. Is that ok? (Edited)

https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-item-summary.js#201

Yeah, that's it, and "Multiple Reminders..." is fine. Let's remove the "..." part though, since it won't be a link to open the reminders dialog. (We might be able to show details for more than one reminder in the future at some point, but that seems like a separate thing, since we'd want to do the same kind of thing in the edit dialog too.)

Comment 11

[Lasana Murray]

One other thing:

show something like "Custom: First Thursday of the Month".
Also, currently we see the "..." like "Custom...". It would be better without the "...".

won't this break when localized?

Comment 12

[Paul Morris [:pmorris]]

Hm, good point. I see the "..." is part of the localized strings, which makes sense. So let's leave it as-is for now, i.e. just use the localized string with the "..." in it. Eventually it would be good to add localized strings without the "..." since it seems out of place in this plain text use (as opposed to a link).

Comment 13

[Lasana Murray]

Attachment: event.png

It may be more legible without prefixing the "Custom..." part. See attached.

Comment 14

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #13)

It may be more legible without prefixing the "Custom..." part. See attached.

Yeah, good call, let's just drop the "Custom..." part. It's not really needed in this context.

Comment 15

[Lasana Murray]

Tests for browser_import.js fail with the changes I've made. Looks like the calendar property of calIItemBase is not set during the parsing phase.
I'm going to add a check to avoid causing the error. The idl file for iCalItemBase does not indicate the calendar attribute is nullable/optional though, so it's a bit misleading.

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

Here is the start of the code path for reference:
https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-ics-file-dialog.js#36

I also noticed that making a calendar read-only that already has reminders will hide the reminder details, I'm going to update the logic so it always shows once there are reminders.

Comment 16

[Lasana Murray]

Attachment: bug1651779v3.patch

Changes made. browse_import test no longer fails.

Comment 17

[Lasana Murray]

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

Comment 18

[Paul Morris [:pmorris]]

Comment on attachment 9176672 [details] [diff] [review]
bug1651779v3.patch

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

Nice work!  This does the job nicely, and with test coverage.  Just a couple of minor refinement comments.  

To take this a step further, in a follow-up patch, for better performance in the ICS import dialog, the next step would be... to remove the xhtml markup for the reminders menu from the big xhtml string in the connectedCallback and put it in a const just above the class, or in a static property on the class.  Then at the top of `updateReminder`, check whether the menu is there yet or not, and if not then do `tdElement.appendChild(MozXULElement.parseXULToFragment(this.reminderMenuXhtmlString)` to render the menu.  That way if we don't need the menu it is never rendered into the DOM.

This would probably be overkill except for the ICS import dialog having potentially many many events to render, most of which likely don't have reminders, and the need to improve performance there.  But I'm fine to go ahead and land this and leave that optimization for later if there are other priorities.

::: calendar/base/content/widgets/calendar-item-summary.js
@@ +341,5 @@
>  
>        // When used in places like the import dialog, there is no calendar (yet).
>        if (item.calendar) {
>          this.mCalendar = cal.wrapInstance(item.calendar, Ci.calISchedulingSupport);
> +        this.mIsInvitation = this.mCalendar && this.mCalendar.isInvitation(item);

Up to you, but here you could use the new `?.` operator:

this.mIsInvitation = this.mCalendar?.isInvitation(item);

::: calendar/test/browser/eventDialog/browser_eventDialog.js
@@ +349,5 @@
> +    "DURATION:PT15M\r\n" +
> +    "ACTION:DISPLAY\r\n" +
> +    "END:VALARM\r\n" +
> +    "END:VEVENT\r\n" +
> +    "END:VCALENDAR\r\n";

Here you could use template strings with `dedent`, like this:
https://searchfox.org/comm-central/source/calendar/test/unit/test_unifinder_utils.js#18-28
And I think it should work fine if you just use the part from BEGIN:VEVENT to END:VEVENT.

Comment 19

[Lasana Murray]

Up to you, but here you could use the new ?. operator:

Is this valid syntax? I have not seen this operator before.

Here you could use template strings with dedent

dedent does not appear to be in scope , I see a definition in head_const.js but that's not a jsm module.

Comment 20

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #19)

Up to you, but here you could use the new ?. operator:

Is this valid syntax? I have not seen this operator before.

It's new, and very handy, see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

Here you could use template strings with dedent

dedent does not appear to be in scope , I see a definition in head_const.js but that's not a jsm module.

Hm, looks like dedent has only been used in unit/xpcshell tests so far and not browser/mochitests. I think it would need to move into calendar/test/modules/CalendarUtils.jsm and then imported from there. Better to do that in a separate patch or separate bug though.

Comment 21

[Lasana Murray]

Better to do that in a separate patch or separate bug though.

There are one or two things I see in head* files that would be useful in CalendarUtils.jsm. What prompts functions to be added there instead of in CalendarUtils?

I think the calendar testing utilities could do with some refactoring and new helpers so that writing calendar tests is a faster experience. Maybe create an actual CalendarTestUtils object and export it in BrowserTestUtils fashion?

Comment 22

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #21)

There are one or two things I see in head* files that would be useful in CalendarUtils.jsm. What prompts functions to be added there instead of in CalendarUtils?

I'm not sure, but maybe because they couldn't be used in mozmill tests? And now they can be used in mochitests. Makes sense to put any useful utility functions where they can be used by both xpcshell and mochitests.

I think the calendar testing utilities could do with some refactoring and new helpers so that writing calendar tests is a faster experience. Maybe create an actual CalendarTestUtils object and export it in BrowserTestUtils fashion?

Sounds good to me, especially as part of migrating away from mozmill to standardize on mochitest style. Might be worth checking in with Geoff about it, see if he has any thoughts.

Comment 23

[Lasana Murray]

Attachment: bug1651779v4.patch

I made the change for the optional chaining operator. The other changes should probably be handled in separate bugs as you mentioned. The priority here was really to hide the broken dropdown from users.

Comment 24

[Paul Morris [:pmorris]]

Comment on attachment 9177005 [details] [diff] [review]
bug1651779v4.patch

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

Agreed this is ready to land, other bugs for test refactoring.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28456bd23dfd
Show the reminder as read-only text when event is not an invitation. r=pmorris