1277972 - [GSoC 2016] Port the current UI for editing events and tasks to a tab

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement)

Description

[Paul Morris [:pmorris]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160511223818

Steps to reproduce:

This bug is for the first part of the "Event in a Tab" project (for GSoC 2016) -- porting the current UI/dialog for editing events and tasks to a tab, as an option.  (The current behavior where this UI opens in a dialog window will remain an option as well.)

Comment 1

[Paul Morris [:pmorris]]

Attachment: wip-event-in-tab-june4.diff

Comment 2

[Paul Morris [:pmorris]]

Comment on attachment 8760005 [details] [diff] [review]
wip-event-in-tab-june4.diff

Just an initial work in progress patch to show what I have so far.  Flip args.inTab to true in calendar-item-editing.js (line 446) to open in a tab instead of a window.

Things that are now working:
- tab titles are now updated like window titles
- tabs will now close like windows close
- items can be deleted from a tab

Main known issues:
- status bar duplication (because of iframe)
- closing a tab by clicking "[x]" on the tab does not prompt to save
changes yet
- edit and delete buttons in toolbar on main calendar and tasks pages are
not working for some reason

Comment 3

[[:MakeMyDay]]

Comment on attachment 8760005 [details] [diff] [review]
wip-event-in-tab-june4.diff

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

In general a good (first WIP) patch, keep up this way.

However, the patch doesn't apply properly (there has been a change in the event dialog code meanwhile). Before uploading a patch, please make sure you have updated your local copy of comm-central to tip and the patch still applies.

Two minor things I noticed when applying the (modified) patch:

- The tab icon is not set unlike it is in window mode for events and tasks.
- Just a minor nit, but there's a reference error in the console each when openeing an event in a tab:

Timestamp: 05.06.2016 17:00:05
Warning: ReferenceError: reference to undefined property this.mSelectedInternal
Source File: chrome://global/content/bindings/menulist.xml
Line: 439

> Main known issues:
> - status bar duplication (because of iframe)

This is true for the menu bar as well for the same reason. If you haven't enabled it permanently, you can display the app menu temporarily by long pressing the ALT key.

Before you fix further issues, you should think about how to resolve this (just to avoid doing useless work in case you have to switch to a different approach).

> - closing a tab by clicking "[x]" on the tab does not prompt to save
> changes yet
> - edit and delete buttons in toolbar on main calendar and tasks pages are
> not working for some reason

This is working here on Windows (have you made sure to select an item before trying to edit/delete it?).

Please find further comments below:

::: calendar/base/content/calendar-item-editing.js
@@ +441,5 @@
>      args.mode = mode;
>      args.onOk = callback;
>      args.job = job;
>      args.initialStartDateValue = (initialDate || getDefaultStartDate());
> +    // XXX - later set this via a hidden preference in about:config or...?

Please add this in the next iteration - also to be able to check for side effects of toggling between both modes without restarting. Take a look how prefs are included elsewhere in calendar code (setting/getting and defining a default).

@@ +472,5 @@
>      }
>  
> +    if (args.inTab) {
> +        // open in a tab
> +        document.getElementById('tabmail').openTab('calendarItem', args);

You can leave it a separate url variable and only need to assign args.url with url in the tab case as you don't seem to need this information within the dialog but only the tab handler.

@@ +477,5 @@
>      } else {
> +        // open in a window
> +
> +        // reminder: event dialog should not be modal (cf bug 122671)
> +        var features;

If you touch code like this, also convert the var to let. Only on top level it must be var.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +194,5 @@
>  
> +    // are we in a window or tab?
> +    gMode = (args.inTab ? "tab" : "window");
> +    gMainWindow = (args.inTab ? parent : window.opener);
> +

Just use a boolean variable here to distinguish between tab and window mode.

Also consider to define a variable to hold the parent.document.getElementById("tabmail") reference, as you use it multiple times in the dialogs.

And it also might be a good idea to move the definition of the gloabls and wrap its initialization in a new function in calendar-dialog-utils.js, which holds shared code used by the dialogs. That way you can spare some code duplication in calendar-event-dialog.js and calendar-summary-dialog.js

@@ +349,5 @@
>      dispose();
>      onCommandSave(true);
> +    if (gMode == "tab" && !gWarning) {
> +        parent.document.getElementById("tabmail").removeCurrentTab();
> +    }

See the above comment about having a variable for parent.document.getElementById("tabmail")

Does disposing in tab mode has any impact on the storing the window properties (size and position) when opening after switching back to window mode again? I haven'r noticed anything obvious, but please cross check.

@@ +425,5 @@
>          // date then closes the dialog and answers with "Save" in
>          // the "Save Event" dialog.
> +        if (gMode == "tab" && !gWarning) {
> +            parent.document.getElementById("tabmail").removeCurrentTab();
> +        }

Same here.

@@ +1288,5 @@
> +    document.title = newTitle;
> +
> +    // if in a tab, also update the tab title.
> +    if (gMode == "tab") {
> +        let tabmail = parent.document.getElementById("tabmail");

and here

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +16,1 @@
>  /**

see the above comment about sharing code with calendar-dialog-utils.js

@@ +26,5 @@
>  
> +    // are we in a window or tab?
> +    gMode = (args.inTab ? "tab" : "window");
> +    gMainWindow = (args.inTab ? parent : window.opener);
> +

see the above comment about sharing code with calendar-dialog-utils.js

@@ +204,5 @@
> +    if (!window.readOnly) {
> +        var args = window.arguments[0];
> +        var oldItem = args.calendarEvent;
> +        var newItem = window.calendarItem;
> +        var calendar = newItem.calendar;

please also do a var to let conversion here.

@@ +212,4 @@
>      }
> +    if (gMode == "tab") {
> +        parent.document.getElementById("tabmail").removeCurrentTab();
> +    }

see the above comment about sharing code with calendar-dialog-utils.js

@@ +222,5 @@
>  function onCancel() {
>      dispose();
> +    if (gMode == "tab") {
> +        parent.document.getElementById("tabmail").removeCurrentTab();
> +    }

see the above comment about sharing code with calendar-dialog-utils.js

::: calendar/lightning/content/calendar-item-panel.xul
@@ +10,5 @@
> +    <vbox id="calendarItemPanel" flex="1" > <!-- collapsed="true" -->
> +      <iframe type="chrome" flex="1" />
> +    </vbox>
> +  </tabpanels>
> +</overlay>

Put this file to calendar/base/content/ or maybe calendar/base/content/dialogs (although that would be an imprecise categorization) instead of calendar/lightning/content

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +140,5 @@
>    }
>  };
>  
> +
> +let calendarItemTabType = {

On top level, use var instead of let.

@@ +167,5 @@
> +    showTab: function(aTab) { },
> +    closeTab: function(aTab) { },
> +    persistTab: function(aTab) { },
> +    restoreTab: function(aTabmail, aState) { },
> +    saveTabState: function(aTab) { }

The tab is currently not restored after restart (and of cause also not put in front when it was before). You still need some implementaion here.

::: calendar/lightning/jar.mn
@@ +8,5 @@
>  % override chrome://messagebody/skin/calendar-event-dialog-attendees.png chrome://calendar/skin/calendar-event-dialog-attendees.png
>  % overlay chrome://messenger/content/messenger.xul chrome://lightning/content/lightning-migration.xul
>  % overlay chrome://messenger/content/msgAccountCentral.xul chrome://lightning/content/messenger-overlay-accountCentral.xul
>  % overlay chrome://messenger/content/messenger.xul chrome://lightning/content/messenger-overlay-sidebar.xul
> +% overlay chrome://messenger/content/messenger.xul chrome://lightning/content/calendar-item-panel.xul

see above comment about the file location

@@ +38,5 @@
>      content/lightning/imip-bar.js                          (content/imip-bar.js)
>      content/lightning/imip-bar-overlay.xul                 (content/imip-bar-overlay.xul)
>      content/lightning/lightning-calendar-creation.xul      (content/lightning-calendar-creation.xul)
>      content/lightning/lightning-calendar-creation.js       (content/lightning-calendar-creation.js)
> +    content/lightning/calendar-item-panel.xul              (content/calendar-item-panel.xul)

see above comment about the file location

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8760005 [details] [diff] [review]
wip-event-in-tab-june4.diff

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

MakeMyDay already made a lot of great comments, therefore here just a few replies:

::: calendar/base/content/calendar-item-editing.js
@@ +441,5 @@
>      args.mode = mode;
>      args.onOk = callback;
>      args.job = job;
>      args.initialStartDateValue = (initialDate || getDefaultStartDate());
> +    // XXX - later set this via a hidden preference in about:config or...?

I think this should actually be user visible. There should be a default which could be set in the prefs, but using the context menu you should be able to open event in a new window or in a new tab.

That said, I think you should just aim for a hidden pref in this bug, then file a followup bug to add the user visible preferences. The smaller we keep the initial patch the easier it will be to review incrementally.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +349,5 @@
>      dispose();
>      onCommandSave(true);
> +    if (gMode == "tab" && !gWarning) {
> +        parent.document.getElementById("tabmail").removeCurrentTab();
> +    }

When you split the current event dialog into a container and an iframe with the content it would be great to move this kind of code into the container. The onAccept code is pretty specific to tab vs dialog, so each such container should have its own version for better separation.

::: calendar/lightning/content/calendar-item-panel.xul
@@ +10,5 @@
> +    <vbox id="calendarItemPanel" flex="1" > <!-- collapsed="true" -->
> +      <iframe type="chrome" flex="1" />
> +    </vbox>
> +  </tabpanels>
> +</overlay>

MakeMyDay, you mentioned putting this file into base/content, but actually the tab implementation is very Lightning-specific, if we adhere to the old split of general-calendaring-things vs things-related-to-integrating-into-thunderbird. I'd prefer keeping this in lightning/content.

Comment 5

[[:MakeMyDay]]

>(In reply to Philipp Kewisch [:Fallen] from comment #4)
> When you split the current event dialog into a container and an iframe with the content it would be
> great to move this kind of code into the container. The onAccept code is pretty specific to tab vs
> dialog, so each such container should have its own version for better separation.

Iirc, the iframe content lives in its own window, which may leads to the observes effects. Maybe it's better to work with overlaying the content instead.

> ::: calendar/lightning/content/calendar-item-panel.xul
> @@ +10,5 @@
> > +    <vbox id="calendarItemPanel" flex="1" > <!-- collapsed="true" -->
> > +      <iframe type="chrome" flex="1" />
> > +    </vbox>
> > +  </tabpanels>
> > +</overlay>
> 
> MakeMyDay, you mentioned putting this file into base/content, but actually
> the tab implementation is very Lightning-specific, if we adhere to the old
> split of general-calendaring-things vs
> things-related-to-integrating-into-thunderbird. I'd prefer keeping this in
> lightning/content.

In general, we should probably reorganize and consolidate the whole structure one day, but this would be a discussion for another day.

Until then, I would appreciate if we store things that belong together at the same place. If we split also the dialog into two separate files, we would end up having the dialog container and the content in calendar while we have the tab container in lightning. But if you prefer to have it in lightning, the file name should be adapted to lightning-item-panel.xul at least.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to MakeMyDay from comment #5)
> >(In reply to Philipp Kewisch [:Fallen] from comment #4)
> > When you split the current event dialog into a container and an iframe with the content it would be
> > great to move this kind of code into the container. The onAccept code is pretty specific to tab vs
> > dialog, so each such container should have its own version for better separation.
> 
> Iirc, the iframe content lives in its own window, which may leads to the
> observes effects. Maybe it's better to work with overlaying the content
> instead.
I'm not sure I see the problem here. Yes, with the current patch I see a double menubar and double status bar, but what I am proposing is to take the current status bar and menubar out of calendar-event-dialog.xul and put them into a separate file only for the actual dialog. This separate file would have the menubar, an iframe, the status bar. Similarly, the container for the tab would not have a menubar, but an iframe, and maybe an extra container for the status bar items that makes it look less like a status bar.

I'd like to avoid overlays because when switching to html we shouldn't make use of overlays to be future proof. Any communication between the iframe and container can be done via message passing or window arguments.



> 
> > ::: calendar/lightning/content/calendar-item-panel.xul
> > @@ +10,5 @@
> > > +    <vbox id="calendarItemPanel" flex="1" > <!-- collapsed="true" -->
> > > +      <iframe type="chrome" flex="1" />
> > > +    </vbox>
> > > +  </tabpanels>
> > > +</overlay>
> > 
> > MakeMyDay, you mentioned putting this file into base/content, but actually
> > the tab implementation is very Lightning-specific, if we adhere to the old
> > split of general-calendaring-things vs
> > things-related-to-integrating-into-thunderbird. I'd prefer keeping this in
> > lightning/content.
> 
> In general, we should probably reorganize and consolidate the whole
> structure one day, but this would be a discussion for another day.
Indeed...one day :-D We haven't even migrated everything away from calendar/resources/.


> Until then, I would appreciate if we store things that belong together at
> the same place. If we split also the dialog into two separate files, we
> would end up having the dialog container and the content in calendar while
> we have the tab container in lightning. But if you prefer to have it in
> lightning, the file name should be adapted to lightning-item-panel.xul at
> least.
I'd prefer to put it in Lightning, I think this kind of split makes sense with the current layout. lightning-item-panel.xul sounds good to me.

Comment 7

[Paul Morris [:pmorris]]

Attachment: wip-event-in-tab-june26.diff

Thanks for the feedback.  Here's another WIP patch.  Most functionality is working.  Below is a list of known issues, (and there are a few things marked with "XXX" comments in the code).  You can access the preference for opening events/tasks in tab or window near the top of the main calendar prefs page.

- enable/disable menubar items on tab show/hide/close etc.
- make sure relevant menu items work for events/tasks in a tab
- View Menu > Toolbars > include the event-in-a-tab toolbar, only when in an event tab
- on restart make tabs restored and active tab still active
- error: undefined this.mSelectedInternal
- in tab, when customizing toolbar, disable and re-enable menu items
- different dispose mechanisms for tab and window
- different onAccept code for tab and window, in outer container scope
- work on the read-only summary dialog
- split calendar-dialog-utils.js into 2 files (for rearrangeAttendees and friends)
- have separate default toolbar button sets for tab and window
- priority indicator in status bar is not shown (tried some CSS fixes, no luck yet)
- today pane state is not preserved when switching to/from item tabs
- finalize preference id, its localization, alt key, etc.
- finalize names of files
- toolbar needs "hamburger" app menu in tab
- tabs need icons for events and tasks
- categories drop-down menu CSS problem, categories are not visible (at least on GNU/Linux theme)
- extra white border/margin in window dialog (tried some CSS fixes, no luck yet)
- write/edit doc strings for functions
- move status bar items into an overlay to avoid duplication
- DTD, CSS, JS files etc. split some up for iframe/container use or some may not be needed for both
- edit and delete toolbar buttons on main tasks page are not working (they work on main calendar page)

Comment 8

[[:MakeMyDay]]

Looks like you made good progress. I haven't had a look at the code yet, but did some testdriving (on Win7) with the patch and had some findings not yet mentioned in the above list, which I want to share:

Both modes:

- font-size of the inner window (labels and controls) is too big. It seems some css is not applied correctly. Issues like this may be a regression of renaming identifiers.

Window mode:

- resizing of the calendar-event-dialog doesn't consider min height and width (calendar-summary-dialog seems to be ok)
- the toolbar customization was reset when applying the patch initially. Afterwards, customizing works properly and idenpendently for tab and window mode as expected

Tab mode:

- toolbar elements are aligned right instead of left
- no background color defined (default is transparent)
- event/task specific menu items in Events and Tasks menu are disabled
- calendar-summary-dialog tabs cannot be closed in any way. Clicking on the ok button triggers an assertion (luckily I could get rid of the zombie tab by restarting TB due to the known tab persiting issue)


While testing, I got one or more of these messages in the error console:

TypeError: arg is undefined
updatePrivacy()                 lightning-item-panel.js:273

ReferenceError: rearrangeAttendees is not defined
onresize()                      calendar-event-dialog.xul:1

ReferenceError: toggleLink is not defined
oncommand()                     lightning-item-panel-window.xul:1

When clicking on OK button for a read-only item in tab mode:

13:06:35.081 Assert failed: aElement
2: [chrome://calendar/content/calendar-ui-utils.js:30] setElementValue
3: [chrome://lightning/content/lightning-item-panel.js:275] updatePrivacy
4: [chrome://lightning/content/lightning-item-panel.js:157] updateItemTabState
5: [chrome://lightning/content/messenger-overlay-sidebar.js:198] calendarItemTabType.showTab
6: [chrome://messenger/content/tabmail.xml:1157] updateCurrentTab
7: [chrome://messenger/content/tabmail.xml:2064] onxblselect
8: [chrome://global/content/bindings/tabbox.xml:410] set_selectedIndex
9: [chrome://global/content/bindings/tabbox.xml:435] set_selectedItem
10: [chrome://global/content/bindings/tabbox.xml:480] _selectNewTab
11: [chrome://global/content/bindings/tabbox.xml:790] onxblmousedown


Regarding your list:

> - extra white border/margin in window dialog (tried some CSS fixes, no luck
> yet)

The dialog element has some default padding, which comes from chrome://global/skin/dialog.css and was overwritten in calendar-event-dialog.css, which doesn't seem to work anymore. Maybe an issue of identifier renaming. You can use the dom inspector addon to investigate such things.

> - DTD, CSS, JS files etc. split some up for iframe/container use or some may
> not be needed for both

Be careful when touching dtd or properties files. I'm not sure, but moving around string definitions will probably result in removing and re-adding the strings for the localizers and therefore cause some needless work at their end. Philipp can provide more insight here if needed.

Comment 9

[Paul Morris [:pmorris]]

Attachment: event-in-tab-June30.patch

Initial implementation for this feature.  Toggle the hidden preference calendar.item.editInTab to edit events/tasks in a tab.

This patch should be free of any regressions to editing events/tasks in a dialog window.  Some known issues affecting editing in a tab will be addressed in the follow-up bug 1283623

Comment 10

[Paul Morris [:pmorris]]

Attachment: event-in-tab-June30-take2.patch

Same as previous patch (June30) except "calendar-event-dialog.js" has been moved and renamed to "lightning-item-iframe.js" to make it clear that it goes with "lightning-item-iframe.xul"

Comment 11

[[:MakeMyDay]]

Comment on attachment 8767002 [details] [diff] [review]
event-in-tab-June30-take2.patch

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

Hi Paul,

first, sorry it took some time, it has been a quite busy week here. I hope this doesn't slow you down.

Now to your patch:

I've given it a try and in general it looks good, but unfortunately, it's not working reliably. It seems to work with a local calendar, but when testing with a remote caldav calendar, the tab cannot be closed and you get error console messages like this when trying (regardless whether just closing the tab or using save & close):

TypeError: cyclic object value                                tabmail.xml:723:26
    closeTab                       chrome://messenger/content/tabmail.xml:723:26
    removeTabByNode                chrome://messenger/content/tabmail.xml:774:13
    removeCurrentTab              chrome://messenger/content/tabmail.xml:1120:11
    closeWindowOrTab   chrome://lightning/content/lightning-item-panel.js:216:13
    receiveMessage      chrome://lightning/content/lightning-item-panel.js:95:13

If you get so far and shut down TB, all open tabs but the default tab are removed on restart. However, window mode seems to work also for that scenario. 

Apart from this, a try run [1] with this patch causes one of our mozmill tests to fail. That failure doesn't appear on c-c, so it's related to your patch (there is another push in that try run, but that is unreleated). This needs to be fixed prior landing. Take a look at the test logs to dig into it (to get there, click on one of the orange Z and than click on the log icon in the toolbar of the blue lower section). You have already level one access iirc, so you can push try runs on your own (running mozmill locally is a pita, at least to me) to see whether the test gets green again once you fixed the issue. There's some documentation for mozmill tests on mdn. If you need further help with that, ask in #calendar or #maildev.

Therefor r- for now to get a working patch and the test failure fixed. Please also consider the comments below for the new patch (not a full review, though).

[1] https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=91c6549e7f5c3c75d34685524900608856f8efeb

::: calendar/base/content/calendar-item-editing.js
@@ +477,5 @@
>  
> +    if (args.inTab) {
> +        // open in a tab
> +        args.url = url;
> +        document.getElementById('tabmail').openTab('calendarItem', args);

can you make this

let tabmail = document.getElementById('tabmail');
tabmail.openTab('calendarItem', args);

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +16,5 @@
>      var item = args.calendarEvent;
>      item = item.clone(); // use an own copy of the passed item
>      var calendar = item.calendar;
>      window.calendarItem = item;
> +    intializeTabOrWindowVariables();

We agreed to leave the calendar-summary-dialog as is for the moment. so please exclude changes to calendar-summary-dialog.js from the patch.

::: calendar/base/content/preferences/general.xul
@@ +64,5 @@
>                          name="calendar.agendaListbox.soondays"
>                          type="int"/>
> +            <preference id="calendar.item.editInTab"
> +                        name="calendar.item.editInTab"
> +                        type="bool"/>

Why is this still needed? We agreed to have a only hidden pref for this patch, so please remove it for now.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +29,5 @@
>  
>  #calendar-event-dialog .todo-only,
> +#calendar-task-dialog  .event-only,
> +#calendar-event-dialog-inner .todo-only,
> +#calendar-task-dialog-inner  .event-only {

Why are #calendar-event-dialog and #calendar-task-dialog still needed - is this for the dynamic id updates?

::: calendar/lightning/content/lightning-item-iframe.js
@@ +143,5 @@
>      }
>  }
>  
> +function sendMessage(message) {
> +    parent.postMessage(message, "*");

Please make this aMessage instead of message. also, if you introduce a new function, please prepend a documentation block describing the purpose, input paramater(s) (with type, name and purpose) and return value (if any; also with type).

@@ +148,3 @@
>  }
>  
> +function receiveMessage(event) {

Here again: aEvent and documentation block.

@@ +211,5 @@
>  function onLoad() {
>      // Set a variable to allow/prevent actions when the dialog is loading.
>      onLoad.isLoading = true;
>  
> +    window.addEventListener("message", receiveMessage, false);

give this a more meaningful name than message.

@@ +1641,5 @@
>  /**
>   * This function rotates the Privacy of an item to the next value
>   * following the sequence  -> PUBLIC -> CONFIDENTIAL -> PRIVATE ->.
>   */
> +function rotatePrivacy(privacy) {

If you add an argument to a function, please use the aPrivacy style and extend the documentation block above. This applies on some more occurences below.

@@ +1655,2 @@
>   */
> +function updatePrivacy(value) {

why do you need an argument here? The privacy status is stored in a global, so it's available here. This applies also on each of the updateXXX functions below.

@@ +2814,5 @@
>      window.onAcceptCallback(item, calendar, originalItem, !aIsClosing && listener);
>  
>  }
>  
> +function cancelItemForDelete() {

Documentation block missing.

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +31,5 @@
> +     document.loadOverlay() will not work on this one. -->
> +<window id="calendar-event-dialog-inner"
> +      onload="onLoad();"
> +      onunload="onEventDialogUnload();"
> +      onresize="rearrangeAttendees();"

This triggeres reference errors in window mode (in strict mode only), which should be avoided. To see them make sure you have enabled javascript.options.strict in the advanced preferences.

::: calendar/lightning/content/lightning-item-panel.js
@@ +74,5 @@
> +    status: "NONE",
> +    showTimeAs: null
> +}
> +
> +function receiveMessage(event) {

Missing documentation block and argument style once again.

@@ +118,5 @@
> +            break;
> +    }
> +}
> +
> +window.addEventListener("message", receiveMessage, false);

Please use something more meaningful than message. This applies also for other functions in this file.

::: calendar/lightning/content/lightning-item-panel.xul
@@ +107,5 @@
> +          <command id="cmd_customize"
> +                   oncommand="onCommandCustomize()"/>
> +          <command id="cmd_toggle_link"
> +                   persist="checked"
> +                   oncommand="toggleLink()"/>

toggleLink doesn't work atm. Trying to use the menu item throws also reference errors. Maybe the same problem as for rearrangeAttendees.

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +139,5 @@
>      ltnSwitch2Mail();
>    }
>  };
>  
> +

One line too much.

@@ +268,5 @@
> +            }
> +        });
> +    }
> +};
> +

Again, only one line for separation, if any.

@@ +759,5 @@
> +/**
> + * Move the event toolbox containing the toolbar, into view for a tab
> + * or back to its hiding place where it is accessed again for other tabs.
> + */
> +function moveEventToolbox(destination) {

Please add the argument documentation and change the name to aDestination.

Comment 12

[Paul Morris [:pmorris]]

(In reply to [:MakeMyDay] from comment #11)

Thanks for the review.  I've addressed the issues you raised in the next patch.

> It seems to work with a local calendar, but when
> testing with a remote caldav calendar, the tab cannot be closed and you get
> error console messages 

Now fixed.

> Apart from this, a try run [1] with this patch causes one of our mozmill
> tests to fail. 

Now fixed.

> ::: calendar/base/content/calendar-item-editing.js
> @@ +477,5 @@
> >  
> > +    if (args.inTab) {
> > +        // open in a tab
> > +        args.url = url;
> > +        document.getElementById('tabmail').openTab('calendarItem', args);
> 
> can you make this
> 
> let tabmail = document.getElementById('tabmail');
> tabmail.openTab('calendarItem', args);

Done.

> ::: calendar/base/content/dialogs/calendar-summary-dialog.js
> @@ +16,5 @@
> >      var item = args.calendarEvent;
> >      item = item.clone(); // use an own copy of the passed item
> >      var calendar = item.calendar;
> >      window.calendarItem = item;
> > +    intializeTabOrWindowVariables();
> 
> We agreed to leave the calendar-summary-dialog as is for the moment. so
> please exclude changes to calendar-summary-dialog.js from the patch.

Ok, done.

> ::: calendar/base/content/preferences/general.xul
> @@ +64,5 @@
> >                          name="calendar.agendaListbox.soondays"
> >                          type="int"/>
> > +            <preference id="calendar.item.editInTab"
> > +                        name="calendar.item.editInTab"
> > +                        type="bool"/>
> 
> Why is this still needed? We agreed to have a only hidden pref for this
> patch, so please remove it for now.

An oversight, now removed.

> ::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
> @@ +29,5 @@
> >  
> >  #calendar-event-dialog .todo-only,
> > +#calendar-task-dialog  .event-only,
> > +#calendar-event-dialog-inner .todo-only,
> > +#calendar-task-dialog-inner  .event-only {
> 
> Why are #calendar-event-dialog and #calendar-task-dialog still needed - is
> this for the dynamic id updates?

For that and also toolbar and menu items are shown or hidden based on these ids / css rules (for events and for tasks).

> ::: calendar/lightning/content/lightning-item-iframe.js
> @@ +143,5 @@
> >      }
> >  }
> >  
> > +function sendMessage(message) {
> > +    parent.postMessage(message, "*");
> 
> Please make this aMessage instead of message. also, if you introduce a new
> function, please prepend a documentation block describing the purpose, input
> paramater(s) (with type, name and purpose) and return value (if any; also
> with type).

Ok, I've added documentation and changed the argument names across the board.

> @@ +211,5 @@
> >  function onLoad() {
> >      // Set a variable to allow/prevent actions when the dialog is loading.
> >      onLoad.isLoading = true;
> >  
> > +    window.addEventListener("message", receiveMessage, false);
> 
> give this a more meaningful name than message.

That's the "official" name of the event the listener is listening for, so we're stuck with it.

> @@ +1655,2 @@
> >   */
> > +function updatePrivacy(value) {
> 
> why do you need an argument here? The privacy status is stored in a global,
> so it's available here. This applies also on each of the updateXXX functions
> below.

Just following a functional programming style, trying to minimize functions with side-effects, since pure functions are easier to test, more composable, etc. And minimizing state-changing code simplifies things.

> ::: calendar/lightning/content/lightning-item-iframe.xul
> @@ +31,5 @@
> > +     document.loadOverlay() will not work on this one. -->
> > +<window id="calendar-event-dialog-inner"
> > +      onload="onLoad();"
> > +      onunload="onEventDialogUnload();"
> > +      onresize="rearrangeAttendees();"
> 
> This triggeres reference errors in window mode (in strict mode only), which
> should be avoided. To see them make sure you have enabled
> javascript.options.strict in the advanced preferences.

Ok, I'm now using javascript.options.strict but did not find any reference errors related to my code. (Maybe the other changes fixed them?)

> ::: calendar/lightning/content/lightning-item-panel.xul
> @@ +107,5 @@
> > +          <command id="cmd_customize"
> > +                   oncommand="onCommandCustomize()"/>
> > +          <command id="cmd_toggle_link"
> > +                   persist="checked"
> > +                   oncommand="toggleLink()"/>
> 
> toggleLink doesn't work atm. Trying to use the menu item throws also
> reference errors. Maybe the same problem as for rearrangeAttendees.

Now fixed.  (Note that in a tab there is no menu item or toolbar button to hide/show the link, so I've defaulted to always showing it in a tab.  We may want a toolbar button for this eventually.)

> ::: calendar/lightning/content/messenger-overlay-sidebar.js
> @@ +139,5 @@
> >      ltnSwitch2Mail();
> >    }
> >  };
> >  
> > +
> 
> One line too much.

Fixed.

Comment 13

[Paul Morris [:pmorris]]

Attachment: event-in-tab-july14.patch

This patch addresses the issues raised in previous review.  Passes mozmill tests locally.  At Philipp's request I'm going ahead and uploading it before doing a try server run. (I ran into some issues with my LDAP/SSH setup so I'll work on fixing them in parallel while Philipp gets the try server run going.)

Comment 14

[Paul Morris [:pmorris]]

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

Comment 15

[Paul Morris [:pmorris]]

Attachment: event-in-tab-july21.patch

Addresses a number of issues with previous patch. Passes mozmill tests locally.  Needs to be tested with a try server run.

Comment 16

[[:MakeMyDay]]

Are there any other changes apart from the fixes for the issues discussed on IRC?

Comment 17

[Paul Morris [:pmorris]]

Yes, it includes fixes and improvements for a few other things I had noticed, and a little refactoring.

- toolbar buttons / menu items being enabled/disabled and persisting their state when switching tabs (invite attendees, attach, privacy, priority, show timezones)
- making sure that some commands are available where needed, either inside the iframe, outside the iframe, or both.  For example, cmd_attach_url is needed in both places, fixes non-functioning 'Attach Webpage...' in attendees context menu.  (cmd_attendees, cmd_attach_url, cmd_save, cmd_accept, cmd_email_undecided)
- icons for tabs

And for completeness, the fixes for issues raised on IRC:

- error in console when closing an unsaved tab and clicking 'don't save' or 'cancel' ('args.calendar is null') which could cause the tab not to close on 'don't save'
- error in console when closing customize toolbar dialog ('aArgs is undefined')
- closing the main TB window with unsaved tabs would not prompt to save changes, for all open unsaved tabs
- when the 'save changes' prompt appears, activate the tab it refers to, to bring it to the front

Comment 18

[[:MakeMyDay]]

Try push containing the latest patch:

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

Comment 19

[[:MakeMyDay]]

Comment on attachment 8773609 [details] [diff] [review]
event-in-tab-july21.patch

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

Ok, finally I got through this. I hope this is still in time after your week off.

First of all, great job! Even if there are a lot of comments below, nearly all of them are related to coding style and mostly based on our new styling which will land with bug 1280898. Apart from that, there are only a few issues left:

1. Wrong tab icons: You're using the calendar/task view icons instead of those used for the event/task dialog.

2. Missing context menu items in attendee context menue: In the attendee tab (either in dialog or window mode), the context menu is lacking the options to send an email to all or to yet undecided attendees (only visible if you have added at least one attendee).

3. Right aligned icons in tab mode toolbar: In tab mode, the toolbar doesn't seem to enforce the spring to be added, which causes the icons to be right aligned instead of left. The current dialog toolbar hasn't a spring by default, so I'm not sure whether adding the spring to the deafult set is enough or you need to inject it using a migration code similar to the one at the end of calendar/base/content/calendar-chrome-startup.js

I give you an r+ already now given you update the patch according to my comments and upload that together with an interdiff from the current version to allow for a brief sanity check. Also, please ping Philipp, whether he still wants to do a review on top of mine. If he doesn't, remove him from the patch header of the final patch.

If you want to go the extra mile, please add a brief documentation sketch of the new file structure used for event tab/dialog to an appropriate place to allow our contributors to easily get in touch with the new structure. But you can leave that also for the post-coding phase.

And before I forget about it: Please take it to your todo list to make the mozmill tests to run with both the window and the tab mode (to be clear, this is not for this patch).

::: calendar/base/content/calendar-item-editing.js
@@ +464,5 @@
>      // open the dialog modeless
>      let url;
>      if (isCalendarWritable(calendar)
>          && (mode == "new"
>              || (mode == "modify" && !isInvitation && userCanModifyItem((calendarItem))))) {

Can you make this

let isEditable = mode == "modify" && !isInvitation && userCanModifyItem(calendarItem);
if (isCalendarWritable(calendar) && (mode == "new" || isEditable)) {

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +498,5 @@
>      updateReminderDetails();
>  }
>  
>  /**
> + * Updates the related link on the dialog.  Currently only used by the

One whitepace too much.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +17,5 @@
>  #calendar-event-dialog,
>  #calendar-task-dialog {
> +    min-width: 550px;
> +    min-height: 550px;
> +}

Can you limit this to our current min value? At least on Windows this are:

min-height: 444px;
min-width: 476px;

@@ +29,5 @@
>  
>  #calendar-event-dialog .todo-only,
> +#calendar-task-dialog  .event-only,
> +#calendar-event-dialog-inner .todo-only,
> +#calendar-task-dialog-inner  .event-only {

Can you please remove the additional whitespace before both .event-only decalarations?

::: calendar/lightning/content/lightning-item-iframe.js
@@ +146,5 @@
>  
>  /**
> + * Sends an asynchronous message to the parent context that contains the iframe.
> + *
> + * @param aMessage      Object, the message being sent.

Please use the following pattern:

/**
 * A description of the purpose of the function
 *
 * @param  {Type} aParamName  A brief description of the param
 * @return {Type}             A brief description of the return value
 */

@param and @return are mandatory if a function uses such, othrewise obsolete.

In this case it should then be

/**
 * Sends an asynchronous message to the parent context that contains the iframe.
 *
 * @param  {Object} aMessage  The message to pass to the parent context
 */

@@ +155,5 @@
>  
>  /**
> + * Receives asynchronous messages from the parent context that contains the iframe.
> + *
> + * @param aEvent    The object containing the message being received.

See the doc pattern comment above.

@@ +278,5 @@
>  
>          // store the 'finalize'-functor in the provided job-object.
>          args.job.finalize = function() {
>              // store any pending modifications...
>              self.onAccept();

Use an arrow function here instead:

args.job.finalize = () => {
 // store any pending modifications...
 this.onAccept();
...
}

and remove the self decalaration above and the part of the comment regarding keeping the context.

@@ +303,5 @@
>  
>      // new items should have a non-empty title.
>      if (item.isMutable && (!item.title || item.title.length <= 0)) {
>          item.title = calGetString("calendar-event-dialog",
>                                    isEvent(item) ? "newEvent" : "newTask");

Please make this cal.calGetString(...)

@@ +444,5 @@
>                                     isEvent(window.calendarItem) ?
>                                        "askSaveTitleEvent" :
>                                        "askSaveTitleTask");
>      var promptMessage = calGetString("calendar",
>                                       isEvent(window.calendarItem) ?

Please make these cal.calGetString(...)

@@ +489,1 @@
>   */

See doc pattern comment above.

@@ +489,2 @@
>   */
> +function onCancel(aIframeId, preventClose) {

Can you make this aPreventClose instead?

@@ +616,5 @@
> +                                              "itemMenuLabelTask");
> +        let accessKey = calGetString("calendar-event-dialog",
> +                                    isEvent ? "itemMenuAccesskeyEvent2" :
> +                                              "itemMenuAccesskeyTask2");
> +        sendMessage({command: "initializeItemMenu",

Can you make this

let accessKeyString = isEvent ? "itemMenuAccesskeyEvent2" : "itemMenuAccesskeyTask2";
let accessKey = cal.calGetString("calendar-event-dialog", accessKeyString);

The same applies to the label desclaration above.

@@ +875,3 @@
>              gStartTime = gStartTime.getInTimezone(kDefaultTimezone);
>          }
>          gStartTime.isDate = allDay;

Can you make this:

if (gTimezonesEnabled || allDay) {
    gStartTime = cal.jsDateToDateTime(getElementValue(startWidgetId), gStartTimezone);
    gStartTime = gStartTime.getInTimezone(kDefaultTimezone);
} else {
    gStartTime = cal.jsDateToDateTime(getElementValue(startWidgetId), kDefaultTimezone);
}

@@ +896,2 @@
>                  gEndTime = gEndTime.getInTimezone(kDefaultTimezone);
>              }

Here the same for gEndTime.

@@ +1391,5 @@
>          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>      }
> +    let newTitle = cal.calGetString("calendar", strName) + ": " +
> +                      getElementValue("item-title");
> +    sendMessage({command: "updateTitle", argument: newTitle});

Make this

 sendMessage({ command: "updateTitle", argument: newTitle });

@@ +1471,5 @@
>   *
>   * @param aEnable           true: enables the command
>   */
>  function enableAcceptCommand(aEnable) {
> +    sendMessage({command: "enableAcceptCommand", argument: aEnable});

sendMessage({ command: "enableAcceptCommand", argument: aEnable });

@@ +1714,3 @@
>   *
> + * @param aPrivacy      String, the current/old privacy value.
> + * @return              String, the new privacy value.

again, doc pattern.

@@ +1725,5 @@
>   * selected calendar. If the selected calendar does not support privacy or only
>   * certain values, these are removed from the UI. This function should be called
> + * any time that gConfig.privacy is updated, passing the new value as an argument.
> + *
> + * @param aValue    String, the new privacy value.

doc pattern

@@ +1765,2 @@
>   *
> + * @param aValue    String, the new priority value.

doc pattern

@@ +1801,3 @@
>   */
> +function updateStatus(aValue) {
> +    sendMessage({command: "updatePanelState", argument: {status: aValue}});

sendMessage({ command: "updatePanelState", argument: { status: aValue } });

@@ +1808,4 @@
>   * from BUSY (Opaque) to FREE (Transparent).
> + *
> + * @param aShowTimeAs   String, the current/old value.
> + * @return              String, the new value.

doc pattern

@@ +1822,1 @@
>   */

doc pattern

@@ +1824,2 @@
>      if (cal.isEvent(window.calendarItem)) {
> +        sendMessage({command: "updatePanelState", argument: {showTimeAs: aValue}});

sendMessage({ command: "updatePanelState", argument: { showTimeAs: aValue } });

@@ +2332,5 @@
>      // really a good idea?
>      if (gIsReadOnly) {
> +        let disableElements = document.getElementsByAttribute("disable-on-readonly", "true");
> +        for (let element of disableElements) {
> +            element.setAttribute('disabled', 'true');

Use double quotes instead of single quotes, please. This applies for the entire block below.

@@ +2351,3 @@
>          }
>      } else {
> +        sendMessage({command: 'removeDisableAndCollapseOnReadonly'});

sendMessage({ command: "removeDisableAndCollapseOnReadonly" });

@@ +3326,4 @@
>      setElementValue("cmd_attach_url", !hasAttachments && "true", "disabled");
> +    sendMessage({
> +        command: "updatePanelState",
> +        argument: {attachUrlCommand: hasAttachments}

argument: { attachUrlCommand: hasAttachments }

@@ +3338,1 @@
>   */

doc pattern

@@ +3338,5 @@
>   */
> +function updateItemURL(aShowLink) {
> +    let itemUrlString = window.calendarItem.getProperty("URL") || "";
> +
> +    function hideOrShow(aBool) {

Missing doc block.

Also, when defining functions within another function, this must either happen at the top of the function or you must make this an assignment to avoid a strict warning. So move this up or make it let hideOrShow = fucntion (aSomething) {...};

And finally, please make the argument name something purpose and not type related, like aShow.

@@ +3349,5 @@
> +    }
> +
> +    if (!itemUrlString.length) {
> +        // Disable if there is no url
> +        sendMessage({command: "disableLinkCommand"});

sendMessage({ command: "disableLinkCommand" });

@@ +3356,5 @@
> +    if (!aShowLink || !itemUrlString.length) {
> +        // Hide if there is no url, or the menuitem was chosen so that the url
> +        // should be hidden.
> +        hideOrShow(false);
> +    } else {

Please turn this upside down:

if (aShowLink && itemUrlString.length) {
...
} else {
...
}

@@ +3357,5 @@
> +        // Hide if there is no url, or the menuitem was chosen so that the url
> +        // should be hidden.
> +        hideOrShow(false);
> +    } else {
> +        let handler, uri;

Make this separate lines, please.

@@ +3377,5 @@
> +        setTimeout(function() {
> +          // HACK the url-link doesn't crop when setting the value in onLoad
> +          setElementValue("url-link", itemUrlString);
> +          setElementValue("url-link", itemUrlString, "href");
> +        }, 0);

Please make this an arrow function:

setTimeout(() => {
...
}, 0);

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +316,5 @@
> +                   value="&newtodo.percentcomplete.label;"/>
> +        </hbox>
> +      </row>
> +
> +          <separator id="event-grid-recurrence-separator" class="groove"/>

Indentation

@@ +621,5 @@
> +                command="cmd_email"/>
> +      <menuitem id="attendee-popup-sendtentativeemail-menuitem"
> +                label="&event.email.tentative.attendees.label;"
> +                accesskey="&event.email.tentative.attendees.accesskey;"
> +                command="cmd_email_undecided"/>

If you have added at least one attendee, the menuitems attendee-popup-sendemail-menuitem and attendee-popup-sendtentativeemail-menuitem are not visible currently, but they should.

::: calendar/lightning/content/lightning-item-panel.js
@@ +18,5 @@
> +    // is false, which means the UI will not be shown
> +}
> +
> +// XXX Are these menu commands needed when in a tab?  If not, put them
> +// in a separate JS file for just the dialog window to use?

I think you need that also in this case. You easilly can test this by commenting these out, run in tab mode and see whether the clipboard commands (either in the context menu or the edit menu) get still updated when e.g. (de)selecting a text.

@@ +142,5 @@
> + *
> + * @param aMessage      The message being sent, usually a javascript object.
> + * @param aIframeId     String (optional), the id of the iframe.
> + */
> +function sendMessage(aMessage, aIframeId) {

doc pattern

@@ +164,5 @@
> + * window, or not if 'cancel' was clicked.  Requires sending and receiving
> + * async messages from the iframes of all open item tabs.
> + *
> + * @param response      Boolean, the response from the tab's iframe
> + */

doc pattern

@@ +183,5 @@
> + * Listener function for window close.  We prevent the window from
> + * closing, then for each open tab we prompt the user to save any
> + * unsaved changes with handleWindowClose.
> + *
> + * @param event     The window close event.

doc pattern

@@ +229,5 @@
> + * just use the properties they need from it.
> + *
> + * @param aArg      Object, its properties hold data about the event/task.
> + */
> +function updateItemTabState(aArg) {

doc pattern

@@ +252,5 @@
> + *
> + * @param label         String, The new name for the menu.
> + * @param accessKey     String, The access key for the menu.
> + */
> +function initializeItemMenu (label, accessKey) {

doc pattern; Use aLabel and aAccesskey

@@ +269,5 @@
> +
> +/**
> + * Handler for when dialog is canceled.
> + *
> + * @param aIframeId     String, the id of the iframe.

doc pattern

@@ +272,5 @@
> + *
> + * @param aIframeId     String, the id of the iframe.
> + */
> +function onCancel(aIframeId) {
> +    sendMessage({command: "onCancel", iframeId: aIframeId}, aIframeId);

sendMessage({ command: "onCancel", iframeId: aIframeId }, aIframeId);

@@ +281,5 @@
> +
> +/**
> + * Closes tab or window. Called after prompting to save any unsaved changes.
> + *
> + * @param aIframeId     String, the id of the iframe.

doc pattern

@@ +286,5 @@
> + */
> +function closeWindowOrTab(iframeId) {
> +    if (!gTabmail) {
> +        window.close();
> +    } else {

Please make this

if (gTabmail) {
...
} else {
...
}

@@ +292,5 @@
> +            // Find the tab associated with this iframeId, and close it.
> +            let filterFunc = function (x) {
> +                return "iframe" in x && x.iframe.id == iframeId;
> +            };
> +            let myTabInfo = gTabmail.tabInfo.filter((filterFunc).bind(this))[0];

Make this an arrow function:

let myTabInfo = gTabmail.tabInfo.filter((x) => "iframe" in x && x.iframe.id == iframeId)[0];

@@ +306,5 @@
> +
> +/**
> + * Handler for saving the event or task.
> + *
> + * @param aIsClosing    Bool, is the tab or window closing.

doc pattern

@@ +309,5 @@
> + *
> + * @param aIsClosing    Bool, is the tab or window closing.
> + */
> +function onCommandSave(aIsClosing) {
> +    sendMessage({command:"onCommandSave", isClosing: aIsClosing});

sendMessage({ command:"onCommandSave", isClosing: aIsClosing });

@@ +316,5 @@
> +/**
> + * Handler for deleting the event or task.
> + */
> +function onCommandDeleteItem() {
> +    sendMessage({command:"onCommandDeleteItem"});

sendMessage({ command:"onCommandDeleteItem" });

@@ +322,5 @@
> +
> +/**
> + * Update the title of the tab or window.
> + *
> + * @param aNewTitle     String, the new title.

doc pattern

@@ +337,5 @@
> +/**
> + * Handler for edit attendees command.
> + */
> +function editAttendees() {
> +    sendMessage({command: "editAttendees"});

sendMessage({ command: "editAttendees" });

@@ +344,5 @@
> +/**
> + * Handler for rotate privacy command.
> + */
> +function rotatePrivacy() {
> +    sendMessage({command: "rotatePrivacy"});

sendMessage({ command: "rotatePrivacy" });

@@ +353,5 @@
> + * the attribute "privacy" of the UI-element aTarget.
> + *
> + * @param aTarget    the calling UI-element;
> + * @param aEvent     the UI-element selection event (only for the popup menu
> + *                   event-privacy-menupopup in the Privacy toolbar button).

doc pattern

@@ +360,5 @@
> +    if (aEvent) {
> +        aEvent.stopPropagation();
> +    }
> +    sendMessage({command: "editPrivacy",
> +                 value: aTarget.getAttribute("privacy")});

sendMessage({
    command: "editPrivacy",
    value: aTarget.getAttribute("privacy")
});

@@ +372,5 @@
> + *
> + *  @param aArg      Object, with: { privacy: string,
> + *                                   hasPrivacy: bool,
> + *                                   calendarType: string,
> + *                                   privacyValues: array-of-strings }

doc pattern

@@ +383,5 @@
> +        let privacyMenuItem = document.getElementById("options-privacy-menu");
> +        if (privacyMenuItem) {
> +            setElementValue("options-privacy-menu", "true", "disabled");
> +        }
> +    } else {

Please make this

if (aArg.hasPrivacy) {
...
} else {
...
}

@@ +391,5 @@
> +        let menupopup = document.getElementById("event-privacy-menupopup");
> +        if (menupopup) {
> +            // Only update the toolbar if the button is actually there
> +            for (let node of menupopup.childNodes) {
> +                // XXX Is currentProvider still doing anything in this function?

Yes, not every provider needs to support this, see comment below. This get's relevant when you switch between calendars of different types.

@@ +480,5 @@
> +/**
> + * Handler for rotate priority command.
> + */
> +function rotatePriority() {
> +    sendMessage({command: "rotatePriority"});

sendMessage({ command: "rotatePriority" });

@@ +488,5 @@
> + * Handler to change the priority.
> + *
> + * @param aTarget    A XUL node with a value attribute which should be
> + *                   the new priority.
> + */

doc pattern

@@ +491,5 @@
> + *                   the new priority.
> + */
> +function editPriority(aTarget) {
> +    sendMessage({command: "editPriority",
> +                 value: parseInt(aTarget.getAttribute("value"))});

sendMessage({
    command: "editPriority",
    value: parseInt(aTarget.getAttribute("value"))
});

@@ +499,5 @@
> + * Updates the dialog controls related to priority.
> + *
> + * @param aArg   Object, with { priority: value,
> + *                              hasPriority: value }
> + */

doc pattern

@@ +563,5 @@
> +function rotateStatus() {
> +    let noneCmd = document.getElementById("cmd_status_none");
> +    let isVisible = !noneCmd.hasAttribute("hidden");
> +    sendMessage({command: "rotateStatus",
> +                 noneCommandIsVisible: isVisible});

sendMessage({
    command: "rotateStatus",
    noneCommandIsVisible: isVisible
});

@@ +571,5 @@
> + * Handler to change the status from the dialog elements.
> + *
> + * @param aTarget    A XUL node with a value attribute which should be
> + *                   the new status.
> + */

doc pattern

@@ +574,5 @@
> + *                   the new status.
> + */
> +function editStatus(aTarget) {
> +    sendMessage({command: "editStatus",
> +                 value: aTarget.getAttribute("value")});

sendMessage({
    command: "editStatus",
    value: aTarget.getAttribute("value")
});

@@ +580,5 @@
> +
> +/**
> + * Update the dialog controls related to status.
> + *
> + * @param aArg   Object, with { status: string }

doc pattern

@@ +587,5 @@
> +    let found = false;
> +    const statusLabels = ["status-status-tentative-label",
> +                          "status-status-confirmed-label",
> +                          "status-status-cancelled-label"];
> +    setBooleanAttribute("status-status", "collapsed", true);

Please define statusLabels before found.

@@ +591,5 @@
> +    setBooleanAttribute("status-status", "collapsed", true);
> +    [ "cmd_status_none",
> +      "cmd_status_tentative",
> +      "cmd_status_confirmed",
> +      "cmd_status_cancelled" ].forEach(

I'm not sure about this from the top of my head, but i think this should be without a whitespace before the first and after the last element. Apart from that you should make this an arrow function:

[...].forEach((aElement, aIndex, aArray) => {
...
});

@@ +610,5 @@
> +      );
> +    if (!found) {
> +        // The current Status value is invalid. Change the status to not
> +        // specified and update the status again.
> +        sendMessage({command: "editStatus", value: "NONE"});

sendMessage({ command: "editStatus", value: "NONE" });

@@ +618,5 @@
> +/**
> + * Handler for rotate transparency command.
> + */
> +function rotateShowTimeAs() {
> +    sendMessage({command: "rotateShowTimeAs"});

sendMessage({ command: "rotateShowTimeAs" });

@@ +626,5 @@
> + * Handler to change the transparency from the dialog elements.
> + *
> + * @param aTarget    A XUL node with a value attribute which should be
> + *                   the new transparency.
> + */

doc pattern

@@ +629,5 @@
> + *                   the new transparency.
> + */
> +function editShowTimeAs(aTarget) {
> +    sendMessage({command: "editShowTimeAs",
> +                 value: aTarget.getAttribute("value")});

sendMessage({
    command: "editShowTimeAs",
    value: aTarget.getAttribute("value")
});

@@ +635,5 @@
> +
> +/**
> + * Update the dialog controls related to transparency.
> + *
> + * @param aArg       Object, with { showTimeAs: string }

doc pattern

@@ +656,5 @@
> +
> +/**
> + * Get the timezone button state.
> + *
> + * @return    Bool, true is active/checked, false is inactive/unchecked.

doc pattern

@@ +667,5 @@
> +/**
> + * Set the timezone button state.  Used to keep the toolbar button in
> + * sync when switching tabs.
> + *
> + * @aArg    Object, with { timezonesEnabled: bool }

doc pattern

@@ +683,5 @@
> +    let cmdTimezone = document.getElementById('cmd_timezone');
> +    let currentState = getTimezoneCommandState();
> +    cmdTimezone.setAttribute("checked", currentState ? "false" : "true");
> +    gConfig.timezonesEnabled = !currentState;
> +    sendMessage({command: "toggleTimezoneLinks", checked: !currentState});

sendMessage({ command: "toggleTimezoneLinks", checked: !currentState });

@@ +691,5 @@
> + * Toggles the visibility of the related link (rfc2445 URL property).
> + */
> +function toggleLink() {
> +    let linkCommand = document.getElementById("cmd_toggle_link");
> +    let checked = (linkCommand.getAttribute("checked") == "true");

Please remove the useless parantheses:

let checked = linkCommand.getAttribute("checked") == "true";

@@ +694,5 @@
> +    let linkCommand = document.getElementById("cmd_toggle_link");
> +    let checked = (linkCommand.getAttribute("checked") == "true");
> +
> +    linkCommand.setAttribute("checked", checked ? "false" : "true");
> +    sendMessage({command: "toggleLink", checked: !checked});

sendMessage({ command: "toggleLink", checked: !checked });

@@ +701,5 @@
> +/**
> + * Prompts the user to attach an url to this item.
> + */
> +function attachURL() {
> +    sendMessage({command: "attachURL"});

sendMessage({ command: "attachURL" });

@@ +707,5 @@
> +
> +/**
> + * Updates dialog controls related to item attachments.
> + *
> + * @param aArg         Object, with { attachUrlCommand: boolean }

doc pattern

@@ +717,5 @@
> +/**
> + * Updates attendees command enabled/disabled state.
> + *
> + * @param aArg         Object, with { attendeesCommand: boolean }
> + */

doc pattern

@@ +726,5 @@
> +/**
> + * Enables/disables the commands cmd_accept and cmd_save related to the
> + * save operation.
> + *
> + * @param aEnable           true: enables the command

doc pattern

@@ +752,5 @@
> +/**
> + * Handler to toggle toolbar visibility.
> + *
> + * @param aToolbarId        The id of the XUL toolbar node to toggle.
> + * @param aMenuitemId       The corresponding menuitem in the view menu.

doc pattern

@@ +771,5 @@
> +    // toggle visibility of the toolbar
> +    toolbar.collapsed = !toolbarCollapsed;
> +
> +    document.persist(aToolbarId, 'collapsed');
> +    document.persist(aMenuItemId, 'checked');

Please use double quotes instead of single quotes.

@@ +779,5 @@
> + * Called after the customize toolbar dialog has been closed by the
> + * user. We need to restore the state of all buttons and commands of
> + * all customizable toolbars.
> + *
> + * @param aToolboxChanged       If true, the toolbox has changed.

doc pattern

@@ +781,5 @@
> + * all customizable toolbars.
> + *
> + * @param aToolboxChanged       If true, the toolbox has changed.
> + */
> +function DialogToolboxCustomizeDone(aToolboxChanged) {

Don't start the function name with an upper case.

@@ +793,5 @@
> +        }
> +    }
> +
> +    // make sure our toolbar buttons have the correct enabled state restored to them...
> +    document.commandDispatcher.updateCommands('itemCommands');

double quotes

@@ +812,5 @@
> +    // done after a toolbar has been customized.
> +    let toolboxId = "event-toolbox";
> +
> +    let toolbox = document.getElementById(toolboxId);
> +    toolbox.customizeDone = DialogToolboxCustomizeDone;

toolbox.customizeDone = dialogToolboxCustomizeDone();

::: calendar/lightning/content/lightning-item-panel.xul
@@ +232,5 @@
> +                     customizable="true"
> +                     labelalign="end"
> +                     defaultlabelalign="end"
> +                     context="event-dialog-toolbar-context-menu"
> +                     defaultset="button-save,button-attendees,button-privacy,button-url,button-priority,button-status,button-freebusy,button-delete,spring"/>

Before the dialog toolbar hasn't had a trailing spring by default. I don't thing this will get added automatically if you have customized the toolbar before. Maybe you need a migration code like at the end of calendar/base/content/calendar-chrome-startup.js to make sure the spring is added. If the spring is missing, this leads to the right aligned toolbar in tab mode.

When it comes to add and lightning-item-appmenu-button in lightning-item-toolbar.xul, in general the same applies. However, for that we would want it to be visible and avialbale for customizatipon in tab mode only.

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +135,2 @@
>    /* because calendar does some direct menu manipulation, we need to change
>     *  to the mail mode to clean up after those hacks.

doc pattern

@@ +151,5 @@
> +        calendarTask: {
> +            type: "calendarTask"
> +        }
> +    },
> +    openTab: function(aTab, aArgs) {

missing doc block

@@ +179,5 @@
> +
> +        // Generate and set the tab title.
> +        let strName;
> +        if (aTab.mode.type == "calendarEvent") {
> +            strName = (aArgs.calendarEvent.title ? "editEventDialog" : "newEventDialog");

Remove the useless parentheses here

@@ +181,5 @@
> +        let strName;
> +        if (aTab.mode.type == "calendarEvent") {
> +            strName = (aArgs.calendarEvent.title ? "editEventDialog" : "newEventDialog");
> +        } else if (aTab.mode.type == "calendarTask") {
> +            strName = (aArgs.calendarEvent.title ? "editTaskDialog" : "newTaskDialog");

Once again.

@@ +199,5 @@
> +        aTab.iframe.contentWindow.arguments = [aArgs];
> +
> +        this.idNumber += 1;
> +    },
> +    saveTabState: function(aTab) {

Missing doc block

@@ +214,5 @@
> +        // move toolbox to the place where it can be accessed later
> +        let to = document.getElementById("lightningItemPanel").firstChild;
> +        moveEventToolbox(to);
> +    },
> +    showTab: function(aTab) {

Missing doc block

@@ +220,5 @@
> +        moveEventToolbox(aTab.panel.firstChild);
> +        Object.assign(gConfig, aTab.itemTabConfig);
> +        updateItemTabState(gConfig);
> +    },
> +    tryCloseTab: function(aTab) {

Missing doc block

@@ +230,5 @@
> +            onCancel(aTab.iframe.id);
> +            return false;
> +        }
> +    },
> +    closeTab: function(aTab) {

Missing doc block

@@ +239,5 @@
> +        }
> +        aTab.itemTabConfig = null;
> +    },
> +
> +    persistTab: function(aTab) {

Missing doc block and row too much.

@@ +248,5 @@
> +        // functionality), thus we confirm we have the expected values.
> +        if (args && args.calendar && args.calendar.id &&
> +            args.calendarevent && args.calendarEvent.id &&
> +            args.initialStartDateValue &&
> +            args.initialStartDateValue.icalString) {

What about tasks without a start date? This is a valid use case for which I would assume persisting to fail with that code.

@@ +266,5 @@
> +                args: args
> +            };
> +        }
> +    },
> +    restoreTab: function(aTabmail, aState) {

Missing doc block

@@ +782,5 @@
> + * Move the event toolbox containing the toolbar, into view for a tab
> + * or back to its hiding place where it is accessed again for other tabs.
> + *
> + * @param aDestination  DOM node, the destination where the toolbox will be moved.
> + */

doc pattern

::: calendar/lightning/themes/common/lightning.css
@@ +35,5 @@
>      width: 18px;
>      height: 18px;
>    }
>  }
> +

remove one line here.

::: calendar/lightning/themes/linux/lightning.css
@@ +90,3 @@
>      list-style-image: url(chrome://lightning-common/skin/mode-switch-icons.png);
>      -moz-image-region: rect(0px 64px 16px 48px);
>  }

This is the wrong icon. The icons used also in window mode live here:

https://dxr.mozilla.org/comm-central/source/calendar/base/themes/{OS}/icons

This applies for all the icon related changes in this file and for OS X and Windows.

::: calendar/test/mozmill/testLocalICS.js
@@ +48,5 @@
>      calUtils.getEventBoxPath(controller, "day", calUtils.CANVAS_BOX, undefined, 1, hour)), 1, 1);
>    controller.waitFor(function() {return mozmill.utils.getWindows("Calendar:EventDialog").length > 0}, sleep);
>    let event = new mozmill.controller.MozMillController(mozmill.utils
>      .getWindows("Calendar:EventDialog")[0]);
>    

Whitespace to be removed.

@@ +56,5 @@
>      + 'id("event-grid")/id("event-grid-rows")/id("event-grid-title-row")/'
>      + 'id("item-title")/anon({"class":"textbox-input-box"})/anon({"anonid":"input"})');
>    event.waitForElement(titleTextBox);
>    event.type(titleTextBox, title);
>    

Again whitespace.

Comment 20

[Paul Morris [:pmorris]]

Thanks for the review.  I've worked through everything and have some questions before making a new patch.

> 1. Wrong tab icons: You're using the calendar/task view icons instead of
> those used for the event/task dialog.

I have the correct icons working for linux and (I assume) windows, but I can't find the icon files for mac osx. There is no base/themes/osx/icons directory like there is for windows and linux, and looking around in base/themes/osx I couldn't find the icons in other directories. How to proceed?

Also, I have left the icon files where they are currently located -- under "base" rather than "lightning" -- and added them to the calendar/base/jar.mn file.  The xul and js files they are used with are "lightning" files rather than "base".  Let me know if I should move the icon files under the "lightning" directory (instead of "base") and add them to calendar/lightning/jar.mn

> 2. Missing context menu items in attendee context menu: In the attendee tab
> (either in dialog or window mode), the context menu is lacking the options
> to send an email to all or to yet undecided attendees (only visible if you
> have added at least one attendee).

Fixed. (It looks like this was not working before, unrelated to my patch.  At least it is not working with my release install of TB 45.2.0 and Lightning 4.7)

> 3. Right aligned icons in tab mode toolbar: In tab mode, the toolbar doesn't
> seem to enforce the spring to be added, which causes the icons to be right
> aligned instead of left. The current dialog toolbar hasn't a spring by
> default, so I'm not sure whether adding the spring to the deafult set is
> enough or you need to inject it using a migration code similar to the one at
> the end of calendar/base/content/calendar-chrome-startup.js

I see the migration code and how it could work, but given that the app menu button should be part of the toolbar for the tab and not for the window...  I think the best approach is probably to have two separate toolbars (with two separate ids), one for the tab that includes the app menu button in its set of buttons and the spring in its default set.  That way when users upgrade they get the default toolbar items for tabs (including the spring and app menu button) and their existing/customized toolbar items for windows.  Then there's no migration code needed.  I still need to look into how to do this with xul overlays.  What do you think?
 
> I give you an r+ already now given you update the patch according to my
> comments and upload that together with an interdiff from the current version
> to allow for a brief sanity check. Also, please ping Philipp, whether he
> still wants to do a review on top of mine. If he doesn't, remove him from
> the patch header of the final patch.

Ok, thanks, will do.

> If you want to go the extra mile, please add a brief documentation sketch of
> the new file structure used for event tab/dialog to an appropriate place to
> allow our contributors to easily get in touch with the new structure. But
> you can leave that also for the post-coding phase.

Ok, I'll leave this for post-coding phase.

> And before I forget about it: Please take it to your todo list to make the
> mozmill tests to run with both the window and the tab mode (to be clear,
> this is not for this patch).

Ok, I've made a note to do this.

I've made all the code style and formatting edits, but had questions about some things.

> Please use the following pattern:
> 
> /**
>  * A description of the purpose of the function
>  *
>  * @param  {Type} aParamName  A brief description of the param
>  * @return {Type}             A brief description of the return value
>  */
> 
> @param and @return are mandatory if a function uses such, othrewise obsolete.

I've followed this pattern and used jsdoc when in doubt (http://usejsdoc.org/tags-param.html).  However, I've indicated optional arguments as below to follow the current convention in the code. Let me know if another way of indicating optional arguments would be better.

/**
 * Send an asynchronous message to an iframe.
 *
 * @param {Object} aMessage   Contains the message being sent
 * @param {string} aIframeId  (optional) the id of the iframe
 */
function sendMessage(aMessage, aIframeId) {
...
};

To document object arguments and their properties, I've done this, per jsdoc:

/**
 * Updates the UI according to the privacy setting and the selected
 * calendar. If the selected calendar does not support privacy or only
 * certain values, these are removed from the UI. This function should
 * be called any time that privacy setting is updated.
 *
 * @param {Object}    aArg                Contains privacy properties
 * @param {string}    aArg.privacy        The new privacy value
 * @param {boolean}   aArg.hasPrivacy     Whether privacy is supported
 * @param {string}    aArg.calendarType   The type of calendar
 * @param {string[]}  aArg.privacyValues  The possible privacy values
 */
function updatePrivacy(aArg) {
...
};

Note the way an array of strings is indicated with "{string[]}" (for aArg.privacyValues).  

I'm tempted to omit the description of aArg ("Contains privacy properties") in cases like this since it is just a container for its properties.

I have used {XUL-node} and {DOM-node} as the type indication for some arguments.

Let me know if I'm on the right track with all of this.

> ::: calendar/lightning/content/lightning-item-panel.js
> @@ +18,5 @@
> > +    // is false, which means the UI will not be shown
> > +}
> > +
> > +// XXX Are these menu commands needed when in a tab?  If not, put them
> > +// in a separate JS file for just the dialog window to use?
> 
> I think you need that also in this case. You easilly can test this by
> commenting these out, run in tab mode and see whether the clipboard commands
> (either in the context menu or the edit menu) get still updated when e.g.
> (de)selecting a text.

Turns out these commands do not need to be defined for the tab case because they are already defined in other files for the main window and menu as a whole.  So I have moved them to the onLoad function where they are only defined for the window case like so:

        /**
         * Update menu items that rely on focus.
         */
        window.goUpdateGlobalEditMenuItems = () => {
        ...
        }

> @@ +781,5 @@
> > + * all customizable toolbars.
> > + *
> > + * @param aToolboxChanged       If true, the toolbox has changed.
> > + */
> > +function DialogToolboxCustomizeDone(aToolboxChanged) {
> 
> Don't start the function name with an upper case.

Done. (And FWIW/in my defense, it was already upper case before I moved it to this file.)

> ::: calendar/lightning/content/lightning-item-panel.xul
> @@ +232,5 @@
> > +                     customizable="true"
> > +                     labelalign="end"
> > +                     defaultlabelalign="end"
> > +                     context="event-dialog-toolbar-context-menu"
> > +                     defaultset="button-save,button-attendees,button-privacy,button-url,button-priority,button-status,button-freebusy,button-delete,spring"/>
> 
> Before the dialog toolbar hasn't had a trailing spring by default. I don't
> thing this will get added automatically if you have customized the toolbar
> before. Maybe you need a migration code like at the end of
> calendar/base/content/calendar-chrome-startup.js to make sure the spring is
> added. If the spring is missing, this leads to the right aligned toolbar in
> tab mode.
> 
> When it comes to add and lightning-item-appmenu-button in
> lightning-item-toolbar.xul, in general the same applies. However, for that
> we would want it to be visible and avialbale for customizatipon in tab mode
> only.

Maybe we need two separate toolbars?  See above.

> @@ +248,5 @@
> > +        // functionality), thus we confirm we have the expected values.
> > +        if (args && args.calendar && args.calendar.id &&
> > +            args.calendarevent && args.calendarEvent.id &&
> > +            args.initialStartDateValue &&
> > +            args.initialStartDateValue.icalString) {
> 
> What about tasks without a start date? This is a valid use case for which I
> would assume persisting to fail with that code.

Hmmm... I tried creating a new task in Lightning without entering anything for a start date and it worked to persist it.  But maybe other calendars/apps might create tasks that have empty/null start dates?  How would we handle restoring them?  I added 

  aState.args.initialStartDateValue = null;

to the restoreTab function to test this, and I got "TypeError: cdt is null" and/or "TypeError: startTime is null" in the console when restoring the tab (because a date was expected).  

So maybe we would need to provide some kind of default start date?  But that seems odd to change the data for the task...  Or maybe we can/should just expect tasks to have a start date?

(While working on this I fixed a couple of minor things that were preventing tabs from being persisted.)

Comment 21

[Stefan Sitter [:ssitter]]

Tasks without start date or tasks without due date or tasks without both dates are valid use cases and have to be supported by the new in tab editing.

I don't know if event in tab editing will be supported for SeaMonkey too. But could you test and verify that your changes doesn't regress Lightning in SeaMonkey and that at least edit in dialog still works?

Comment 22

[Paul Morris [:pmorris]]

(In reply to Stefan Sitter from comment #21)
> Tasks without start date or tasks without due date or tasks without both
> dates are valid use cases and have to be supported by the new in tab editing.

Turns out this was a false alarm. args.initialStartDateValue is just the initial start date which is loaded into the datepicker.  For a task it is not set on the actual task (as its start date) unless you check the start date checkbox.  So these are two separate things.

args.initialStartDateValue should always have a date value, but if for some reason it doesn't, I've got the code assigning it a default date value when restoring a tab.

> I don't know if event in tab editing will be supported for SeaMonkey too.
> But could you test and verify that your changes doesn't regress Lightning in
> SeaMonkey and that at least edit in dialog still works?

I will create a follow-up bug to verify that there are no regressions on SeaMonkey, to fix them if there are, and if feasible to offer in-tab editing.

Comment 23

[Paul Morris [:pmorris]]

Attachment: event-in-tab-aug5.patch

This patch addresses all the issues with the previous version, including adding the app/hamburger toolbar button and a spring (for alignment) to the tab toolbar, and migrating the user's customized window dialog toolbar items to the tab toolbar on upgrade.

Passes mozmill tests.  Needs testing with try server run.

Comment 24

[Paul Morris [:pmorris]]

Attachment: interdiff-aug5.diff

An interdiff between the previous two patches: 
8773609: event-in-tab-july21.patch
8778484: event-in-tab-aug5.patch
Bugzilla was choking on/taking forever showing differences between them.

Comment 25

[Richard Marti (:Paenglab)]

Comment on attachment 8778484 [details] [diff] [review]
event-in-tab-aug5.patch

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

Over all this looks really very promising

::: calendar/lightning/content/lightning.js
@@ +150,5 @@
>  // occurrence of a repeating item in calFilter
>  pref("calendar.filter.maxiterations", 50);
>  
> +// Edit events and tasks in a tab (by default) rather than a window
> +pref("calendar.item.editInTab", false);

The comment says in tab is default but the pref is on false.

Should this be fixed or is the current setting only for before check-in?

::: calendar/lightning/themes/common/lightning.css
@@ +52,5 @@
> + *   Event dialog toolbar buttons
> + *-------------------------------------------------------------------*/
> +
> +#button-save {
> +  list-style-image: url(chrome://calendar-common/skin/calendar-toolbar.svg#save);

Indentation in this file should be 4 spaces.

All this rules for the button icons (not the inverted) are working for Linux, OS X <10.11 and Win 7. But not for XP there are still the old icons used and Win8+ and OS X 10.11+ where flat icons are used. If you want, I have a fix for this.

But why don't you add common/calendar-event-dialog.css and calendar-event-dialog.css to the includes? Then you wouldn't need to duplicate styles what makes maintenance harder in the future?

::: calendar/lightning/themes/windows/lightning.css
@@ +139,5 @@
>  
> +  .icon-holder[type="calendarEvent"],
> +  .tabmail-tab[type="calendarEvent"] {
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog.png);
> +  }

Instead of adding this and the following rule here and the same in the no-XP section you can add it before the 

@media (-moz-os-version: windows-xp) {

line where the common rules are.

And add after both list-style-image lines a -moz-image-region: auto;

Windows has a -moz-image-region set for the default icon.

Comment 26

[Richard Marti (:Paenglab)]

Attachment: repeatEvent.png

Maybe not for this bug but for a follow-up: in tab the repeat details text is in three lines but in a tab we have enough space to show them on one line.

To try you can use:

#repeat-details {
  -moz-box-orient: horizontal;
}

This would also need adjustments to the label spacing to look like a normal sentence.

Comment 27

[Richard Marti (:Paenglab)]

With the current patch I see two issues:

1. Read only Events can't be closed. Not with the tab close button, nor with the OK/Cancel button on bottom. Also are the toolbar buttons all active where they should be disabled and also the OK/Cancel buttons on bottom are shown.

2. The Privacy, Busy etc. status icons are shown in the statusbar. That's okay when the bar is shown, but what when the statusbar is hidden? Then a Event/Task own statusbar with the states should be shown.

Comment 28

[[:MakeMyDay]]

Richard, thanks for commenting.

Can you elaborate on the read only issue a bit by providing STR? In case an item is read only when opening, it currently should not be opened in tab but in the event-summray-dialog in window mode as before. Is this about a calendar getting ro while an event is opened in a tab already? 

Regarding the statusbar issue: is the statusbar displayed by default in our shipped themes for all OS?

Comment 29

[Richard Marti (:Paenglab)]

Attachment: Local.ics

(In reply to [:MakeMyDay] from comment #28)
> Richard, thanks for commenting.
> 
> Can you elaborate on the read only issue a bit by providing STR? In case an
> item is read only when opening, it currently should not be opened in tab but
> in the event-summray-dialog in window mode as before. Is this about a
> calendar getting ro while an event is opened in a tab already? 

I attached the event. It's the one you sent one time for testing.
In dialog mode, the event-summray-dialog opens on double click.

STR:
- With newly started TB and no open Calendar/Task/New Event tab, open the Calendar tab.
- Now double click the event I sent (you need to edit the RTICIPANT from my email to yours before import).

With dialog mode everything is okay. It opens in event-summray-dialog.
In tab mode it opens in tab and gives following errors in console:

Sat Aug 06 2016 15:01:42
Error: Failed to read 'dialog.tooltip.attendeeRole2.REQ-PARTIC
 IPANT' from chrome://calendar/locale/calendar.properties. Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js :: calGetString :: line 677"  data: no]
Source file: resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js
Line: 683
 ----------
Sat Aug 06 2016 15:01:42
Error: Failed to read 'dialog.tooltip.attendeeRole2.REQ-PA
 RTICIPANT' from chrome://calendar/locale/calendar.properties. Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js :: calGetString :: line 677"  data: no]
Source file: resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js
Line: 683
 ----------
Sat Aug 06 2016 15:01:42
Error: TypeError: opener is null
Source file: chrome://calendar/content/calendar-summary-dialog.js
Line: 185

Trying to close through tab close button or Cancel button shows no errors. Clicking on OK shows this errors:

Sat Aug 06 2016 15:03:38
Error: TypeError: this.mOldItem is null
Source file: resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calTransactionManager.js
Line: 167
 ----------
Sat Aug 06 2016 15:03:38
Error: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.mOldItem is null" {file: "resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calTransactionManager.js" line: 167}]'[JavaScript Error: "this.mOldItem is null" {file: "resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calTransactionManager.js" line: 167}]' when calling method: [nsITransaction::doTransaction]
Source file: resource://calendar/modules/calUtils.jsm -> file:///Z:/Mozilla/TB/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calTransactionManager.js
Line: 42

To remove this tab I need to delete the session files in my profile.

> Regarding the statusbar issue: is the statusbar displayed by default in our
> shipped themes for all OS?

As I know, yes. But this Issue would be an intermediate problem until the changes from the mockups, where the states will be shown in menulists, will be done.

Comment 30

[Richard Marti (:Paenglab)]

I Forgot to write, I tested this also with only this patch in the queue with the same result.

Comment 31

[Richard Marti (:Paenglab)]

Attachment: tab-icons.zip

Icons for the tabs in 16x16.

I renamed them to calendar-event-tab.png and calendar-task-tab.png to not collide with existing files.

Comment 32

[[:MakeMyDay]]

Comment on attachment 8778484 [details] [diff] [review]
event-in-tab-aug5.patch

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

Check the migration code implementation how this would work when launched in window mode (see below) as well as the originTarget when posting messages. While opening r/o events in a tab can be easily prevented, how does the tab behave if the calendar of the event gets switched to r/o while the tab is open? If there are issues in such a scenario, this should be handled in a follow-up bug.

Apart from that, please consider comment 25 from Richard, make use of the icons from comment 31 and take care of the further style nits below.

The optimization from comment 26 (as well as no 2 from comment 27 if that will be considered an issue) can be left for a follow-up bug. The same applies for further file renaming and moving around.

So r- for now as we need another patch version anyway, but you're almost there. And again, an interdiff is appreciated when uploading the updated patch.

::: calendar/base/content/calendar-chrome-startup.js
@@ +160,5 @@
>              calbar.insertItem("calendar-appmenu-button");
>              let taskbar = document.getElementById("task-toolbar2");
>              taskbar.insertItem("task-appmenu-button");
>          }
> +        if (currentUIVersion < 2) {

How does this code behave when running in window mode, which is still the default value for now? Don't you have to make an additional check here to make sure you're in tab mode to prevent failing the below code, especailly assigning things to tabBar?

@@ +171,5 @@
>  
> +            if (xulStore.hasValue(uri, "event-toolbar", "currentset")) {
> +                let windowSet = xulStore.getValue(uri, "event-toolbar", "currentset");
> +                let items = "calendar-item-appmenu-button";
> +                if (!windowSet.includes("spring")) {

It's probably not enough to check for the existance of a spring but also to make sure it is the trailing toolbar item already.

@@ +174,5 @@
> +                let items = "calendar-item-appmenu-button";
> +                if (!windowSet.includes("spring")) {
> +                    items = "spring," + items;
> +                }
> +                let tabSet = windowSet == "__empty" ? items : windowSet += "," + items;

Please make this

let previousSet = windowSet == "__empty" ? "" : windowSet + ",";
let tabSet = previousSet + items;

@@ +178,5 @@
> +                let tabSet = windowSet == "__empty" ? items : windowSet += "," + items;
> +                let tabBar = document.getElementById("event-tab-toolbar");
> +
> +                tabBar.currentSet = tabSet;
> +                tabBar.setAttribute("currentset", tabSet);

Can you eleborate why you need currentSet and currentset here?

::: calendar/base/content/calendar-item-editing.js
@@ +473,2 @@
>      } else {
>          url = "chrome://calendar/content/calendar-summary-dialog.xul";

Add an args.inTab = false here to effectively prevent opening invitations and read-only items in the tab.

@@ +473,5 @@
>      } else {
>          url = "chrome://calendar/content/calendar-summary-dialog.xul";
>      }
>  
> +    if (args.inTab && isCalendarWritable(calendar)) {

With the above args.inTab modification, you can spare the isCalendarWritable check here.

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +498,5 @@
>  }
>  
>  /**
> + * Updates the related link on the dialog.  Currently only used by the
> + * read-only summary dialog.

This a Whitespace too much in the comment.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +44,3 @@
>  
>  var eventDialogQuitObserver = {
>    observe: function(aSubject, aTopic, aData) {

Can you please add a doc block here...

@@ +94,5 @@
>              }
>          }
>      },
>  
>      onDeleteItem: function(aDeletedItem) {

... and here?

@@ +137,1 @@
>   */

As you touched this, please rename item to aItem here also add the type declaration here. These are

calICalendar for aCalendar and
calIItemBase for aItem

@@ +146,5 @@
>  
>  /**
> + * Sends an asynchronous message to the parent context that contains the iframe.
> + *
> + * @param {Object} aMessage  The message to pass to the parent context

Even if I mentioned that a specifying object properties is not required, here this would be helpful to explain the expected structure of the Object as this is key for the new implementation.

@@ +151,3 @@
>   */
> +function sendMessage(aMessage) {
> +    parent.postMessage(aMessage, "*");

Can you make the targetOrigin here specific rahther then *?

@@ +155,5 @@
>  
>  /**
> + * Receives asynchronous messages from the parent context that contains the iframe.
> + *
> + * @param {Object} aEvent  Contains the message being received

Be more precise here: you're expecting a MessageEvent not just an Object.

@@ +1787,2 @@
>   */
> +function rotateStatus(aStatus, aCalIsEvent, aNoneCommandIsVisible) {

make this aIsEvent instead of aCalIsEvent

@@ +3351,5 @@
> +            setElementValue("event-grid-link-separator", !aShow && "true", "hidden");
> +        }
> +    };
> +
> +    let itemUrlString = window.calendarItem.getProperty("URL") || "";

You would have need either assign the function to a variable or move it to the top. So leave it here with

function hideOrShow() {
...
}

@@ +3357,5 @@
> +        // Disable if there is no url
> +        sendMessage({ command: "disableLinkCommand" });
> +    }
> +
> +    if (aShowLink || itemUrlString.length) {

This must be && instead of ||

!(!foo || !bar) equals to (foo && bar)

::: calendar/lightning/content/lightning-item-panel.js
@@ +84,5 @@
> +
> +/**
> + * Receive an asynchronous message from the iframe.
> + *
> + * @param aEvent    The object containing the message being received.

Make this more specific: you're expecting a MessageEvent here.

@@ +140,5 @@
> + * Send an asynchronous message to an iframe.
> + *
> + * @param {Object} aMessage   Contains the message being sent
> + * @param {string} aIframeId  (optional) the id of the iframe
> + */

As already mentioned above, it would be helpful if you can describe the expected structure of Object - it would be sufficient here if you reference the other description.

@@ +153,5 @@
> +    } else {
> +        iframeId = "lightning-item-panel-iframe";
> +    }
> +    let iframe = document.getElementById(iframeId);
> +    iframe.contentWindow.postMessage(aMessage, "*");

Can make the targetOrigin specific here rather then using *?

@@ +171,5 @@
> +        return;
> +    } else if (gItemTabIdsCopy.length > 0) {
> +        let nextId = gItemTabIdsCopy.shift();
> +        sendMessage({ command: "closingWindowWithTabs", id: nextId }, nextId);
> +    } else {

Can you add brief comments for each of the else blocks like in the initial if block?

@@ +595,5 @@
> +    setBooleanAttribute("status-status", "collapsed", true);
> +    ["cmd_status_none",
> +     "cmd_status_tentative",
> +     "cmd_status_confirmed",
> +     "cmd_status_cancelled"].forEach((element, index, array) => {

Please define for the commands also a const above and use that for the iteration. Also, you didn't rename the arguments to aElement etc.

::: calendar/lightning/content/lightning.js
@@ +150,5 @@
>  // occurrence of a repeating item in calFilter
>  pref("calendar.filter.maxiterations", 50);
>  
> +// Edit events and tasks in a tab (by default) rather than a window
> +pref("calendar.item.editInTab", false);

As mentioned on IRC, make the comment

// Edit events and tasks in a tab rather than a window

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +141,5 @@
>      ltnSwitch2Mail();
>    }
>  };
>  
> +var calendarItemTabType = {

Can you reference the mailTabs.js description for more details of tab info objects.

@@ +288,5 @@
> +            // Note that this is not the start date for the event or task.
> +            let initialStartDate =
> +                (args.initialStartDateValue && args.initialStartDateValue.icalString) ?
> +                    args.initialStartDateValue.icalString :
> +                    null;

Can you make this

let hasDateValue = args.initialStartDateValue &&
                   args.initialStartDateValue.icalString;
let initialStartDate = hasDateValue
    ? args.initialStartDateValue.icalString : null;

@@ +317,5 @@
> +        if (aState.args && aState.calendarId && aState.itemId) {
> +            aState.args.initialStartDateValue =
> +                aState.initialStartDate ?
> +                    cal.createDateTime(aState.initialStartDate) :
> +                    getDefaultStartDate();

Can you make this

aState.args.initialStartDateValue = aState.initialStartDate
    ? cal.createDateTime(aState.initialStartDate) : getDefaultStartDate();

Comment 33

[Paul Morris [:pmorris]]

Attachment: event-in-tab-aug7.patch

Thanks for the review and feedback.  This patch addresses the issues raised about the previous patch.

Given the discussion on IRC, targetOrigin in the message passing code is still "*" since other values have not worked.

Passes mozmill tests.  Still needs a TryServer run. (I got an error when I tried to start one.)

Comment 34

[Paul Morris [:pmorris]]

Attachment: interdiff-aug7.diff

Interdiff for August 7 patch compared to previous August 5 patch.

Comment 35

[Paul Morris [:pmorris]]

Thanks for the review and suggestions.  Here's further explanation for a couple of things.

> ::: calendar/base/content/calendar-chrome-startup.js
> @@ +160,5 @@
> >              calbar.insertItem("calendar-appmenu-button");
> >              let taskbar = document.getElementById("task-toolbar2");
> >              taskbar.insertItem("task-appmenu-button");
> >          }
> > +        if (currentUIVersion < 2) {
> 
> How does this code behave when running in window mode, which is still the
> default value for now? Don't you have to make an additional check here to
> make sure you're in tab mode to prevent failing the below code, especailly
> assigning things to tabBar?

As discussed on IRC, I tested this code by creating a new profile with my patch not applied, customized the window dialog toolbar, and then applied my patch.  The migration worked; the custom set of toolbar items was transferred to the tab toolbar.  

It works because once the patch is applied (upgrade happens) the tab toolbar is loaded into the DOM on startup via an overlay (regardless of the state of the calendar.item.editInTab pref) and is available for the migration code to edit, even though it is not visible to the user until they open a tab.

> @@ +171,5 @@
> >  
> > +            if (xulStore.hasValue(uri, "event-toolbar", "currentset")) {
> > +                let windowSet = xulStore.getValue(uri, "event-toolbar", "currentset");
> > +                let items = "calendar-item-appmenu-button";
> > +                if (!windowSet.includes("spring")) {
> 
> It's probably not enough to check for the existance of a spring but also to
> make sure it is the trailing toolbar item already.

Actually, I think a spring anywhere in the toolbar is enough; it doesn't need to be the rightmost item.  Consider, if the user has added a spring anywhere in the toolbar, to right-align one or more items, then the app menu button will also be right-aligned and an additional spring is not needed.  In that case we'd want the user's right-aligned buttons to stay right-aligned (just to the left of the app menu button) and not end up centered between two springs.  So I've left this code unchanged.

Comment 36

[Richard Marti (:Paenglab)]

Comment on attachment 8778720 [details] [diff] [review]
event-in-tab-aug7.patch

Looks good, and the CSS code is a lot simpler :)

I see one issue, but this is for a follow-up bug: In tab mode open a event or task. Choose Customize on the toolbar. Now add a new toolbar, give some name, and move a button to this toolbar. Click "Done" and close the event/task. Reopen or open a new task/event.
Automagicially two new toolbars are added to the two toolbars. Every opening of a event/task adds two toolbars to the existing ones.

Comment 37

[Paul Morris [:pmorris]]

(In reply to Stefan Sitter from comment #21)
> I don't know if event in tab editing will be supported for SeaMonkey too.
> But could you test and verify that your changes doesn't regress Lightning in
> SeaMonkey and that at least edit in dialog still works?

I've now tested editing in the window dialog in SeaMonkey and did not find any regressions.  I also briefly tried editing in a tab and did not find any issues there with a cursory check.  More thorough testing for tab editing to follow after this patch lands.

Comment 38

[Paul Morris [:pmorris]]

Attachment: event-in-tab-aug10.patch

This patch:
- fixes an issue that slipped back in with CSS changes in previous patch (the 'priority' indication in the status bar for event/task tabs was not working)
- reduces the targetOrigin arguments accepted by receiveMessage functions to the minimum
- fixes one indentation issue
- passes mozmill tests

Comment 39

[Paul Morris [:pmorris]]

Attachment: interdiff-aug10.diff

Interdiff for August 10 patch compared to previous August 7 patch.

Comment 40

[[:MakeMyDay]]

Comment on attachment 8779942 [details] [diff] [review]
event-in-tab-aug10.patch

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

Ok, this is it, r=me. Thanks for all your work.

Comment 41

[[:MakeMyDay]]

As the tree is currently closed, I'm setting checkin-needed so it can be landed once the tree opens.

Comment 42

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/341f2588de6410bb151926249dd334317785a0d7
Bug 1277972 - Let event/task dialog open in a tab. r=MakeMyDay

Comment 43

[aleth [:aleth]]

Thanks!

Comment 44

[Jorg K (CEST = GMT+2)]

Is it possible that event entry/edit is now pretty much completely broken :-(

I'm trying this in a Daily that of today, which has this function integrated. Also on a local build.

Just a few observations:
- Double click into an empty day, not equal today.
  I get offered today and the current time. Not right, I wanted the other day.
  The time used to be rounded to the next hour, now I get the exact time.
- Double click an existing event. Again, I get today and now instead of the event's
  details.
- I enter a reoccurring event. I save it. I edit it again.
  On top of the time being wrong, the recurrence also doesn't show.
- I see this annoying rotating cursor all the time.
- Sometimes, when I open a reoccurring event, I get prompted to save changes 
  although I haven't changed anything.

Can we please back this out until it's at least ready for basic usage?

Comment 45

[Jorg K (CEST = GMT+2)]

A backout would most likely also need to backout bug 1295089 first.

I think this should be backed out on Tuesday morning before the next daily is compiled.

Comment 46

[Paul Morris [:pmorris]]

Thanks for the report.  I just tested here on linux/ubuntu and everything is working fine for me.  I was not able to reproduce any of the issues you mention.  Maybe they are platform specific?  I'm fine with backing things out until we can figure out what's going on.

Comment 47

[[:MakeMyDay]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #44)
> Is it possible that event entry/edit is now pretty much completely broken :-(
> 
> I'm trying this in a Daily that of today, which has this function
> integrated. Also on a local build.
> 
> Just a few observations:
> - Double click into an empty day, not equal today.
>   I get offered today and the current time. Not right, I wanted the other
> day.
>   The time used to be rounded to the next hour, now I get the exact time.
> - Double click an existing event. Again, I get today and now instead of the
> event's
>   details.
> - I enter a reoccurring event. I save it. I edit it again.
>   On top of the time being wrong, the recurrence also doesn't show.
> - I see this annoying rotating cursor all the time.
> - Sometimes, when I open a reoccurring event, I get prompted to save changes 
>   although I haven't changed anything.

Thanks for testing. But like Paul, I cannot reproduce any of the reported issues here (Win 7), both with and without the tab feature enabled.

Also, the code to determine default values and using these for a new event hasn't been touched by this bug at all.

However, this patch is expected to disrupt calendar addons (an obvious candidate would be the ews provider, but there likely are others), so please retry with all addons but Lightning disabled. Also, please check the error console for suspicious messages and provide any further information that might be helpful to make this actionable (e.g. used platform, did you test with or without the pref enabled or didn't it work for you either way?). To do so, please file a separate bug.

> Can we please back this out until it's at least ready for basic usage?

That said, I currently see no point to back out this patch. But I'm willing to reconsider if you can provide the information requested above and it's indicating to not clearly be related to another addon.

If that does not work for you, you can workaround this by overinstalling a build of Lightning from current cycle from before the patch landed.

Comment 48

[Jorg K (CEST = GMT+2)]

Sorry for the false alarm, works fine on a new profile. However, it still doesn't work with all add-ons disabled. So something fishy is going on here. I'll have to look into it further. Maybe some installation/update issue with an existing profile?

Comment 49

[Jorg K (CEST = GMT+2)]

There is something seriously weird going on here. This is what I've done:
- removed calendar database (local.sqlite, gets recreated) to rule out a bad database.
- removed extensions folder
- copied extensions.ini from working new profile, only includes now:
[ExtensionDirs]
Extension0=C:\Program Files\Mozilla Thunderbird 51\extensions\{e2fda1a4-762b-4020-b5ad-a41df1933103}

[ThemeDirs]
Extension0=C:\Program Files\Mozilla Thunderbird 51\extensions\{972ce4c6-7e08-4474-a285-3208198ce6fd}.xpi

[MultiprocessIncompatibleExtensions]
Extension0={e2fda1a4-762b-4020-b5ad-a41df1933103}

So a pointer to the theme and the calendar coming from "Program Files" (64bit version in this case).

Error console shows nothing. But all the problems described in comment #44 persist.

What's next, MakeMyDay?

Comment 50

[Jorg K (CEST = GMT+2)]

This might be a hint:
Running a debug build on the "good" profile works, on the "production/bad" profile I get:

JavaScript error: chrome://lightning/content/lightning-item-iframe.js, line 1866: TypeError: optionsPopup is null.

Looking at the code it has to do with cloud providers which I have configured in my "production/bad" profile. Setting mail.cloud_files.enabled to false, the calendar starts to work as expected.

So there your go.