1229148 - Use the new itip icons also in the event dialog

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[[:MakeMyDay]]

Attachment: event-attendees.png

Basically the same bug 1229141 but for the event dialog. This is a little more sophisticated as the existing dialog has a different concept to provide information about the attendees, which is poor esp. if you have larger meetings to schedule.

Therefore, I am experimenting with a another approach modifying the previous concept by adding a tab section at the lower part of the dialog to provide enough space for all of the information but not to clutter the dialog to much. This can be used to better display attachments as well.

I have not yet a patch for this but the screenshots may illustrate it. Feedback welcome ;-)

Comment 1

[[:MakeMyDay]]

Attachment: event-decription.png

Comment 2

[[:MakeMyDay]]

Attachment: event-decription.png

Comment 3

[Richard Marti (:Paenglab)]

I like the approach with the tabs to unclutter the dialog.

You added two times the event-description image. Do you have a attachment image?

About attachments, will this still be only URLs or are also files allowed? If yes, would a drag and drop into this field be allowed (in a other bug)? If not, what abut a filelink approach, where it is again only a URL in the dialog but with the ability to download the file?

Comment 4

[[:MakeMyDay]]

Attachment: event-attachments.png

Sure, here we go.

This is just about changing the UI but not the functionality, so as long the file attachment support is not implemented, there would only be links like today.

An interesting question is how to deal with the tab approach, if there yet are neither attendees nor attachments.

- Fall back to the existing layout (probably a little bit odd)
- hide the not needed tabs or
- disable the same.


Also, is it ok / preferred just to have the context menus for actions within a tab or would separate buttons be preferrable (like e.g. 'add attendee')?

Comment 5

[Richard Marti (:Paenglab)]

I would show them all the time when where is the possibility to add attendees/attachments. Buttons to add/delete would be better because not all user know the right click possibility. But add also the right click menus.

Comment 6

[[:MakeMyDay]]

Attachment: UseNewItipIconsInEventDialog-v1.diff

WIP patch. It required patches from bug 1229141 and bug 1197623 to work.

- there are still some minor UI nits like some colors and maybe location of css code
- the buttons in attachment and attendee tabpanel are not fully working and partially just mockup, so we would need to agree what we want to have available by button

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 8696233 [details] [diff] [review]
UseNewItipIconsInEventDialog-v1.diff

This looks already good.

I had already noted in IRC that in attachment tab clicking in the attachment list a add link dialog opens. This is a bit unexpected as this doesn't happen in other such lists.
"Edit" doesn't work, but you wrote, not all is working. But this is needed because double-click opens the link. I would rather expect the editing of the link.

In Attendee tab it's not possible to select one attendee and then delete it with the button on the right. Clicking a attendee opens the Invite Attendee dialog.
Also the attendee list is not accessible through keyboard. Tab jumps directly to the buttons.

Comment 8

[[:MakeMyDay]]

Thank you for your feedback.

(In reply to Richard Marti (:Paenglab) from comment #7)
>
> I had already noted in IRC that in attachment tab clicking in the attachment
> list a add link dialog opens. This is a bit unexpected as this doesn't
> happen in other such lists.

With the add button in the dialog this can be easily adapted.

> "Edit" doesn't work, but you wrote, not all is working. But this is needed
> because double-click opens the link. I would rather expect the editing of
> the link.

Currently no edit functionality exists and this maybe for a reason, as this is used for file link modifying the link may just abandon the file in the cloud. But I'm not sure about this. So probably it is the best to just remove the edit button at all.

> In Attendee tab it's not possible to select one attendee and then delete it
> with the button on the right. Clicking a attendee opens the Invite Attendee
> dialog.

This is not trivial as the box is not a listbox anymore and the label element used to display the attendee name cannot receive a focus. One can mock this behaviour visually, but this is not enough to deal with the remove button. In the worst case, the remove button for a single attendee should be avoided here. Noteworthy: in the current releases there is no option to remove attendees directly from within that dialog.

> Also the attendee list is not accessible through keyboard. Tab jumps
> directly to the buttons.

Yes, iirc this is a known issue (although I currently have no bug at hand) - there are other accesskeys which also seem not behaving correctly.

Comment 9

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

I like the idea of using tabs to display the description/attendees/attachments. I haven't applied the patch, so my comments are based on the screenshots. Please excuse if I missed something.

I was about to comment that I'd also prefer a button for adding attendees and attachments, but then again there is already the toolbar button in for both of these actions (given they are visible by default).

I think that attendees should be selectable as in a list box, in the future we could even add functionality to email only the selected attendee. If you can't use a listbox, you can make things selectable with -moz-user-focus. Just clicking the boxes shoudn't open any dialogs or edit attendees/attachments, I think this should be done by the toolbar button or context menu.

Hope I answered all needinfo questions, let me know if I missed something.

Comment 10

[[:MakeMyDay]]

Attachment: UseNewItipIconsInEventDialog-v2.diff

Ok. This patch removes the buttons again. There is also no option to edit a link as this cannot be implemented without the risk of data loss in case of file link usage. For the edit link use case the user would have to remove and re-add the link (at least for now).

The selecting items is implemented as well as two additional context menu options to remove attendees. To open an attachment or edit an attendee now a double click is required, while clicking on not used areas of attendee and attachment box doesn't trigger anything.

The accesskey situation is quite a mess, but this is not related to this bug. As this is localization specific, we can optimize this still after string freeze for en-US.

Comment 11

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

Comment on attachment 8697811 [details] [diff] [review]
UseNewItipIconsInEventDialog-v2.diff

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

r=philipp with the following comments:

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +530,5 @@
>              inRow = determineAttendeesInRow();
>              window.attendeesInRow = inRow;
>          } else {
> +            while(attBoxRows.length > 0) {
> +                attBox.removeChild(attBoxRows[0]);

Do we have calendar-ui-utils included here? in that case you could use removeChildrenhttp://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-ui-utils.js#221

@@ +615,5 @@
>          }
> +    } else {
> +        while(attBoxRows.length > 0) {
> +            attBox.removeChild(attBoxRows[0]);
> +        }

Same here

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +2309,5 @@
>   * @param event     The DOM event caused by the clicking.
>   */
>  function attachmentLinkClicked(event) {
> +    // we open the attachment on double click
> +    if (event.button == 0 && event.detail == 2 && event.originalTarget.localName == "listitem") {

You could just change the handler to use ondblclick instead of onclick

@@ +3457,5 @@
> +    let attendeeTab = document.getElementById("event-grid-tab-attendees");
> +    let attendeePanel = document.getElementById("event-grid-tabpanel-attendees");
> +    if (!isEvent(window.calendarItem)) {
> +        attendeeTab.setAttribute('collapsed', 'true');
> +        attendeePanel.setAttribute('collapsed', 'true');

Double quotes instead of single quotes for consistency

@@ +3466,5 @@
> +        if (window.organizer && window.organizer.id) {
> +            document.getElementById("item-organizer-row").removeAttribute("collapsed");
> +            let cell = document.getElementsByClassName("item-organizer-cell")[0];
> +            let icon = cell.getElementsByTagName("img")[0];
> +            let text = cell.getElementsByTagName("label")[0];

Consider using querySelector here and/or giving icon and text a class in the DOM.

@@ +3489,2 @@
>          } else {
> +            document.getElementById("item-organizer-row").setAttribute("collapsed", "true");

you could use setBooleanAttribute("item-organizer-row", "collapsed", true); here

@@ +3593,5 @@
> +    let popup = document.getElementById("attendee-popup");
> +    let mailto = document.getElementById("attendee-popup-emailattendee-menuitem");
> +    let remove = document.getElementById("attendee-popup-removeattendee-menuitem");
> +    let attId = event.target.parentNode.getAttribute("attendeeid");
> +    let attendee = window.attendees.filter(aAtt => aAtt.id == attId)[0];

let attendee = window.attendees.find(aAtt => aAtt.id == attId);

@@ +3611,5 @@
>      }
>  
>      // Show the popup.
>      var attendeeList = document.getElementById("attendee-list");
> +    popup.openPopup(attendeeList, "after_start", event.clientX, event.clientY, true);

Does the positioning work as expected here? If you use the anchored popup, then the positioning arguments are relative to the anchor, see MDN for details. Also, since you have it here, you can specify the triggerEvent parameter.

::: calendar/base/content/dialogs/calendar-event-dialog.xul
@@ +1182,5 @@
> +                           rows="12"/>
> +                </tabpanel>
> +                <tabpanel id="event-grid-tabpanel-attachements">
> +                    <hbox flex="1">
> +                      <vbox flex="1">

Why do you need the hbox/vbox here? It looks like you can position this with pack/align.

@@ +1206,5 @@
> +                        <img class="itip-icon"/>
> +                        <label id="item-organizer"
> +                               class="item-attendees-cell-label"
> +                               crop="right"/>
> +                        <spacer flex="1"/>

I don't think you need a spacer here, you can use the pack or align attribute, I always forget which one. I think pack="start" will do it.

@@ +1382,5 @@
>      </popupset>
> +
> +    <!-- attendee box template -->
> +    <vbox id="item-attendees-box-template"
> +          collapsed="true">

Consider using hidden="true" instead. While the nodes are hidden in both cases, the sizes are still calculated when collapsed=true is used, because it uses CSS visibility: collapse. hidden=true uses CSS display:none.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 8697811 [details] [diff] [review]
UseNewItipIconsInEventDialog-v2.diff

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

ui-r+ with looking at my comments

::: calendar/base/themes/common/calendar-attendees.css
@@ +23,2 @@
>      padding: 0px 3px;
>      max-height: 18px;

Could you remove this line? On Win7 this makes the itip-icon crop on bottom (15px height). On Windows this row has a height of 22-23px.

@@ +33,5 @@
> +}
> +
> +#calendar-event-dialog .item-attendees-cell:focus {
> +    background-color: -moz-cellhighlight;
> +    color: -moz-cellhighlighttext;

At least on Windows Highlight and HighlightText would be better because the actual colors are almost not remarkable.

I could also in a followup bug style them like the other listbox items.

Comment 13

[[:MakeMyDay]]

calendar-ui-utils.js seems not to be available. Positioning of the context menu worked as expected, so I haven't changed anything regarding that comments. All other comments above from review and ui-review are considered. Thanks for the short term review!

Paenglab, feel free to do further fine tuning if required.

https://hg.mozilla.org/comm-central/rev/95367f284f9e

Comment 14

[Decathlon]

A little bit late, anyway I tested the nightly and I think that this is a good way to display all the informations related to the attendees and their status (in particular when there are many) without enlarge excessively the dialog.

My notes are just about the style of the tabs that maybe should be the same of the tabs in Thunderbird's dialogs (e.g. New contact or Options dialogs) i.e. with the white background in all the tab and not only in the active element inside.
The top border in the Attendee tab should be increased a few pixels when the attendees list is empty to make it equal to the others two tabs.

Not sure about this, but should the selected tab made persistent in order to make appear the last selected tab every time the dialog is opened?

Another note is about the possibility to open the Invite Attendees dialog from the tabs when the list is empty. I think that an easy way to open the dialog directly from the tab would be useful because now an element related to the attendees is always visible (the tab), instead the button in the toolbar could be absent (toolbar customized or removed). It could be a simple double click in the empty area or a context menu with a single item or something else maybe less bulky then a button.

Comment 15

[[:MakeMyDay]]

(In reply to Decathlon from comment #14)
> A little bit late, anyway I tested the nightly and I think that this is a
> good way to display all the informations related to the attendees and their
> status (in particular when there are many) without enlarge excessively the
> dialog.

Thanks for your feedback.

> My notes are just about the style of the tabs that maybe should be the same
> of the tabs in Thunderbird's dialogs (e.g. New contact or Options dialogs)
> i.e. with the white background in all the tab and not only in the active
> element inside.
> The top border in the Attendee tab should be increased a few pixels when the
> attendees list is empty to make it equal to the others two tabs.

Personally, I don't like the white tab background. But I leave the css fine tuning to Paenglab.

> Not sure about this, but should the selected tab made persistent in order to
> make appear the last selected tab every time the dialog is opened?

I don't think we should do this. I think the description is the obvious choice when opening the dialog. I was thinking about to switch to the attendee tab when returning from the attendee dialog to make visible that the attendees are added, but I discarded this.

Persisting may lead to odd results. If you had displayed e.g. the attachment tab before closing an event with attachments and then create a new event, you would get displayed the attachment box tab again, even if that is is probably the less probable tab you want to use in the first place. 

> Another note is about the possibility to open the Invite Attendees dialog
> from the tabs when the list is empty. I think that an easy way to open the
> dialog directly from the tab would be useful because now an element related
> to the attendees is always visible (the tab), instead the button in the
> toolbar could be absent (toolbar customized or removed). It could be a
> simple double click in the empty area or a context menu with a single item
> or something else maybe less bulky then a button.

I filed bug 1233827 to modify the context menu.