1614972 - Calendar Event: Location URLs are not clickable

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement, P1)

Description

[dave]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

Click on an event in the calendar that has a URL for the location (i.e anything that uses Zoom, Blue Jeans et. al. for video conferencing).

Actual results:

The URL displayed in the location field is not a hyperlink

Expected results:

I would like for it to behave like a hyperlink and open in a browser window when clicked.

Comment 1

[dave]

Related to 1535018 - but that is more focussed on the notifications rather than the event dialog.

Comment 2

[dave]

Attachment: bug-1614972.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9126131 [details] [diff] [review]
bug-1614972.patch

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

You'll have to ask review for the patch to move

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +134,5 @@
>  
>    let location = item.getProperty("LOCATION");
>    if (location && location.length) {
>      document.getElementById("location-row").removeAttribute("hidden");
> +    if (cal.isURL(location)) {

I think you just want to do something like
if (/http?s:\/\//.test(location))

Comment 4

[Magnus Melin [:mkmelin]]

Dave, are you able to finish off he patch?

Comment 6

[Peter Lupo]

Is it possible to identify URLs and linkify them in the description? Some software, like Microsoft Teams, add the link to the description and not to the location. This implementation solve the specific problem of not having links in the location but it doesn't address fully the need, that is to be able to quickly get to the meeting without too much hassle.
This is something that the email pane already does. Can we just reuse the same component?

Comment 7

[Magnus Melin [:mkmelin]]

Khushil, can you finish this off?

Comment 8

[Khushil Mistry [:khushil324]]

Here, Location field is input. For some attendees it will be read only field and for some, it will be editable field. So, how do we want to tackle the editable field case? I will add Paul in the discussion to finalise the feature.
Here, we can do one thing, we can add the location URL(provided it's an URL) in the Attachments. In Attachments section, when you click on the URL, you will be redirected to the browser.
We can do another thing, take both Description and Location field, whatever URL we find in them, we add those to the Attachments. I find this very interesting. What do you suggest, Paul?

Comment 9

[Nathan Neulinger]

Right now, just a simple "make the link clickable in the reminder dialog that comes up for meetings" would be a HUGE improvement. Is there anything else in that dialog that is actually editable?

I get the desire to have it linked in the description/edit dialog, but right now, the worst aspect of this is that the reminder dialogs are not clickable so you have to do the annoying extra several steps to even get to where the location url is copyable.

Comment 10

[Magnus Melin [:mkmelin]]

The reminder dialog is one place, though maybe another bug. This would be for at least when you're looking at the summary dialog (all things readonly). The summary dialog we should really show by default when clicking open an event (bug 1575195).

Comment 11

[Khushil Mistry [:khushil324]]

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

The reminder dialog is one place, though maybe another bug. This would be for at least when you're looking at the summary dialog (all things readonly). The summary dialog we should really show by default when clicking open an event (bug 1575195).

Okay, got it. We just want to do it for summary dialog right now.

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614972_location-url-clickable-calendar-summary-dialog-0.patch

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9147055 [details] [diff] [review]
Bug-1614972_location-url-clickable-calendar-summary-dialog-0.patch

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

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +137,1 @@
>    if (location && location.length) {

let's drop the location.length check, since "" will be falsy too

@@ +137,5 @@
>    if (location && location.length) {
>      document.getElementById("location-row").removeAttribute("hidden");
> +    let locationUrl = null;
> +    try {
> +      locationUrl = new URL(location);

This will allow javascript: urls and maybe some others that would be bad.

I think something like this would be better, so it also finds the url if the location is e.g. "Zoom; https://foo.bar"

let urlMatch = location.match(/(https?:\/\/[^ ]*)/);
var url = urlMatch && urlMatch[1];

Can we just make it an <a:href> if we find the link, maybe the we don't need the onclick/oncommand?

Comment 14

[Khushil Mistry [:khushil324]]

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

Can we just make it an <a:href> if we find the link, maybe the we don't need
the onclick/oncommand?

We will need onclick/oncommand just like this: https://searchfox.org/comm-central/source/mail/base/content/aboutDialog.js#135
We can use <label is="text-link"/> if we don't want to have onclick and oncommand.

Comment 15

[Khushil Mistry [:khushil324]]

(In reply to Khushil Mistry [:khushil324] from comment #14)

We can use <label is="text-link"/> if we don't want to have onclick and oncommand.

<label is="text-link"/> will be opened in the Thunderbird only. So we can't use that.

Comment 16

[Magnus Melin [:mkmelin]]

Ok.

Comment 17

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614972_location-url-clickable-calendar-summary-dialog-1.patch

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9147130 [details] [diff] [review]
Bug-1614972_location-url-clickable-calendar-summary-dialog-1.patch

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

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +139,5 @@
> +    let urlMatch = location.match(/(https?:\/\/[^ ]*)/);
> +    let url = urlMatch && urlMatch[1];
> +    let itemLocation = document.getElementById("item-location");
> +    if (url) {
> +      itemLocation.setAttribute("hidden", "hidden");

instead of hiding, you should be able to do

itemLocation.replaceWith(locationLink);

::: calendar/base/content/dialogs/calendar-summary-dialog.xhtml
@@ +190,5 @@
>              </box>
>            </html:td>
>          </html:tr>
>          <html:tr id="location-row" hidden="hidden">
>            <html:th for="item-location">

hmm, this woudln't work (didn't earlier either). for is used with a html:label
I think it should just be removed.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614972_location-url-clickable-calendar-summary-dialog-2.patch

Comment 20

[Paul Morris [:pmorris]]

Comment on attachment 9147172 [details] [diff] [review]
Bug-1614972_location-url-clickable-calendar-summary-dialog-2.patch

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

Overall, this looks good, and will be a nice improvement.  I have a couple minor comments.  I tested the patch and the links are created correctly for "http" and "https" urls.  When I clicked them, I already had Firefox open, the dialog to choose Firefox profile opened, and when I chose a profile I got a dialog saying: "Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system." This is on Ubuntu.  Not sure if that occurs on other platforms or not.  I've seen it before and don't think it's related to this patch.  Something to tackle in a follow-up bug I think.

I'm reviewing and giving my r+ since it is calendar code.  I'm also working on this summary dialog code for bug 1631902, but this will likely land first, so I'll adapt my wip changes.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +133,5 @@
>    updateLink();
>  
>    let location = item.getProperty("LOCATION");
> +
> +  if (location.length) {

This works, but could be slightly simpler as `if (location)` (because an empty string is falsy in JS).

@@ +140,5 @@
> +    let url = urlMatch && urlMatch[1];
> +    let itemLocation = document.getElementById("item-location");
> +    if (url) {
> +      let locationLink = document.createElementNS("http://www.w3.org/1999/xhtml", "a");
> +      locationLink.setAttribute("id", "item-location-href");

Hm, I wonder if it would be better to use the same "item-location" as the "id" for both linkified and non-linkified locations?  That would make it easier to get the element by id, regardless of its type.  Then we could then check for the element type or the "text-link" class to tell which type it is.  Otherwise you have to try to get the element by both ids.  Hm... yeah, to me it seems somewhat better to use the same id.  Let me know what you think, or if I'm missing something.  There are trade-offs with either approach.

Comment 21

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #20)

got a dialog saying: "Firefox is already running, but is not responding. To
open a new window, you must first close the existing Firefox process, or
restart your system."

This is because you're running a Thunderbird/Firefox with -no-remote
See bug 382477

Hm, I wonder if it would be better to use the same "item-location" as the
"id" for both linkified and non-linkified locations? That would make it
easier to get the element by id, regardless of its type.

I would keep them like in the patch. You rarely need to get this element. For css, could use a class, but... meh.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9147172 [details] [diff] [review]
Bug-1614972_location-url-clickable-calendar-summary-dialog-2.patch

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

For bonus points, we want to add also a context menu to copy the location which is now a bit more tricky. 
Like https://searchfox.org/comm-central/search?q=taskview-link-context-menu&path=

Comment 23

[Khushil Mistry [:khushil324]]

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

For bonus points, we want to add also a context menu to copy the location
which is now a bit more tricky.
Like
https://searchfox.org/comm-central/search?q=taskview-link-context-menu&path=

Nice idea, I will update the patch with this.

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614972_location-url-clickable-calendar-summary-dialog-3.patch

Comment 25

[Khushil Mistry [:khushil324]]

I have used xul:label insted of html:a as a context property only works on XUL elements.

Comment 26

[Paul Morris [:pmorris]]

Comment on attachment 9147366 [details] [diff] [review]
Bug-1614972_location-url-clickable-calendar-summary-dialog-3.patch

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

The context menu is a nice finishing touch.  Code looks good and the context menu worked when I tested it.  A couple of nits on the doc string. r=pmorris

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +538,5 @@
> +
> +/**
> + * Copy the value of the given link node to the clipboard
> + *
> + * @param {String} linkNode The node containing the value to copy to the clipboard

Nits on doc string:
- add a period (".") at end of sentences in two places (after "clipboard").
- {String} -> {string} 

('string' indicates primitive string values, "String" indicates string Objects.  Usually "string" is what you want.)

Comment 27

[Paul Morris [:pmorris]]

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

This is because you're running a Thunderbird/Firefox with -no-remote
See bug 382477

Aha! Good to know, thanks.

I would keep them like in the patch. You rarely need to get this element. For css, could use a class, but... meh.

That works for me.

Comment 28

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614972_location-url-clickable-calendar-summary-dialog-4.patch

Comment 29

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614972_location-url-clickable-calendar-summary-dialog-5.patch

Review flag updated.

Comment 30

[Paul Morris [:pmorris]]

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

Hm, I wonder if it would be better to use the same "item-location" as the
"id" for both linkified and non-linkified locations? That would make it
easier to get the element by id, regardless of its type.

I would keep them like in the patch. You rarely need to get this element. For css, could use a class, but... meh.

I was thinking about this some more. Here are 10 places where the current id "item-location" is used (or at least that string).
https://searchfox.org/comm-central/search?q=%22item-location%22&path=

Two of them are tests. So we need to check all those places where it's used and make sure using a different id for a different element, when it's a link, won't cause trouble. Maybe they are all okay, and Khushil, maybe you've already checked these? At any rate we need a try run.

Comment 31

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #30)

Two of them are tests. So we need to check all those places where it's used and make sure using a different id for a different element, when it's a link, won't cause trouble. Maybe they are all okay, and Khushil, maybe you've already checked these? At any rate we need a try run.

The summary dialog related "item-location" is not used anywhere else. Elsewhere, we are using "item-location" element from lightning-item-iframe.xhtml.
But it's good to send it for a try run. I am not on the trunk right now, so anyone else can send it for a try run?

Comment 32

[Paul Morris [:pmorris]]

Okay, thanks Khushil. I can start a try run.
Edit: here it is: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=76c74910e3921936d627eaa5f279e186edd49cd0

Comment 33

[Magnus Melin [:mkmelin]]

Sent one off already: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c5cdd664a8b56640fbba53589881c70cfed495c

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc5a341a3e9f
Fix Location URLs are not clickable in Calendar Summary Dialog. r=pmorris

Comment 35

[Magnus Melin [:mkmelin]]

Attachment: bug1614972_clickable_url_followup.patch

As Paul noticed, if you have a text with a url, all we do now is show the URL. We should show all the text.

Comment 36

[Magnus Melin [:mkmelin]]

Attachment: bug1614972_clickable_url_followup.patch

Comment 37

[Khushil Mistry [:khushil324]]

Are we taking care the case of multiple URLs now?

Comment 38

[Magnus Melin [:mkmelin]]

No, the whole location text is linked to the first URL we spot. It's quite a bit more complicated supporting many URLs, and that's basically bad data so I'm not sure we need to do that.

Comment 39

[Paul Morris [:pmorris]]

Comment on attachment 9152036 [details] [diff] [review]
bug1614972_clickable_url_followup.patch

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

Looks good overall.  It would be slightly nicer to just link-ify the URL part, but this works and fixes the issue.  The context menu item is still "Copy Link Location", which I would expect to be just the URL, but it now copies the whole 'location' text.  So I think the menu item label should change to better reflect the behavior (or alternatively just have it copy the URL).

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +211,5 @@
>    padding-inline-start: 4px;
>  }
>  
> +.item-location-link {
> +  padding-inline-start: 0;

While we're here, it might be nice to have the cursor change on hover, like it does for other links on web pages etc.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/00e93f6c59c3
make event URLs clickable - make sure all the text from the location is visible, not only the location. r=pmorris