Closed Bug 1273500 Opened 5 years ago Closed 4 years ago

Attachments are not displayed in the event summary dialog

Categories

(Calendar :: Dialogs, defect)

Lightning 4.7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ldu.linagora, Assigned: MakeMyDay)

References

(Depends on 1 open bug)

Details

(Whiteboard: [event summary dialog][late-l10n])

Attachments

(5 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20160420141331

Steps to reproduce:

UserA and userB are both on the same caldav server,

1. UserA creates an event and attach an URL : http://mozilla.org
2. UserA invites userB in this event
3. UserB receives the invitation and opens it in the calendar grid


Actual results:

The dialog don't display any information about the attachment.
The URL is inside the received ICS

BEGIN:VEVENT
CREATED:20160517T131717Z
LAST-MODIFIED:20160517T131717Z
DTSTAMP:20160517T131720Z
UID:5db65ff9-3d0d-4311-92c5-f2d3a30ad39a
SUMMARY:urlfora
ORGANIZER;RSVP=TRUE;PARTSTAT=ACCEPTED;ROLE=CHAIR:mailto:user.a@example.com
ATTENDEE;RSVP=TRUE;CN=A User DGAC/SSIM;PARTSTAT=NEEDS-ACTION;ROLE=REQ-PART
 ICIPANT:mailto:user.b@example.com
ATTACH:http://mozilla.org/
DTSTART;TZID=Europe/Paris:20160521T160000
DTEND;TZID=Europe/Paris:20160521T170000
X-APPLE-NEEDS-REPLY:TRUE
END:VEVENT


Expected results:

The dialog should allow the attendees to see the attachment.

When the organizer userA opens this even, the URL is displayed correctly.
Yes. ATTACH property is currently not supported in the summary dialog, only URL is. However, URL cannot be set actively in the event dialog, so this is not a consistent behaviour.
Status: UNCONFIRMED → NEW
Component: Calendar Views → Dialogs
Ever confirmed: true
Whiteboard: [event summary dialog]
Hi Laurent, nice to see you filing some bugs! Hope you are doing well!

Indeed ATTACH is not supported in that dialog, it shouldn't be too hard to implement though. There is a student working on a new event dialog in a tab, maybe we can make the new dialog double as the summary dialog to avoid such inconsistencies in the future.

This could certainly be fixed in the old dialog as well by copying the UI and maybe applying some stylesheets, but for now I'm making this a dependency.
Depends on: event-in-a-tab
Summary: Attachments are not displays for the attendees → Attachments are not displayed in the event summary dialog
Depends on: 1294924
No longer depends on: event-in-a-tab
Attached image Capture1.PNG
Thunderbird
sogo web interface exemple
hello, I added two attachments such example. I have the same problem, you have a solution for the user receiving an invitation can see the piece attached to the event? a patch?
As its currently more likely that the summary dialog will survive for the next esr cycle, here's a patch for it.

I'm not sure whether we want to follow the approach to assign the filelink provider icons here as the detection is quite weak.

But maybe there's a better way to do a reverse lookup whether a filelink is pointing to a specific provider.
@clokep: you've been dealing with filelink iirc, any ideas on that?

The second issue with this patch is that the icons are currently not displayed.
@paenglab: any idea?

I still upload an appropriate ics to test this patch.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8800395 - Flags: feedback?(richard.marti)
Attachment #8800395 - Flags: feedback?(clokep)
Attached file attachment-test.ics
Just open that ICS from within Lightning - that will create a separate calendar with one event dated Oct 13. Make that calendar read-only and you're ready to test-drive the patch.
Comment on attachment 8800395 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V1.diff

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

::: calendar/base/content/dialogs/calendar-summary-dialog.xul
@@ +240,5 @@
> +              <!-- attachment box template -->
> +              <hbox id="attachment-template"
> +                    hidden="true"
> +                    disable-on-readonly="true">
> +                <img class="attachment-icon"/>

Change this to
<image class="attachment-icon"/>
and the corresponding "img" to "image" in calendar-summary-dialog.js and the icons are shown.

A align="center" on the parent hbox makes the icon appear in the correct dimension.
Attachment #8800395 - Flags: feedback?(richard.marti) → feedback+
i have one error : 

Erreur d'analyse XML : entité non définie
Emplacement : chrome://calendar/content/calendar-summary-dialog.xul
Numéro de ligne 237, Colonne 13 :            <label value="&read.only.attachments.label;"
------------^
have you an idea?
(In reply to informatique from comment #9)
> i have one error : 
> 
> Erreur d'analyse XML : entité non définie
> Emplacement : chrome://calendar/content/calendar-summary-dialog.xul
> Numéro de ligne 237, Colonne 13 :            <label
> value="&read.only.attachments.label;"
> ------------^
> have you an idea?

Have you also patched calendar-event-dialog.dtd with the new entity? You could also exchange value="&read.only.attachments.label;" with value="Attachments:".
Comment on attachment 8800395 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V1.diff

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

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +197,5 @@
> +                let iconSrc = aAttachment.uri.spec.length ? aAttachment.uri.spec : "dummy.html";
> +                icon.setAttribute("src", "moz-icon://" + iconSrc);
> +                // we may want to override the default icon with a cloud provider specific icon
> +                // if the user has installed the respective provider addons
> +                let supportsCloudFiles = false;

Would it make more sense set this as a constant inside the try/catch where you import cloudFileAccounts?

@@ +210,5 @@
> +                    for (let [key, provider] of cloudFileAccounts.enumerateProviders()) {
> +                        if (provider.serviceURL) {
> +                            let providerUrl = Services.io.newURI(provider.serviceURL, null, null);
> +                            if (aAttachment.uri.asciiHost && providerUrl.asciiHost &&
> +                                aAttachment.uri.asciiHost == providerUrl.asciiHost) {

This seems fragile-ish, but should work for some of our providers. Was this code taken from somewhere else? Seems like it could be a nice utility to add onto cloudFileAccounts itself, something like:
let provider = cloudFileAccounts.matchProvider(uri)
if (provider && provider.iconClass) ...

Looking at the providers, I think this will even work for the WebDAV provider, which is the one that concerned me.

@@ +225,5 @@
> +                }
> +                document.getElementById("item-attachement-cell").appendChild(attachment);
> +                attCounter++;
> +            }
> +            

Nit: Trailing whitespace.
Attachment #8800395 - Flags: feedback?(clokep) → feedback+
Thanks for your feedback.

(In reply to Richard Marti (:Paenglab) from comment #8)
> Change this to
> <image class="attachment-icon"/>
> and the corresponding "img" to "image" in calendar-summary-dialog.js and the
> icons are shown.
> 
> A align="center" on the parent hbox makes the icon appear in the correct
> dimension.

Funny thing - I just used the organizer row in the same file as a blueprint. But probably the img works there as all is implemented in css. (I won't correct that with this patch as it a. works and b. I have to add some further xul/css optimizations for aligning properly already and don't want this patch to be too regression-prone.)


(In reply to Patrick Cloke [:clokep] from comment #11)

> This seems fragile-ish, but should work for some of our providers.

That's why I was asking whether you have a better idea for this. ;-)

> Was this
> code taken from somewhere else? Seems like it could be a nice utility to add
> onto cloudFileAccounts itself, something like:
> let provider = cloudFileAccounts.matchProvider(uri)
> if (provider && provider.iconClass) ...

No, this isn't copied code. If you think it makes sense, I could extend cloudFileAccounts.js to make it further available.

> Looking at the providers, I think this will even work for the WebDAV
> provider, which is the one that concerned me.

The WebDAV provider does not have a serviceUrl defined, at least as long you haven't configured anything - so I don't think it will work for that in every case.

The downside of displaying filelink provider icons at all is that this would replace the file type icon detected beforehand. The question is, what is more relevant for the user, the source of the attachment or its type. As both are derived from the url, they are available anyway at least for a capable user, so it's a dead heat for me.
(In reply to [:MakeMyDay] from comment #12)
> (In reply to Patrick Cloke [:clokep] from comment #11)
> 
> > This seems fragile-ish, but should work for some of our providers.
> 
> That's why I was asking whether you have a better idea for this. ;-)
> 
> > Was this
> > code taken from somewhere else? Seems like it could be a nice utility to add
> > onto cloudFileAccounts itself, something like:
> > let provider = cloudFileAccounts.matchProvider(uri)
> > if (provider && provider.iconClass) ...
> 
> No, this isn't copied code. If you think it makes sense, I could extend
> cloudFileAccounts.js to make it further available.

I think it makes more sense in there, yes. Don't worry too much about generalizing it, but it will be nice for other people to know this exists.

> The downside of displaying filelink provider icons at all is that this would
> replace the file type icon detected beforehand. The question is, what is
> more relevant for the user, the source of the attachment or its type. As
> both are derived from the url, they are available anyway at least for a
> capable user, so it's a dead heat for me.

I'm unsure which is more "useful", I think this is a good question for Paenglab.
(In reply to Patrick Cloke [:clokep] from comment #13)
> (In reply to [:MakeMyDay] from comment #12)
> > The downside of displaying filelink provider icons at all is that this would
> > replace the file type icon detected beforehand. The question is, what is
> > more relevant for the user, the source of the attachment or its type. As
> > both are derived from the url, they are available anyway at least for a
> > capable user, so it's a dead heat for me.
> 
> I'm unsure which is more "useful", I think this is a good question for
> Paenglab.

I think it's more "useful" for the user what type the attachment is than where it is stored.
Ok, in that case also don't need the extension of cloudFileAccounts.js - at least not for this bug. Patrick, if you think this would be useful by itself, we can implement that in a separate bug, then.
Updated patch without the filelink provider icons but including also the follow-up activities from bug 1297449 regarding including the css file properly and taking care of the resulting regressions - therefor also ui-r? to make sure I have catched everything.

As mentioned in comment 12, I leave the img tags used for the attendee icon as is.
Attachment #8800395 - Attachment is obsolete: true
Attachment #8800779 - Flags: ui-review?(richard.marti)
Attachment #8800779 - Flags: review?(philipp)
All in all it looks good. I found two issues:
- the attachments aren't well aligned with the row above. 6px before the icons and 3px between the icons and the links would do the thing.
- in the attendees box the icon touches the box. Removing from .item-attendees-cell the 0px padding and giving on the left and right 2px would fix this.
Comment on attachment 8800779 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V2.diff

ui-r- because of comment 17.
Attachment #8800779 - Flags: ui-review?(richard.marti) → ui-review-
(In reply to [:MakeMyDay] from comment #15)
> Ok, in that case also don't need the extension of cloudFileAccounts.js - at
> least not for this bug. Patrick, if you think this would be useful by
> itself, we can implement that in a separate bug, then.

Not unless we're going to use it! Thanks. :)
Updated patch with respect to comment 17.
Attachment #8800779 - Attachment is obsolete: true
Attachment #8800779 - Flags: review?(philipp)
Attachment #8800858 - Flags: ui-review?(richard.marti)
Comment on attachment 8800858 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V3.diff

The attendees icon still touches the box border. But this was already before this patch. If you could fix it, great. If not, I can do it in a new bug.
Attachment #8800858 - Flags: ui-review?(richard.marti) → ui-review+
> The attendees icon still touches the box border. But this was already before
> this patch. If you could fix it, great. If not, I can do it in a new bug.

Ups, seems like I forgot to refresh before uploading.
Attachment #8800858 - Attachment is obsolete: true
Attachment #8801506 - Flags: review?(philipp)
Blocks: ltn54
Whiteboard: [event summary dialog] → [event summary dialog][l10n impact]
Duplicate of this bug: 1303622
Comment on attachment 8801506 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V4.diff

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

r=philipp

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +382,5 @@
>  }
> +
> +/**
> + * Opens an attachment
> + * 

Whitespace here. Can you check the linter?
Attachment #8801506 - Flags: review?(philipp) → review+
The linter is broken in my setup, so I would appreciate if you can do and post any findings.

Unfortunately, this didn't land in time for the merge, so I'm tagging this a l10n candidate.
Whiteboard: [event summary dialog][l10n impact] → [event summary dialog][late-l10n]
Attachment #8801506 - Flags: approval-calendar-aurora?(philipp)
https://hg.mozilla.org/comm-central/rev/fcaede61f5c17597a55035feadc6beb7438f388d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.5
Comment on attachment 8801506 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V4.diff

a+ from Philipp on irc
Attachment #8801506 - Flags: approval-calendar-aurora?(philipp) → approval-calendar-aurora+
Aurora (Thunderbird 52 / Lightning 5.4)
https://hg.mozilla.org/releases/comm-aurora/rev/b746917d68d5
Target Milestone: 5.5 → 5.4
Duplicate of this bug: 471967
Duplicate of this bug: 689174
You need to log in before you can comment on or make changes to this bug.