1583529 - The today pane is shown in tabs like the preferences tab

Status: RESOLVED FIXED

(Calendar :: General, defect, P3)

Description

[Paul Morris [:pmorris]]

Now that the preferences UI is in a tab and not in a separate window, the calendar today pane appears in the preferences tab. It distracts from the task of editing preferences. We should not show the today pane in the preferences tab.

Alessandro do you agree in terms of UX?

Implementation note: the version of the today pane that appears is the same one that appears in a mail tab, so we would need to differentiate prefs and mail tabs.

This affects TB ESR 68 and beta.

Comment 1

[Alessandro Castellani [:aleca]]

Yes, I think that's a really good point.
It doesn't make sense showing the Today Pane in the preferences tab, extensions/themes tab, and the future account settings tab.

Comment 2

[Paul Morris [:pmorris]]

Attachment: hide-today-pane-0.patch

Before this patch if the today pane was set to appear in mail tabs, then it would also appear in the preferences tab, add-ons manager tab, and other "contentTab" tabs (like "about:xyz" tabs). With this patch it doesn't ever appear in those special tabs.

The next step along these lines would be bug 780404 ("Decouple display of today pane in chat tab from mail tab").

Comment 3

[Paul Morris [:pmorris]]

Attachment: hide-today-pane-1.patch

Another pass. Since we're monitoring tab switching, we might as well handle switching the mode based on the tab all in that one place, which also lets us simplify the code a bit.

Comment 4

[Paul Morris [:pmorris]]

Attachment: hide-today-pane-2.patch

Another little refinement.

Comment 5

[Geoff Lankow (:darktrojan)]

This seems good but I haven't looked closely yet. I'd really like to see this behaviour locked in with tests. calendar/test/browser/browser_tabs.js is probably a good place to start.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part1-add-todaypane-tests-0.patch

Adds tests for current state of things -- tests separate todaypane visibility in mail, calendar, and tasks tabs.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part2-hide-todaypane-3.patch

Adds tests to check that the todaypane no longer appears in the preferences tab.
Edit: that is, "adds tests" alongside the other changes from the previous version of this patch.

Comment 8

[Paul Morris [:pmorris]]

Attachment: wip-addons-tab-tests-0.patch

This is WIP on adding tests for the addons tab, like for the preferences tab. I haven't had a chance to look into this much yet. Putting it up just in case you (Geoff) immediately see what the problem might be. I see the addons tab appear when the tests are running, but there's this error:

 0:09.34 PASS addons tab is open - 
 0:09.35 PASS addons tab is selected - 
 0:09.35 PASS todaypane is collapsed in addons tab - 
 0:09.35 FAIL A promise chain failed to handle a rejection: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURI] - stack: loadURI/<@chrome://global/content/elements/browser-custom-element.js:1000:28
_wrapURIChangeCall@chrome://global/content/elements/browser-custom-element.js:934:11
loadURI@chrome://global/content/elements/browser-custom-element.js:999:12
_loadURL@chrome://mozapps/content/extensions/extensions.js:1865:19
show@chrome://mozapps/content/extensions/extensions.js:1819:10
async*loadViewInternal@chrome://mozapps/content/extensions/extensions.js:892:27
loadInitialView@chrome://mozapps/content/extensions/extensions.js:808:10
initialize@chrome://mozapps/content/extensions/extensions.js:199:19
EventListener.handleEvent*@chrome://mozapps/content/extensions/extensions.js:93:10
Rejection date: Mon Sep 30 2019 17:29:03 GMT-0400 (Eastern Daylight Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 265
Stack trace:
resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:265
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1381
chrome://mochikit/content/browser-test.js:Tester_execTest:1385
chrome://mochikit/content/browser-test.js:nextTest/<:1213
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805

Comment 9

[Geoff Lankow (:darktrojan)]

I suspect either the Add-On Manager is trying to access the network, which is naughty, or there's a problem trying to do electrolysis. Try openAddonsMgr("addons://list/extension"); for the first, so that it loads the list of extensions and not the recommendations panel. Try running the test with --disable-e10s for the second.

Comment 10

[WaltS48 [:walts48]]

Why can't I have the Today pane showing no matter what tab, Preferences, Add-ons or Message tab is open?

If it is distracting the user can toggle it closed or do you plan on removing removing that feature.

Will there be a preference in Preferences to allow it to be visible in all tabs? I may want to scan it while in another tab.

Comment 11

[Alessandro Castellani [:aleca]]

The purpose of the Today Pane is to offer a glimpse to the user of the upcoming events and tasks.
This is really helpful when you're writing an email, setting up an appointment, or while organizing your future tasks while keeping an eye on the next or current week.

Showing the Today Pane in the Preferences Tab, or Extensions Manager Tab doesn't make sense at all and it's visually distracting.
It's totally out of context and doesn't offer any necessary information for the decision making process of updating settings or changing the theme/add-ons.

Comment 12

[WaltS48 [:walts48]]

I don't know how many users enabled "mail.preferences.inContent;true", like I did when the preference was added to Thunderbird. Version 45? 38?

I didn't find the Today pane distracting or recall seeing a bug report filed for the issue. Maybe I was the only user that toggled the pref.

If it is distracting, use the toggle to close it. That is what is there for and you won't have any new code to maintain.

I appreciate the work the developers are doing, but it is getting harder to tell what is a new feature, or a bug when I'm testing release candidates and may have to rethink being a tester. I do have less time these days.

Comment 13

[Paul Morris [:pmorris]]

Attachment: part2-hide-todaypane-4.patch

Thanks for the tip Geoff! It was the attempt at network access. This patch includes the addons page tests.

I agree with Alessandro on the UX question. Just to add a bit to what he already expressed so well, previously the preferences were in their own window, so you could focus on just the task of editing them, then close the window. Now that they are in a tab I think it's good to preserve that focus and single task workflow, when you're accessing preferences/addons manager, etc.

Comment 14

[Paul Morris [:pmorris]]

Attachment: part2-hide-todaypane-5.patch

I forgot to export the new functions in head.js to keep eslint happy.

Comment 15

[Paul Morris [:pmorris]]

Attachment: part2-hide-todaypane-6.patch

And we'll need functions to close the prefs and addons tabs to clean up once the test is done.

A try server run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0926c1e102ea48cb424f360302d98b4aea353863

Comment 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 9097499 [details] [diff] [review]
part1-add-todaypane-tests-0.patch

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

The idea's good, needs some work though.

::: calendar/test/browser/browser.ini
@@ +14,5 @@
>  
>  [browser_calendarList.js]
>  [browser_import.js]
>  [browser_tabs.js]
> +[browser_todaypane.js]

You'll have to find a new name, I just created browser_todayPane.js (sorry!) which will cause problems on Windows.
browser_todayPane_visibility.js, perhaps?

::: calendar/test/browser/browser_todaypane.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +async function clickTodayPaneButton() {
> +  let todaypaneButton = document.getElementById("calendar-status-todaypane-button");

IMO, if there's an uppercase B in todaypaneButton, there should be an uppercase P. I'm nitpicking though. ;-)

Here and elsewhere.

@@ +32,5 @@
> +  is(todaypaneIsVisible(), true, "todaypane is visible in folder tab");
> +  await openCalendarTab();
> +  is(todaypaneIsVisible(), false, "todaypane is collapsed in calendar tab");
> +  await openTasksTab();
> +  is(todaypaneIsVisible(), false, "todaypane is collapsed in tasks tab");

You keep changing the order and that's confusing. I would do everything in the order of the tabs, ie. folder, calendar, tasks.

::: calendar/test/browser/head.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* exported openCalendarTab, setCalendarView, closeCalendarTab, openTasksTab, closeTasksTab
> +selectFolderTab */

Indent the second line. Also, if you're going to use commas, use commas everywhere. (I was not aware they weren't needed until now.)

@@ +86,5 @@
> +    tabmail.selectedTab = folderMode.tabs[0];
> +  }
> +
> +  is(folderMode.tabs.length, 1, "folder tab is open");
> +  is(tabmail.selectedTab, folderMode.tabs[0], "folder tab is selected");

This function doesn't make sense if there's more than one folder tab open. There isn't in your current test, but one day someone will come along and…

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9097721 [details] [diff] [review]
part2-hide-todaypane-6.patch

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

Seems okay but I think we should hide the status bar button if it can't be clicked on. You'll need to update this patch with the other one, so I'll re-review it then.

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +55,5 @@
> +    switch (aNewTab.mode.name) {
> +      case "calendar": {
> +        calSwitchToCalendarMode();
> +        break;
> +      }

Curly braces aren't needed in switch statements unless you're declaring variables. Also, you should put a break in the default case.

::: calendar/test/browser/head.js
@@ +7,4 @@
>  
>  /* import-globals-from ../../base/content/calendar-views-utils.js */
>  
> +/* import openOptionsDialog openAddonsMgr */

This line does nothing. You want /* globals etc. */ but it's apparently not needed here.

Comment 18

[Paul Morris [:pmorris]]

Thanks for the review. The comments make sense, but I have some thoughts on the todaypane / todayPane thing:

IMO, if there's an uppercase B in todaypaneButton, there should be an uppercase P. I'm nitpicking though. ;-)

I know. It's not nice. In general I also prefer uppercase P there, but I searched the code base and unfortunately there are 10 instances of "todayPane" and 152 of "todaypane". Also it is treated as a single word in cases like "calendar_task_filter_todaypane_command" and "#calendar-status-todaypane-button" (even though it is two words in the UI). So I thought it would be better to go towards consistency: treat it as a single word, always use lowercase p.

But looking again now, I see there's no case where it is lowercase p in a camel-cased name, so maybe our inconsistent consistency is:

• in camel-cased names it's two words: todayPane
• in non-camel-cased names it's one word: todaypane
• in the UI it's two words: "Today Pane"

So... OK, uppercase P it is.

I could see standardizing on "todayPane", "today_pane", and "today-pane" everywhere, since that removes any ambiguity, but that's another discussion/bug. And well, then the drawback there is you wouldn't be able to search for "todaypane" and get all occurrences.

Comment 19

[Paul Morris [:pmorris]]

Attachment: part1-add-today-pane-tests-1.patch

1 of 2, rebased and addressing review.

You'll have to find a new name, I just created browser_todayPane.js (sorry!)
which will cause problems on Windows.
browser_todayPane_visibility.js, perhaps?

Done.

IMO, if there's an uppercase B in todaypaneButton, there should be an
uppercase P. I'm nitpicking though. ;-)

Here and elsewhere.

"P" it is.

You keep changing the order and that's confusing. I would do everything in
the order of the tabs, ie. folder, calendar, tasks.

Good call, done.

+/* exported openCalendarTab, setCalendarView, closeCalendarTab, openTasksTab, closeTasksTab
+selectFolderTab */

Indent the second line. Also, if you're going to use commas, use commas
everywhere. (I was not aware they weren't needed until now.)

Commas added. (Sometimes I miss Scheme and its lack of commas, maybe that's coming through in my fingers.)

This function doesn't make sense if there's more than one folder tab open.
There isn't in your current test, but one day someone will come along and…

Good point, fixed. This had occurred to me, but then I forgot to come back and fix it.

Comment 20

[Paul Morris [:pmorris]]

Attachment: part2-hide-today-pane-7.patch

2 of 2, rebased and addressing review.

Seems okay but I think we should hide the status bar button if it can't be
clicked on.

Good call, done.

Curly braces aren't needed in switch statements unless you're declaring variables.

Yep, I had added them on the theory of "if you always add them you never have to worry about it". I've taken them out now.

Also, you should put a break in the default case.

Done.

+/* import openOptionsDialog openAddonsMgr */

This line does nothing. You want /* globals etc. */ but it's apparently not
needed here.

Oh, yeah, I saw this in the try run. Now fixed. I put them in because my editor apparently cares about them being there.

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9098619 [details] [diff] [review]
part2-hide-today-pane-7.patch

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

Unfortunately I'm going to make you go back over this group of patches again. Sorry! You can have your r+ this time though.

::: calendar/lightning/content/messenger-overlay-sidebar.xul
@@ +353,5 @@
>               value="&event.freebusy.legend.busy;"
>               hidden="true"/>
>      </hbox>
>      <!-- end event/task in tab statusbarpanels -->
> +    <calendar-modebox id="calendar-show-todaypane-panel"

Oh, that's a good way of doing it I hadn't considered. Nice.

::: calendar/test/browser/browser_todayPane_visibility.js
@@ +9,5 @@
>  async function clickTodayPaneButton() {
>    let todayPaneButton = document.getElementById("calendar-status-todaypane-button");
>    EventUtils.synthesizeMouseAtCenter(todayPaneButton, { clickCount: 1 });
>    await new Promise(resolve => setTimeout(resolve));
>  }

Hmm, with the button hidden, this doesn't make a lot of sense. Check for visibility before the click and comment about why.

@@ +14,5 @@
>  
>  function todayPaneIsVisible() {
>    const todayPanePanel = document.getElementById("today-pane-panel");
>    return todayPanePanel.getAttribute("collapsed") != "true";
>  }

It just occurred to me, we should be using BrowserTestUtils.is_visible instead of this function. Makes things a bit tidier too.

@@ +35,4 @@
>    }
>  
>    await selectFolderTab();
>    is(todayPaneIsVisible(), true, "today pane is visible in folder tab");

While you're at it, use `ok` to check for truthy values instead of comparing against true. So for example this line would be:

ok(BrowserTestUtils.is_visible(todayPanePanel), "today pane is visible in folder tab");

Comment 22

[Paul Morris [:pmorris]]

Attachment: part1-add-today-pane-tests-2.patch

1 of 2. Thanks for the reviews. I've made those changes.

Comment 23

[Paul Morris [:pmorris]]

Attachment: part2-hide-today-pane-8.patch

2 of 2. Made changes from the review, and added some tests to check visibility of the today pane button now that we're showing/hiding it.

Comment 24

[Paul Morris [:pmorris]]

Hold up, there's an eslint issue that slipped through.

Comment 25

[Paul Morris [:pmorris]]

Attachment: part2-hide-today-pane-9.patch

2 of 2. This fixes a tiny (and strange...) eslint issue. part1 is unaffected. Here's a try run, from before the eslint error was fixed:

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f10404956c72
Add initial tests for today pane visibility. r=darktrojan
https://hg.mozilla.org/comm-central/rev/8e2b67d5ceb3
Do not show today pane in tabs like preferences tab. r=darktrojan