Closed Bug 1614972 Opened 11 months ago Closed 8 months ago

Calendar Event: Location URLs are not clickable

Categories

(Calendar :: Dialogs, enhancement, P1)

Product:
Calendar
Component:
Dialogs
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 78.0

People

(Reporter: dave, Assigned: khushil324)

Details

Attachments

(2 files, 7 obsolete files)

Bug-1614972_location-url-clickable-calendar-summary-dialog-5.patch
6.48 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
bug1614972_clickable_url_followup.patch
4.02 KB, patch
pmorris
: review+
Details | Diff | Splinter Review

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.

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

Attached patch bug-1614972.patch (obsolete) — — Splinter Review 
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))
Assignee: nobody → dave
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Dave, are you able to finish off he patch?

Flags: needinfo?(dave)
Priority: -- → P1
Duplicate of this bug: 1535018

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?

Khushil, can you finish this off?

Assignee: dave → 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?

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.

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

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

Attachment #9126131 - Attachment is obsolete: true
Attachment #9147055 - Flags: review?(mkmelin+mozilla)
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?
Attachment #9147055 - Flags: review?(mkmelin+mozilla) → review-

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

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

Ok.

Attachment #9147055 - Attachment is obsolete: true
Attachment #9147130 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9147130 - Flags: review?(mkmelin+mozilla)
Attachment #9147130 - Attachment is obsolete: true
Attachment #9147172 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9147172 - Flags: review?(mkmelin+mozilla) → review+

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

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

Attachment #9147172 - Attachment is obsolete: true
Attachment #9147366 - Flags: review?(paul)

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

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.)
Attachment #9147366 - Flags: review?(paul) → review+

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

Attachment #9147366 - Attachment is obsolete: true
Attachment #9147376 - Flags: review+
Keywords: checkin-needed-tb

Review flag updated.

Attachment #9147376 - Attachment is obsolete: true
Attachment #9147379 - Flags: review+

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

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

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/fc5a341a3e9f
Fix Location URLs are not clickable in Calendar Summary Dialog. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Target Milestone: --- → 78

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.

Attachment #9152033 - Flags: review?(paul)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #9152033 - Attachment is obsolete: true
Attachment #9152033 - Flags: review?(paul)
Attachment #9152036 - Flags: review?(paul)

Are we taking care the case of multiple URLs now?

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 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.
Attachment #9152036 - Flags: review?(paul) → review+

Pushed by mkmelin@iki.fi:
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

Status: REOPENED → RESOLVED
Closed: 8 months ago → 8 months ago
Resolution: --- → FIXED
Flags: needinfo?(dave)
You need to log in before you can comment on or make changes to this bug.