1619732 - The iMIP bar should work well when users are not using Calendar

Status: RESOLVED FIXED

(Calendar :: E-mail based Scheduling (iTIP/iMIP), enhancement)

Description

[Paul Morris [:pmorris]]

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

Following up on bug 1617786, in light of further conversations with Alessandro, regarding the IMIP bar that appears above an email message ("this message contains an invitation..."), :

• IMIP bar should be shown by default even when calendars are disabled, for discoverability of calendar features. In this case the message should be something like "All calendars are disabled, enable one to handle invitations." Provide a button to open the calendar tab.

• Users should have a way to opt out of seeing the IMIP bar, both in general but specifically for the case where they have all calendars disabled. For example the "More" menu button should offer an option to "Do not show me this kind of message again".

• If all calendars are disabled and then the user enables a calendar, then the IMIP bar should appear again until/unless the user opts out of seeing it as described above.

Comment 1

[Paul Morris [:pmorris]]

Attachment: part1-imip-bar-calendar-deactivated-0.patch

• Adds an imip bar state for when all calendars are disabled (calendar component deactivated).
• Adds an 'open calendar' button for that imip bar state and the imip bar state where all the enabled calendars are read-only.
• Adds a "don't show this" option to the imip bar "More" menu
• Adds the more menu to those two imip bar states.
• When calendar component is activated after it has been deactivated, the imip bar is shown again.
• Everything should work correctly when multiple windows are open (since the main window and message windows can have imip bars in them).

Comment 2

[Paul Morris [:pmorris]]

Attachment: wip-part2-remove-imip-bar-observers-0.patch

This is WIP. I thought it would be worth it to remove and add observers rather than flipping and checking a pref. But it turns out you need the pref anyway to know what to do when new windows are opened, so you have to update both. So now I'm not convinced this is worth it. Curious what you think, Geoff.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9130605 [details] [diff] [review]
part1-imip-bar-calendar-deactivated-0.patch

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

Overall it looks good, with some fixes to be added.

* We should limit the size of the imip icon with a width of 20px in the `#imip-bar > image` attribute.
* The hbox imip-bar should have an align="center" attribute to vertically align the content.
* The description inside the hbox imip-bar should have a margin-bottom set to 0.

I just noticed that this bar appears above the message header, is this correct?
It feels a bit weird, and I think these notifications should appear underneath the message header, above the message body, to keep the context consistent.

::: calendar/base/themes/common/lightning.css
@@ +166,5 @@
>      list-style-image: url(chrome://calendar/skin/shared/icons/priority.svg);
>  }
>  
> +.imipGoToCalendarButton {
> +    list-style-image: url(chrome://calendar/skin/shared/icons/imip-bar.svg);

This button should use the calendar icon and not the imip icon.

Comment 4

[Paul Morris [:pmorris]]

Attachment: part1-imip-bar-calendar-deactivated-1.patch

Same as previous version except for updated imip-bar.svg to icon32.svg per Alessandro's review in comment 3 and also a few improved variable names for window objects in imip-bar.js.

Comment 5

[Paul Morris [:pmorris]]

Attachment: wip-part2-remove-imip-bar-observers-1.patch

Just rebased on new version of part1.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part3-improve-imip-bar-position-appearance-0.patch

Moves the iMIP bar below the message headers and makes other style changes from Alessandro's review above in comment 3.

I adjusted the bottom border color to match the similar notifications that appear in this position (e.g. remote content warning). I haven't checked that on windows, but I assume it is similar to linux and the same change will make sense on both.

Those other notifications also have an animation where they drop in (notification.animation style in m-c). I tried adding that transition style to the iMIP bar but it did not work, so skipping that for now. At some point it would be worth exploring converting the iMIP bar so it is a <notification> like the others, for better consistency. (Maybe after XUL -> HTML transition is done.)

Flagging Geoff for review since it is calendar, and Alessandro since these changes address his review.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9131159 [details] [diff] [review]
part3-improve-imip-bar-position-appearance-0.patch

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

This looks good to me, with the little border color change.
r+

::: calendar/base/themes/linux/lightning.css
@@ +8,5 @@
>    background-color: #baeeff;
>    color: #000;
> +  /* border color matches the border color of `notification[type="warning"]` in
> +     mozilla-central/toolkit/themes/shared/notification.inc.css */
> +  border-bottom: 1px solid rgba(12, 12, 13, 0.2);

Let's use the `ThreeDShadow` built-in variable instead of a custom rgba color.
Same for all the others.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part3-improve-imip-bar-position-appearance-1.patch

Thanks, and good call. Now using ThreeDShadow in those two places.

Comment 9

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Comment on attachment 9131153 [details] [diff] [review]
part1-imip-bar-calendar-deactivated-1.patch

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

I'm happy with the code changes but I have some issues with the wording. Aleca, what do you think?

::: calendar/lightning/content/imip-bar.js
@@ +70,2 @@
>        ltnImipBar.resetBar();
> +      ltnImipBar.tbHideMessageHeaderPane.apply(null, args);

This could be ltnImipBar.tbHideMessageHeaderPane(...args);

::: calendar/locales/en-US/chrome/calendar/calendar.dtd
@@ +360,5 @@
>  <!ENTITY calendarproperties.name.label                     "Name:">
>  <!ENTITY calendarproperties.readonly.label                 "Read Only">
>  <!ENTITY calendarproperties.firealarms.label               "Show Reminders">
>  <!ENTITY calendarproperties.cache3.label                   "Offline Support">
> +<!ENTITY calendarproperties.enabled.label                  "Enable this calendar">

When changing a string you need to give it a new name or translators won't know it has changed. (The cache string above it is the third version.)

"Switch this calendar on" and "Enable this calendar" largely mean the same thing, so it's not *super*-critical here, but you should still do it.

::: calendar/locales/en-US/chrome/lightning/lightning.dtd
@@ +62,5 @@
>  <!ENTITY lightning.imipbar.btnDelete.label                                  "Delete">
>  <!ENTITY lightning.imipbar.btnDelete.tooltiptext                            "Delete from calendar">
>  <!ENTITY lightning.imipbar.btnDetails.label                                 "Details…">
>  <!ENTITY lightning.imipbar.btnDetails.tooltiptext                           "Show event details">
> +<!ENTITY lightning.imipbar.btnDoNotShowImipBar.label                        "Do not show me this kind of message again">

I don't think I like this – it's too wordy – but I'm struggling to come up with something better.
"Don't ask again" could mean just this message. "Don't ask about invitations again"?
(I'm not sure if you're supposed to use "don't" instead of "do not", but we do in a lot of places. I prefer it.)

::: calendar/locales/en-US/chrome/lightning/lightning.properties
@@ +141,5 @@
>  # %1$S - datetime of deletion
>  imipBarReplyToRecentlyRemovedItem=This message contains a reply referring to an event that was removed from your calendar at %1$S.
>  imipBarUnsupportedText=This message contains an event that this version of Lightning cannot process.
>  imipBarProcessingFailed=Processing message failed. Status: %1$S.
> +imipBarCalendarDeactivated=All calendars are disabled, please enable a calendar to handle invitations.

Without the text explaining why the bar is showing in the first place this is a bit confusing. Is there any situation where something like this text would be inaccurate?
"This message contains an invitation to an event. Enable a calendar to handle invitations."

Comment 10

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Another thought: it's not at all obvious how to enable a calendar, even having been taken to the calendar tab. Probably something for another whole bug, but just pointing it out because it's relevant to what's happening here.

Comment 11

[Matt]

I would see someone trying to add an appointment or invite and not having an active calendar being lead down a wizard to add one, not presented with an error message or having he calendar tab open. The wizard is probably something for another bug, but I think anything here should be done as an interim measure until something better can be offered.

Comment 12

[Magnus Melin [:mkmelin]]

+<!ENTITY calendarproperties.enabled.label "Enable this calendar">

Should also capitalize.

Comment 13

[Paul Morris [:pmorris]]

(In reply to Geoff Lankow (:darktrojan) from comment #10)

Another thought: it's not at all obvious how to enable a calendar, even having been taken to the calendar tab. Probably something for another whole bug, but just pointing it out because it's relevant to what's happening here.

Indeed, this is on the radar. I met with Alessandro about the details today and have opened bug 1621130 for the immediate step. We discussed some other improvements that will help that will go under another meta bug about improving the calendar list UI.

Comment 14

[Paul Morris [:pmorris]]

(In reply to Matt from comment #11)

I would see someone trying to add an appointment or invite and not having an active calendar being lead down a wizard to add one, not presented with an error message or having he calendar tab open. The wizard is probably something for another bug, but I think anything here should be done as an interim measure until something better can be offered.

Yeah, there is room for something like this eventually after we put this first step in place. One option would be to show a list of calendars, like in a drop down menu or popup doorhanger dialog, that lets you select a calendar to enable, inline, without having to go to the calendar tab. Or as you say open a 'create calendar' dialog/wizard if/when we support no calendars existing.

Comment 15

[Paul Morris [:pmorris]]

Attachment: part1-imip-bar-calendar-deactivated-2.patch

Thanks for the review(s). This addresses the comments. After some discussion on matrix, the strings I've gone with here are:

• "This message contains event information. Enable a calendar to handle it."
• "Don't show me these messages"

Avoiding "invitations" because the messages could contain canceled event notification, counter proposals, etc. Avoiding "don't ask" because we're not asking a question just making a statement. We use "me" elsewhere in similar cases, so using it here for consistency.

Comment 16

[Paul Morris [:pmorris]]

Attachment: part2-remove-imip-bar-observers-2.patch

Removing the observers when they aren't needed is the more thorough way to go here, but the increase in complexity (since you need a pref anyway) might not be worth it.

Comment 17

[Paul Morris [:pmorris]]

Attachment: part3-improve-imip-bar-position-appearance-2.patch

No changes here, just rebased.

Comment 18

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Comment on attachment 9132148 [details] [diff] [review]
part1-imip-bar-calendar-deactivated-2.patch

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

::: calendar/base/modules/calCalendarDeactivator.jsm
@@ +89,5 @@
>      }
> +
> +    if (someCalsEnabled) {
> +      Services.prefs.setBoolPref("calendar.itip.showImipBar", true);
> +    }

This means that if the user disables all calendars then the messages are shown, even if the user had previously opted out. I think this is okay but I felt like I should point it out.

::: calendar/lightning/content/imip-bar.js
@@ +561,5 @@
> +    let openCal = mainWindow => {
> +      mainWindow.focus();
> +      mainWindow.document.getElementById("tabmail").openTab("calendar", {
> +        title: mainWindow.document.getElementById("calendar-tab-button").getAttribute("title"),
> +      });

Is this really the best we've got to open the calendar tab? (Apparently, yes it is.) Wow, that sucks.

::: calendar/locales/en-US/chrome/calendar/calendar.dtd
@@ +360,5 @@
>  <!ENTITY calendarproperties.name.label                     "Name:">
>  <!ENTITY calendarproperties.readonly.label                 "Read Only">
>  <!ENTITY calendarproperties.firealarms.label               "Show Reminders">
>  <!ENTITY calendarproperties.cache3.label                   "Offline Support">
> +<!ENTITY calendarproperties.enabled2.label                 "Enable this Calendar">

Lower-case C. I can see that other stuff nearby has capitals on every word, but IMO that's wrong too. This just looks weird and so does "Enable This Calendar".

::: calendar/locales/en-US/chrome/lightning/lightning.dtd
@@ +64,5 @@
>  <!ENTITY lightning.imipbar.btnDetails.label                                 "Details…">
>  <!ENTITY lightning.imipbar.btnDetails.tooltiptext                           "Show event details">
> +<!ENTITY lightning.imipbar.btnDoNotShowImipBar.label                        "Don't show me these messages">
> +<!ENTITY lightning.imipbar.btnGoToCalendar.label                            "Calendar">
> +<!ENTITY lightning.imipbar.btnGoToCalendar.tooltiptext                      "Go to the Calendar Tab">

Lower-case C and T.

Comment 19

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Comment on attachment 9132151 [details] [diff] [review]
part2-remove-imip-bar-observers-2.patch

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

I don't think this is worth doing.

Comment 20

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Comment on attachment 9132152 [details] [diff] [review]
part3-improve-imip-bar-position-appearance-2.patch

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

I'd like to see this converted to a real notification but I imagine that's a fair amount of work and this'll do for now.

Comment 21

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Ha, I see Magnus disagrees with me about the "Enable this Calendar" capitalisation. Leave it how it is then, I guess. I still think it looks weird.

Comment 22

[Paul Morris [:pmorris]]

Attachment: part1-imip-bar-calendar-deactivated-3.patch

Thanks for the reviews. This is the same as the previous version but with "Don't show me these messages" in lower case.

(In reply to Geoff Lankow (:darktrojan) from comment #18)

This means that if the user disables all calendars then the messages are
shown, even if the user had previously opted out. I think this is okay but I
felt like I should point it out.

Hm, I don't think that statement, as written, is accurate. When all calendars are disabled and a user enables a calendar, then the imip bar will start appearing again, even if the user opted out of it before. (I assume that's what you meant.) That's the expected behavior, from discussions with aleca and as described at the top of the bug.

Basically, whenever a user starts using calendars we want to be sure to show the imip bar for discoverability of that functionality, even if they may have to opt out again if they don't want to see the imip bar. It's likely if they went from not using calendar to using calendar they will also want to use the imip bar, or at least should be offered the chance to start doing so (again).

Is this really the best we've got to open the calendar tab? (Apparently, yes
it is.) Wow, that sucks.

Agreed.

Lower-case C. I can see that other stuff nearby has capitals on every word,
but IMO that's wrong too. This just looks weird and so does "Enable This
Calendar".

Yeah, I had lowercase "Enable this calendar" at first (and prefer that). I capitalized "Calendar" based on Magnus' feedback (as you saw). So I've left it at "Enable this Calendar", which I guess is at least more consistent with the capitalization used elsewhere on that view in the dialog.

Comment 23

[Paul Morris [:pmorris]]

Comment on attachment 9132151 [details] [diff] [review]
part2-remove-imip-bar-observers-2.patch

part2 patch is no longer needed.

Comment 24

[Paul Morris [:pmorris]]

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

Comment 25

[Magnus Melin [:mkmelin]]

"Enable this Calendar" is a strange mix of capitalizion, we usually only lowercase prepositions and such.

Comment 26

[Paul Morris [:pmorris]]

Attachment: part1-imip-bar-calendar-deactivated-4.patch

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

"Enable this Calendar" is a strange mix of capitalizion, we usually only lowercase prepositions and such.

Okay, now with "Enable This Calendar" then.

Comment 27

[Paul Morris [:pmorris]]

.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/44f5cf16dec6
Make the iMIP bar work when calendar component is deactivated. r=darktrojan
https://hg.mozilla.org/comm-central/rev/506be393d5ef
Improve iMIP bar position and appearance. r=aleca, r=darktrojan

Comment 29

[Magnus Melin [:mkmelin]]

Yay for moving the iMIP bar down! It really was in the wrong place where it was.