1647855 - Provide a "Edit All Occurrences" button in calendar summary dialog, improve recurring event workflow.

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement)

Description

[Lasana Murray]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:77.0) Gecko/20100101 Firefox/77.0

Comment 1

[Lasana Murray]

See this comment here.

Comment 2

[Magnus Melin [:mkmelin]]

Note: you can auto link bugzilla comments just by writing "bug" and "comment", like bug 1575195 comment 30.
But please do copy over a description: when someone is following by email all (or at least most) information should be there.

Comment 3

[Lasana Murray]

Enhancement proposed by Paul:

For UX, for repeating events, offer not just "edit" but "edit this occurrence" and "edit all occurrences" buttons in this dialog, and drop the extra step of clicking through the separate dialog.

Comment 4

[Lasana Murray]

Attachment: bug1647855part1v1.patch

Requires this patch applied first: https://bug1575195.bmoattachments.org/attachment.cgi?id=9160129

It's a bit tricky to change the text of the buttons via Fluent then attach event listeners so I left the label as "Edit all occurrences" and "Edit" respectively if the event is recurring.

Comment 5

[Lasana Murray]

Attachment: bug1647855part1v2.patch

Added access keys. Hide the "Edit all occurrences" button when not relevant.

Comment 6

[Paul Morris [:pmorris]]

Comment on attachment 9164071 [details] [diff] [review]
bug1647855part1v2.patch

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

Looking good.  I think for repeating events the buttons should be labelled as they are in the calendar-occurrence-prompt dialog ("Edit only this occurrence" and "Edit all occurrences").  That will require using fluent in a more dynamic way.  See for example https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-ics-file-dialog.js#127  

Also following the occurrence prompt dialog, I think we should also add the text "This is a repeating event." above the buttons when it is a repeating event.  It might be worth asking Alex what he thinks on that though.

That calendar-occurrence-prompt dialog is used for deleting and cutting/copying items, so we still need it.  I was thinking that as part of this we should modify that dialog to remove the parts related to editing items, now that editing is handled through this other path, otherwise it could get confusing.  But that said, I see a number of places where `modifyEventWithDialog` is called with the 3rd argument `true` (which will open that occurrence prompt dialog).  So maybe we leave it as it is for now and in a follow-up see if we can streamline things so that all item editing paths go through the event summary dialog and not the occurrence prompt dialog.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +224,5 @@
>  }
>  
>  /**
> + * This configures the dialog buttons depending on the writable status
> + * of the item and recurrence settings:

Instead of "recurrence settings" I think it would be clearer to say something like "whether the item is a recurring item or not".

@@ +239,3 @@
>    let acceptButton = dialog.getButton("accept");
> +  let extraButton = dialog.getButton("extra1");
> +  let isRecurring = item.recurrenceInfo != null || item.recurrenceId != null;

While looking at the calendar-occurrence-dialog code, I noticed a different way to detect whether an item is recurring.  
https://searchfox.org/comm-central/source/calendar/base/content/calendar-item-editing.js#605-606   

Apparently if the item's `parentItem` property does not point to the item itself, then it is an occurrence of a repeating item.  So it looks to me like we should try using that instead.

Comment 7

[Paul Morris [:pmorris]]

Eventually we would like to offer an "Edit all future occurrences" option. (There is already scaffolding for this in the occurrence dialog code.) So that's another reason to use "Edit only this occurrence" instead of just "Edit" for the button. When there are three buttons instead of two it will help make it clear what each one does.

Comment 8

[Lasana Murray]

Also following the occurrence prompt dialog, I think we should also add the text "This is a repeating event." above the buttons when it is a repeating event. It might be worth asking Alex what he thinks on that though.

Is that necessary? The event summary dialog already has a "Repeat" field for events that do.

Comment 9

[Lasana Murray]

Attachment: bug1647855part1v3.patch

Using the formatValue API to retrieve the translation. Comment updated.

Comment 10

[Lasana Murray]

Attachment: preview-image.png

Here is a screenshot for convenience.

Comment 11

[Paul Morris [:pmorris]]

Thanks for the screenshot, that's really helpful!

(In reply to Lasana Murray from comment #8)

Also following the occurrence prompt dialog, I think we should also add the text "This is a repeating event." above the buttons when it is a repeating event. It might be worth asking Alex what he thinks on that though.

Is that necessary? The event summary dialog already has a "Repeat" field for events that do.

Hm, yeah, seeing the screenshot I think it's okay to do it as you have done it there. I'm needinfo-ing Alex for his thoughts on this.

I think it would be good to add some white space (like the equivalent of one line of text) above the buttons to separate them a bit from the rest. (I'll try to get to the review soon.)

Comment 12

[Magnus Melin [:mkmelin]]

I think it's slightly problematic to be adding many buttons like this since it doesn't scale. Like: there should really be a delete button too and that would have similar functionality. (Should also have at least for delete an "and all future occurrences", once we implement that.) Check the Fastmail calendar UI.

Comment 13

[Paul Morris [:pmorris]]

Attachment: fastmail-calendar-repeat-buttons.png

Here's the Fastmail calendar UI. Clicking on the "Delete" button shows the same menu of options for deleting the event(s).

Comment 14

[Alessandro Castellani [:aleca]]

Thanks for the ping on this.

Regarding the UI
The dialog as it is looks a bit rough, but I don't think it's a major problem as we're focusing more on the usability rather than the interface.
I'd be more than happy to do a UI refresh in a follow up bug once this lands.
For now, focusing on readability and usability is the key.

Regarding buttons
The Fastmail UI is a decent starting point we could look at in order to get inspirations (shamefully admitting that my mock-up is inspired by that: https://bug1575195.bmoattachments.org/attachment.cgi?id=9139613).
We already have the paradigm of a bollbarbutton opening a menulist for multiple actions (Account Settings > Account Actions button at the bottom of the left sidebar).

I think the optimal solution would be to try to be smart and adapt those buttons base on the situation.
So, if the event is only one and doesn't repeat:

--------------------------
[Delete]            [Edit]
--------------------------

If the event has multiple instances:

---------------------------------
[Delete  ▼]             [Edit  ▼]
---------------------------------

With the buttons opening menulists for multiple actions.

Also, little note, let's try to always follow the semantic convention of having the Primary Action to the Right and Secondary Actions to the Left.
I guess the primary is to Edit and not Delete, so Edit should be on the Right.

Comment 15

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #14)

Also, little note, let's try to always follow the semantic convention of having the Primary Action to the Right and Secondary Actions to the Left.
I guess the primary is to Edit and not Delete, so Edit should be on the Right.

On Windows this is normally inverted.

Comment 16

[Alessandro Castellani [:aleca]]

On Windows this is normally inverted.

Yuk!
Even in our Preference/Account settings subdialogs?

Comment 17

[Richard Marti (:Paenglab)]

Attachment: dialog-windows.png

(In reply to Alessandro Castellani (:aleca) from comment #16)

On Windows this is normally inverted.

Yuk!
Even in our Preference/Account settings subdialogs?

Yes, toolkit dialogs do this automatically. You only define the accept and cancel buttons and toolkit places them according to the OS.

Comment 18

[Paul Morris [:pmorris]]

Alex, thanks for the input. Lasana, let's go with what Alex describes in comment 14. To keep things more incremental, let's proceed as follows:

1. In this bug let's just focus on implementing the edit button first (with menu for repeating events, etc.).
2. Fix the reminders menu, bug 1651779.
3. Add edit context menu item, bug 1651783
4. In another follow-up bug add the delete button (with menu for repeating events, etc.)
5. Any other follow-ups.

After this bug is done we'll be ready to land what we have so far. (Lasana, sorry the design is changing out from underneath you after you've already implemented it in one way.)

Comment 19

[Lasana Murray]

We already have the paradigm of a bollbarbutton opening a menulist for multiple actions (Account Settings > Account Actions button at the bottom of the left sidebar).
...
In this bug let's just focus on implementing the edit button first (with menu for repeating events, etc.).
Is this possible with

To be clear, this means reverting to using explicit buttons in the calendar-item-summary.xhtml instead of the dialog's buttons attribute?

Comment 20

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #19)

To be clear, this means reverting to using explicit buttons in the calendar-item-summary.xhtml instead of the dialog's buttons attribute?

Hm, that's a good point. Yeah, if the dialog buttons don't support a button-with-menu (and I doubt that they do) then yeah, I think we'll need to supply our own buttons (either in the xhtml file or via JS) instead of using the built-in dialog buttons. That's unfortunate, but sounds like it will be necessary to implement this design.

Comment 21

[Paul Morris [:pmorris]]

Comment on attachment 9165161 [details] [diff] [review]
bug1647855part1v3.patch

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

FWIW, these changes look good.  Just wanted to give you some feedback, even though this is headed in a different direction now.

Comment 22

[Lasana Murray]

Attachment: add-a-dropdown-edit-menu.patch

This adds a button menu to the dialog if the event is repeating with the occurrence options. In those cases the accept button will be hidden.

Comment 23

[Lasana Murray]

Attachment: with-edit-menu.png

Here is a screenshot of how it looks on my machine. CC'ing Alessandro for opinion.

Comment 24

[Paul Morris [:pmorris]]

Comment on attachment 9167362 [details] [diff] [review]
add-a-dropdown-edit-menu.patch

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

Looking good, a few comments to address.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +238,2 @@
>    let acceptButton = dialog.getButton("accept");
> +  let editMenuButton = document.getElementById("calendar-summary-dialog-edit-button");

This editMenuButton is only used in one branch of the conditional below, so just move it there.  No need for it to be defined in the wider scope.

@@ +246,5 @@
> +    // Add the menu to the edit button and show the custom footer.
> +    let menu = document.getElementById("edit-button-context-menu");
> +    let hbox = document.getElementById("custom-dialog-buttons");
> +    editMenuButton.appendChild(menu);
> +    hbox.hidden = false;

Since you are only using this special edit button for recurring events, why not just make the menu (<menupopup>) a child of the button in the xhtml file?  Then all you have to do here is show the custom footer.  Unless I'm missing something.

That said, another possibility would be to use the custom footer for both single items and recurring items, and just never show the default dialog buttons.  That would make it easier to keep the styling/spacing consistent in both cases, especially when we add a "delete" button in the future.  With this approach you could define both kinds of buttons separately in the xhtml (with the menu as a child of one of them) and then just show/hide them as needed.  I leave it up to you if you want to do it this way or not.

::: calendar/base/content/dialogs/calendar-summary-dialog.xhtml
@@ +143,5 @@
> +              data-l10n-id="edit-button-context-menu-this-occurrence"
> +              oncommand="event.stopPropagation();onEditMenuItemClicked('edit-this-occurrence')"/>
> +    <menuitem id="edit-button-context-menu-all-occurrences"
> +              data-l10n-id="edit-button-context-menu-all-occurrences"
> +              oncommand="onEditMenuItemClicked('edit-all-occurrences',event)"/>

`onEditMenuItemClicked` only takes one argument.  
Also, should we do `event.stopPropagation()` with both of these menuitems?

Comment 25

[Alessandro Castellani [:aleca]]

Comment on attachment 9167366 [details]
with-edit-menu.png

I think this looks good.
Will this button adapt to match if the event is single or repeated? Eg. if single we don't need the dropdown with the disabled "Edit all occurrences".

Comment 26

[Lasana Murray]

(In reply to Alessandro Castellani (:aleca) from comment #25)

Comment on attachment 9167366 [details]
with-edit-menu.png

I think this looks good.
Will this button adapt to match if the event is single or repeated? Eg. if
single we don't need the dropdown with the disabled "Edit all occurrences".

Yes. For non-repeating events a regular "Edit" button is shown.

Comment 27

[Lasana Murray]

Attachment: use-custom-buttons-for-both-instead-of-dialog.patch

Made the <menupopup> a child of the <button> I was under the impression that we could not nest data-l10n-id when the parent has one, so I was adding the menu manually. Seems to work this way though.

Made both the single edit and repeating edit custom buttons with the single edit still retaining the dlgtype="accept" attribute.

Cleaned up the stray stopPropagation() calls. They should not be needed with this route as we don't hook into the "dialogaccept" event when the event repeats.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9167746 [details] [diff] [review]
use-custom-buttons-for-both-instead-of-dialog.patch

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

::: calendar/locales/en-US/calendar/calendar-summary-dialog.ftl
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +calendar-summary-dialog-edit-button =
> +  .label = Edit
> +  .accesskey = e

accesskeys are case sensitive, but fall back to working insensitively. It's preferable to use the correct case

Comment 29

[Lasana Murray]

Attachment: update-tests.patch

Updated the failing tests affected by this change. Introduced an invokeEditingRepeatEventDialog function and did a little clean up from prior patches.

Comment 30

[Lasana Murray]

Try run, errors seem unrelated.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8264e397dca8c823aedf6e1e6d9a6503aeb60967

Comment 31

[Paul Morris [:pmorris]]

Comment on attachment 9167746 [details] [diff] [review]
use-custom-buttons-for-both-instead-of-dialog.patch

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

Looking good.  Just a few comments to address.  This is not applying cleanly for me at this point (bitrot), so I haven't been able to try it out yet.  I'll give the next version a try.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +239,2 @@
>    if (window.readOnly === true) {
> +    editButton.focus();

At first glance it seems odd to be focusing the edit button for a read-only item.  Could you add a comment here to say that the edit button works like an "accept" button, to allow quickly closing the dialog by hitting enter key, in this case?

@@ +295,5 @@
> +      break;
> +    default:
> +      break;
> +  }
> +}

What about replacing this with two functions `onEditThisOccurrence()` and `onEditAllOccurrences()`?  That way you avoid passing the string arguments which can be brittle and error-prone.  If a function is called with a misspelled name then we get an undefined function error, whereas if a string argument has a typo we have a silent failure.

::: calendar/base/content/dialogs/calendar-summary-dialog.xhtml
@@ +135,5 @@
>                oncommand="locationCopyLink(document.popupNode)"/>
>    </menupopup>
> +
> +  <hbox id="calendar-summary-dialog-custom-button-footer" hidden="true">
> +    <spacer class="button-spacer" flex="1"/>

Can we just use the new CSS file to achieve this spacing, or is the <spacer> still needed?  Would be nice to not have to have a <spacer> if we can avoid it.

Comment 32

[Lasana Murray]

This is not applying cleanly for me at this point (bitrot)

You may need to apply the patches from bug 1575195 first.

What about replacing this with two functions onEditThisOccurrence() and onEditAllOccurrences()...

I implemented it like this in anticipation of more menu times in the future and possibly handling the delete action here as well. I can add a throw clause at the end for invalid strings if that works.

Can we just use the new CSS file to achieve this spacing, or is the <spacer> still needed?

I can try. I was just following the markup of the dialog footer to keep things simple for now. I'll put it in the css file I created.

Comment 33

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #32)

You may need to apply the patches from bug 1575195 first.

Yep, I did that when I tried to apply this patch.

I implemented it like this in anticipation of more menu times in the future and possibly handling the delete action here as well. I can add a throw clause at the end for invalid strings if that works.

Okay, a throw clause would be good, and that works for me if you prefer that approach. (On the other hand, we could always add more functions in the future too. And if the concern is about having too many of them at the top level we could put them in a single top level object like buttonHandler.onEditThisOccurrence(). But I leave it up to you.)

I can try. I was just following the markup of the dialog footer to keep things simple for now. I'll put it in the css file I created.

Ah, okay, good idea. It's good to keep consistency with the standard dialog footer.

Comment 34

[Paul Morris [:pmorris]]

Comment on attachment 9167989 [details] [diff] [review]
update-tests.patch

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

This is looking good, after a brief read-through.  A couple minor comments.  I'll do a full review along with the other patch for this bug when it's ready for another look and testing.

::: calendar/test/modules/CalendarUtils.jsm
@@ +310,5 @@
>  }
>  
>  /**
> + * This callback should close the dialog when finished.
> + * @callback eventDialogCallback

I'd usually capitalize a type like this, so: EventDialogCallback

@@ +313,5 @@
> + * This callback should close the dialog when finished.
> + * @callback eventDialogCallback
> + * @param {MozMillController} eventController - The controller for the event
> + *                                              window.
> + * @param {object} mockIframeController       - Used with helpersForControllers.

object -> Object

@@ +336,5 @@
>  }
>  
>  /**
> + * This callback should close the dialog when finished.
> + * @callback eventSummaryDialogCallback

EventSummaryDialogCallback

Comment 35

[Lasana Murray]

Attachment: part1-provide-edit-button-dropdown.patch

Added comment on editButton.focus(). Using individual functions instead of a switch statement for menu items.

Comment 36

[Lasana Murray]

Attachment: part2-update-existing-tests.patch

Comments updated.

Comment 37

[Lasana Murray]

The patches from 1575195 need to be applied in the order mentioned on bug 1575195 comment 63 otherwise hg import will refuse to apply this one.

Comment 38

[Paul Morris [:pmorris]]

Comment on attachment 9169241 [details] [diff] [review]
part1-provide-edit-button-dropdown.patch

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

The changes look good.  When I tested it, if I open a repeating event and choose "edit only this occurrence" from the edit menu, the "Edit Repeating Event" dialog opens, asking me again whether I want to edit just this occurrence or all occurrences.  So that's not working yet.  When I choose "edit all occurrences" in the menu it opens the event dialog as expected, allowing me to edit all occurrences, so that option is working as expected.

Comment 39

[Paul Morris [:pmorris]]

Comment on attachment 9169247 [details] [diff] [review]
part2-update-existing-tests.patch

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

When I tried running these two tests I ran into issues.  For the first one I got:
ERROR TEST-UNEXPECTED-FAIL | comm/calendar/test/browser/recurrence/browser_daily.js | application terminated with exit code 11
ERROR TEST-UNEXPECTED-FAIL | comm/calendar/test/browser/recurrence/browser_daily.js | application terminated with exit code 11

The second one stalled out showing the "Edit Repeating Event" dialog.

Comment 40

[Lasana Murray]

Attachment: part1-provide-edit-button-dropdownv2.patch

I left out a part from the previous patch by mistake while trying to figure out why it was not applying cleanly.

This should take care of the occurrence prompt. Sorry!

Comment 41

[Lasana Murray]

Here is another try run. Current failures seem unrelated.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=33c906c8a06bb4c4a9647109f47be13983ce22ce

Comment 42

[Paul Morris [:pmorris]]

Comment on attachment 9169247 [details] [diff] [review]
part2-update-existing-tests.patch

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

Looks good and tests pass.  Just one small thing, looks like a glitch left over from rebasing.

::: calendar/test/modules/CalendarUtils.jsm
@@ +400,5 @@
> + *                                           the event dialog is open.
> + * @param {boolean} [editAll=false]        - If true, will edit all
> + *                                           occurrences of the event.
> + */
> +async function invokeEditingRepeatEventDialog(mWController, clickBox, body, editAll = false) {

body -> callback

Comment 43

[Paul Morris [:pmorris]]

Comment on attachment 9169483 [details] [diff] [review]
part1-provide-edit-button-dropdownv2.patch

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

I thought it must be something like that.  It's working now, so ship it!

Comment 44

[Lasana Murray]

Attachment: part3-rename-parameter.patch

Just fixing the typo. Sorry!

Comment 45

[Magnus Melin [:mkmelin]]

The patch doesn't apply (starting with part1)

Comment 46

[Lasana Murray]

I'm going to do a fresh clone of comm-central and do this one over manually.

Comment 47

[Lasana Murray]

Attachment: part1-provide-edit-button-dropdownv3.patch

Sames as the previous one except for the metadata.
I tested it with a fresh clone and applies cleanly.

Comment 48

[Lasana Murray]

Attachment: part2-update-existing-testsv2.patch

Made the parameter change in this one otherwise same as before.

Comment 49

[Magnus Melin [:mkmelin]]

Part 1 doesn't have a proper commit message.
But why is it two patches? Isn't part2 just updating tests for part1? Such things should be kept in the same commit. Separate commits are only useful if they can be landed independently/incrementally and still not break stuff.

Comment 50

[Lasana Murray]

Attachment: part1-provide-edit-button-dropdownv4.patch

Commit message updated.

Comment 51

[Lasana Murray]

But why is it two patches? Isn't part2 just updating tests for part1?

It was easier on me. I would have figured out how to make the changes first then start working on the tests. I'm still getting used to testing so it would have taken some time, during which, I would have wanted some feedback on the changes.

Such things should be kept in the same commit. Separate commits are only useful if they can be landed independently/incrementally and still not break stuff.

Ok. I still want to add another test for this though, without using the elementslib stuff.

Comment 52

[Magnus Melin [:mkmelin]]

I see. But doing it that way (in general) makes it harder to review.

I think we should now squash them into one patch, since they are not independent code changes.

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 9169891 [details] [diff] [review]
part1-provide-edit-button-dropdownv4.patch

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

Will fold these and land them

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/acb58eb03c60
Provide a button menu for editing event occurrences. r=pmorris

Comment 55

[Magnus Melin [:mkmelin]]

Was there anything left to do in this bug, or should it be closed and other work moved elsewhere?

Comment 56

[Lasana Murray]

I want to add a test for this. Will get it done soon.

Comment 57

[Lasana Murray]

Attachment: tests.patch

Adds a test for this change. Apologies on the delay, initially I was using this to get more familiar with the non-mozmill apis.

Comment 58

[Lasana Murray]

I figure this and the other calendar patch I submitted will need to be updated for the recent changes in bug 1659558

Comment 59

[Lasana Murray]

Attachment: testsv2.patch

Updated for recent calendar changes. Here is a try run in progress:

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

Comment 60

[Magnus Melin [:mkmelin]]

Comment on attachment 9175848 [details] [diff] [review]
testsv2.patch

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

Looks good, r=mkmelin

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8506fea3b911
Add tests for using event dialog edit button and menu. r=mkmelin