Once the new appmenu lands (via bug 1546309) Lightning will need to be adapted to add its menu items somewhere since the current code adding them to the appmenu will no longer work. See bug 1558572 about the general plan for add-ons for this.

I've looked into this some more and here are two options. (I think the second option is better.)

Option 1: Move Calendar menu items into a separate menu accessible via new calendar toolbar button.

This follows how add-ons work in Firefox, using the browserAction WebExtensions API. However, AFAICT this API only provides a clean slate of an HTML file to work with. So we would have to "roll our own" menu widget and get it to work and look well-integrated into the app.

Complicating this is the number of Calendar menu items in the appmenu (over 50). So it would require sub-menus, and you would have to implement a mechanism for that. Using the appmenu code is appealing, but seems like it would be difficult. For example, the appmenu code uses XUL and the browserAction works with HTML.

The end result would be a significant change for users. And since the plan is to eventually move Calendar into TB so it is no longer an add-on, users would have to adapt to this change and then adapt back later. Also, we would do significant work to move things out of the appmenu only to throw that away and move them back in later.

Option 2: Keep the Calendar menu items in the appmenu, via the usual XUL overlay mechanism.

This provides more continuity for users since menu items will remain in the same place as before. Also the implementation is much more straightforward. We'd just need to add "id" attributes to some <vbox> elements to make it possible to overlay them, and then make the necessary XUL changes.

While it is more future-proof for add-ons to use the browserAction API (as in option 1 above) and we want to encourage that approach, Calendar is an exceptional case since it is bundled with TB, the plan is for it to become part of TB, it is already so fully integrated into the appmenu, etc.

So I plan to move forward with option 2, especially given the benefits of continuity for users and that we need this fixed quickly for TB 68 (which weighs on the side of not hastily making such user facing changes, as in option one, if we can avoid it).

How about add the items directly in panelUI.inc.xul with the hidden attribute and only show them when Calendar is active. Similar to the Chat items.

That's an interesting thought. It would start us down the path of moving Calendar into TB. I don't know if we're ready to take that step yet or not, and defer to mkmelin and Fallen on that question.

By "calendar is active" do you mean "add-on installed" or "is the active tab"? If "active tab" then it raises UX questions. For example, should you be able to create a new calendar item (via appmenu) while the mail tab is active, or should you have to open or switch to the calendar tab before you can add a new calendar item.

Doing the overlay option (or if we can un-hide menu items if calendar is installed) preserves the current behavior and avoids having to deal with such UX questions until we're ready to really tackle them. (I'm assuming we'll want to do a revision of the appmenu before the next big TB release, so it would be easier to address such UX questions as part of that effort.)

I mean installed and enabled. When Calendar is enabled it could simply show the menuitems with a rule in lightning.css like: .calendar-menuitem { display: -moz-box; }. With a disabled calendar no CSS is executed and the menuitems are hidden.

Okay, that makes sense, thanks. Either approach seems fine to me. I'm going to needinfo Fallen and mkmelin for their input.

mkmelin and Fallen: The question is whether to use a XUL overlay for adding calendar menu items to the appmenu, or to go ahead and add them directly to the TB appmenu code but hidden unless calendar is enabled (given the plan to merge calendar into TB).

While Richard's suggestion sounds ok to me, that might be bita problematic for localization and 68, I think.

We didn't yet discuss how/when/if to merge lightning, but I do think it would make sense.

Good thought about localization. I'll go ahead with the overlay approach. It will be easy enough to convert that to the other approach later if we want to do that.

1 of 2 parts. This is the main patch. It migrates the calendar appmenu items to the new appmenu.

There are a few more involved things that remain to be fixed in follow-up bugs (which I will file). I think it makes sense to tackle these as follow-ups so we can go ahead and get the rest of the calendar appmenu items into beta releases.

• View / Calendar / Current View
  The 4 items in this submenu are not working. They also are not working in the same menu in the main menu bar. Looks like the de-xbl work on calendar views broke them both. [EDIT: on closer inspection, it's not all four items, there's more to it. See comment 10 below.]

• Events and Tasks / Priority

• Events and Tasks / Progress
  These two submenus are now custom elements rather than XBL bindings, and the custom elements will need to be adapted to work with the appmenu in addition to the traditional style menu. [EDIT: this is now bug 1561096]

Part 2 of 2. This just renames a couple of set up and tear down functions for greater clarity.

Adding a 3rd patch (part3).

On a closer look into the "View / Calendar / Current View" menu issues -- in both the appmenu and the main "View" menu, the "Tasks in View" and "Show Completed Tasks" items were not working when the current calendar view was "Month". Other calendar views were fine. ("date" argument was undefined error.) This patch is a simple fix for that. It did not seem to warrant its own bug.

In that same "Current View" menu the "Rotate View" item can cause an intermittent error which is quite bad. ("Too much recursion".) It has to do with the "adjustScrollBarSpacers" function. I'll open a separate bug for that with more details. [EDIT: this is now bug1561093]

The "Workweek days only" item in that menu works fine.

Clearing the needinfo request from Fallen, since mkmelin provided the needed info.

Minor revision of part 1. (Cleaned up some comments and added classes that were missing from <panelview> elements.)

1 of 3, rebased on current trunk to un-bitrot them.

2 of 3, rebased on current trunk to un-bitrot them.

3 of 3, rebased on current trunk to un-bitrot them.

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

::: calendar/base/content/calendar-chrome-startup.js
@@ +216,5 @@

function setUpCalendarAppMenuItems(remove) {
const addOrRemoveListener = remove ? "removeEventListener" : "addEventListener";

I think it's preferable not to parametrize this. It's not super readable.

On the other hand, I'm not sure why we bother with removing the listeners
ever either. The go away with the element anyway.

Ah, good point! I've removed the code that removes the listeners for these menu items, and getting rid of the parameterization. I also made a part4 patch that does the same with the code for tearing down the appmenu buttons.

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

::: calendar/base/content/calendar-views.js
@@ +274,1 @@

         const dateToShow = date || date.getInTimezone(this.timezone);

can you adjust this. it's of course nonsense to begin with...

I looked at where this came from and it was a misstep in refactoring that happened in the de-xbl work, causing this issue. I've now fixed it to restore the behavior from before de-xbl happened.

4 patches coming up next.

1 of 4.

2 of 4.

3 of 4.

4 of 4. This new one removes the unneeded code that was removing event listeners for the appmenu buttons.

Any review for part 4? Looks OK to me. What about beta, does it need to go there?

This does need to go to beta. I'll work on patches for that. (Now that I think about it, it's probably good to land things on both trunk and beta at the same time, right? So I should just plan on going ahead and doing patches for both in cases like this.)

On review for part 4, it addresses a review comment that mkmelin made while giving a r+ review, and it could have been done as a revision of one of the other patches, but I thought it was better to put it in its own patch, since it was cleaning up some previously existing code that wasn't directly related. Also, since it looks OK to you (jorgk), and the changes are straightforward, I think we can say it's sufficiently reviewed.

Two patches needed rebasing here in calendar-chrome-startup.js. Here's a try run with some other stuff to avoid surprises:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d9d72d906cee42325376e1459aa29fc16e6aa9da

1 of 4 for beta.

(In reply to Jorg K (GMT+2) from comment #26)

Excellent. In fact, can you please add r+ when you had r+ and just addressed
some comments. Otherwise it all looks unreviewed. If the reviewer is not
already in the commit message, I'll have to go and check who it was anyway.

Makes sense. I'll add r+ status in bugzilla, and on commit messages, when I've already got r+.

(In reply to Jorg K (GMT+2) from comment #27)

Two patches needed rebasing here in calendar-chrome-startup.js. Here's a try run with some other stuff to avoid surprises:

Thanks for rebasing and starting a try run. Will rebase before setting checkin-needed in future.

2 of 4 for beta.

3 of 4 for beta.

4 of 4 for beta.

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

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/80baeab83f55
Make Lightning work with the new appmenu. r=mkmelin
https://hg.mozilla.org/comm-central/rev/f0df11ad8ffa
Rename setUpCalendarAppMenus. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a9eca8202820
Fix "Tasks in View" and "Show Completed Tasks". r=mkmelin
https://hg.mozilla.org/comm-central/rev/fe454c1cff84
Don't manually tear down calendar appmenu buttons. r=mkmelin

TB 68 beta 4 / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/6f57ab5c95383e005078492e038e4491c0dedd1c
https://hg.mozilla.org/releases/comm-beta/rev/61567b96f53f44c791ee865846c25d8a9d95b941
https://hg.mozilla.org/releases/comm-beta/rev/5a578fabd1647400cae264e2b3ac75552662069a
https://hg.mozilla.org/releases/comm-beta/rev/bcf79fcc86557402c20e5f4fb294149f8329c61c