ldubois Reporter
	Description • 8 years ago

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.

	[:MakeMyDay] Assignee
	Comment 1 • 8 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 2 • 8 years ago

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.

	informatique
	Comment 3 • 8 years ago

Thunderbird

	informatique
	Comment 4 • 8 years ago

sogo web interface exemple

	informatique
	Comment 5 • 8 years ago

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?

	[:MakeMyDay] Assignee
	Comment 6 • 8 years ago

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.

	[:MakeMyDay] Assignee
	Comment 7 • 8 years ago

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.

	Richard Marti (:Paenglab)
	Comment 8 • 8 years ago

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.

	informatique
	Comment 9 • 8 years ago

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?

	Richard Marti (:Paenglab)
	Comment 10 • 8 years ago

(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:".

	Patrick Cloke [:clokep]
	Comment 11 • 8 years ago

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.

	[:MakeMyDay] Assignee
	Comment 12 • 8 years ago

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.

	Patrick Cloke [:clokep]
	Comment 13 • 8 years ago

(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.

	Richard Marti (:Paenglab)
	Comment 14 • 8 years ago

(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.

	[:MakeMyDay] Assignee
	Comment 15 • 8 years ago

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.

	[:MakeMyDay] Assignee
	Comment 16 • 8 years ago

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.

	Richard Marti (:Paenglab)
	Comment 17 • 8 years ago

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.

	Richard Marti (:Paenglab)
	Comment 18 • 8 years ago

Comment on attachment 8800779 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V2.diff

ui-r- because of comment 17.

	Patrick Cloke [:clokep]
	Comment 19 • 8 years ago

(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. :)

	[:MakeMyDay] Assignee
	Comment 20 • 8 years ago

Updated patch with respect to comment 17.

	Richard Marti (:Paenglab)
	Comment 21 • 8 years ago

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.

	[:MakeMyDay] Assignee
	Comment 22 • 8 years ago

> 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.

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 24 • 8 years ago

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?

	[:MakeMyDay] Assignee
	Comment 25 • 8 years ago

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.

	[:MakeMyDay] Assignee
	Comment 26 • 8 years ago

https://hg.mozilla.org/comm-central/rev/fcaede61f5c17597a55035feadc6beb7438f388d

	[:MakeMyDay] Assignee
	Comment 27 • 8 years ago

Comment on attachment 8801506 [details] [diff] [review]
DisplayUriAttachementsInSummaryDialog-V4.diff

a+ from Philipp on irc

	Jorg K (CEST = GMT+2)
	Comment 28 • 8 years ago

Aurora (Thunderbird 52 / Lightning 5.4)
https://hg.mozilla.org/releases/comm-aurora/rev/b746917d68d5