1297426 - The todaypane's shown/hidden state should be separate for mail, event, and task tabs

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement, P5)

Description

[Paul Morris [:pmorris]]

Currently the shown/hidden state of the todaypane is shared between mail, event, and task tabs.  It should be separate for each of these tab types.

Comment 1

[Decathlon]

It doesn't work only with the new edit dialog in tabs.
At first glance it seems that the "mode" mail/calendar/task (see the modeBroadcaster) doesn't change when these new tabs become active or are selected. The mode seems staying in "mail" for these new tabs.

Comment 2

[Paul Morris [:pmorris]]

(In reply to Decathlon from comment #1)
> It doesn't work only with the new edit dialog in tabs.
> At first glance it seems that the "mode" mail/calendar/task (see the
> modeBroadcaster) doesn't change when these new tabs become active or are
> selected. The mode seems staying in "mail" for these new tabs.

Yes, given previous discussions with Fallen and MakeMyDay the first choice is to avoid using the modeBroadcaster system for events and tasks tabs if we can, since eventually Lightning may move away from using it.  I think we should be able to fix this by using the tabmail interface instead.

Comment 3

[Paul Morris [:pmorris]]

Attachment: today-pane-event-task-tabs-0.patch

This does the job, with tests. I wrote a "waitUntil" utility function that should live in some more generally accessible place, but I wasn't able to see where that would be for this kind of mochitest utility.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9101592 [details] [diff] [review]
today-pane-event-task-tabs-0.patch

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

There's a bunch of stuff here to work on.

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +66,5 @@
> +        calSwitchToMode("calendarEvent");
> +        break;
> +      case "calendarTask":
> +        calSwitchToMode("calendarTask");
> +        break;

You could collapse these three cases and do calSwitchToMode(aNewTab.mode.name), up to you.

::: calendar/test/browser/browser_todayPane_visibility.js
@@ +62,4 @@
>    await openPreferencesTab();
>    is(BrowserTestUtils.is_visible(todayPane), false, "today pane is collapsed in preferences tab");
>    await openAddonsTab();
>    is(BrowserTestUtils.is_visible(todayPane), false, "today pane is collapsed in addons tab");

This is getting long-winded and repetitive. Consider refactoring it to a function which takes a list of tab modes that should have a visible today pane.

Also your `is(…, false)` tests could be `ok(!…)` tests for consistency.

::: calendar/test/browser/head.js
@@ +9,1 @@
>   */

Can we move this */ to the end of the previous line?

@@ +20,5 @@
> + * @param {Function} test - Test function that returns true or false.
> + * @param {number} timeLimit - How long to retry the test until giving up.
> + * @param {Promise} - A promise that is resolved when we are done.
> + */
> +function waitUntil(test, timeLimit = 8192) {

This is unnecessary as I've mentioned elsewhere.

@@ +151,5 @@
>  
>    await new Promise(resolve => setTimeout(resolve));
>  }
>  
> +async function openCalendarEventTab() {

Thinking ahead, this (and the corresponding task bits) should allow for multiple tabs being open. The other modes generally only have one tab per window but these can have any number of tabs per window. You'll need to return some identifying thing and use it as an argument to close the tab, and a function to select an already open tab.

@@ +177,5 @@
> +
> +  if (calendarEventMode.tabs.length == 1) {
> +    tabmail.closeTab(calendarEventMode.tabs[0]);
> +    // Tab does not immediately close, so wait for it.
> +    await waitUntil(() => calendarEventMode.tabs.length == 0);

Wait for the TabClose event on calendarEventMode.tabs[0].tabNode.

@@ +285,5 @@
>    await closePreferencesTab();
>    await closeAddonsTab();
> +  await closeCalendarEventTab();
> +  await closeCalendarTaskTab();
> +  delete window.calItemSaveControls;

Did the test runner complain about this? It should be cleaned up somewhere in the real code, not the test code. Not your fault (unless it is, I didn't check) but let's make things better when we notice them.

Comment 5

[Paul Morris [:pmorris]]

Attachment: today-pane-event-task-tabs-1.patch

Thanks for the review. This addresses the things you raised.

Did the test runner complain about this?

Yep, fixed in the real code now.

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

Comment 6

[Paul Morris [:pmorris]]

The failures on the try run are due to the event and task tabs not closing at the end of the test. When running this single test locally they close just fine. When running more than just the single test locally a dialog appears asking if you want to save the event (and task) before closing it. So that must be what's happening on the try run.

It's strange because usually if you don't modify the event or task (and we don't in the test) then the tab just closes and you don't get that save dialog. (I even tried using the same tabmail.closeTab() method that we use in the test in the console, and the tab closed with no dialog.)

Probably related to the mysterious save prompt appearances mentioned in bug 1590682. I tried adding the savePromptObserver code from that bug to comm/calendar/test/browser/head.js so it would work for this test. Then when I ran the whole set of calendar tests, things froze just before the task and event tabs were closed, but no save prompt appeared. Then when I killed the test in the terminal the log showed:

FAIL Unexpected save prompt appeared - JS frame :: chrome://mochitests/content/browser/comm/calendar/test/browser/head.js

So looks like this test won't pass reliably until we figure out these mysterious save prompt appearances.

Comment 7

[Geoff Lankow (:darktrojan)]

Fortunately, I have figured out the save prompt. I'll make a bug and post a patch now.

Comment 8

[Geoff Lankow (:darktrojan)]

Okay, I haven't figured out the save prompt in this test, it's not for the same reason as the other ones. I'll have look into this one.

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9104064 [details] [diff] [review]
today-pane-event-task-tabs-1.patch

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

Okay, I'm happy with this. It will need rebasing.

::: calendar/test/browser/head.js
@@ +4,5 @@
>  
> +/* exported closeAddonsTab, closeCalendarTab, closeChatTab, closePreferencesTab, closeTasksTab,
> +   openAddonsTab, openCalendarTab, openChatTab, openNewCalendarEventTab, openNewCalendarTaskTab,
> +   openPreferencesTab, openTasksTab, selectCalendarEventTab, selectCalendarTaskTab,
> +   selectFolderTab, setCalendarView */

I think I'm going to change the linter config so that we do not require these comments in head.js files. M-C doesn't require them. Do you agree?

@@ +131,5 @@
> + *
> + * @param {string} tabMode - Mode of the new tab, either `calendarEvent` or `calendarTask`.
> + * @return {string} - The id of the new tab's panel element.
> + */
> +async function openNewCalendarItemTab(tabMode) {

Call this _openNewCalendarItemTab, since we don't expect anyone to use it directly. Same with the others below.

@@ +166,5 @@
> +  let tabmail = document.getElementById("tabmail");
> +  let itemTabs = tabmail.tabModes[tabMode].tabs;
> +  let tabToSelect = itemTabs.find(tab => tab.panel.id == panelId);
> +
> +  if (tabToSelect) {

If tabToSelect doesn't exist, the test should fail.

@@ +280,5 @@
> +    await closeCalendarEventTab(id);
> +  }
> +  for (let id of taskTabPanelIds) {
> +    await closeCalendarTaskTab(id);
> +  }

At some point we should set calendar.item.editInTab back to the default value. I think this is the best place.

Comment 10

[Paul Morris [:pmorris]]

Attachment: today-pane-event-task-tabs-2.patch

I think I'm going to change the linter config so that we do not require these comments in head.js files. M-C doesn't require them. Do you agree?

Sounds fine to me. Thanks for the review. I've addressed your comments, and with the patch from bug 1593879 applied, when I ran all the calendar mochitests locally they all passed. \o/

Here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=00a8acbe86fcc8b537e6d63334d94d0d53aa4a24

If that goes well, this is ready to land after bug 1593879 does.

Comment 11

[Jorg K (CEST = GMT+2)]

Assuming a positive try.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/715b0e356aa0
Separate today pane state for calendar event and task tabs. r=darktrojan