URLs entered in the task description should be clickable and open a browser

ASSIGNED
Assigned to

Status

--
enhancement
ASSIGNED
9 years ago
3 years ago

People

(Reporter: carleeto, Assigned: manu.jain13)

Tracking

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20100111 Lightning/1.0b1 Thunderbird/3.0.1

If you enter URLs in a task's description, they are not clickable and they should be. 

Reproducible: Always

Steps to Reproduce:
1.Create a new task
2.Add information to it in the description, with one or more URLs.
3.You cannot click on the URL to go to the page
Actual Results:  
The URL appears as plain text.

Expected Results:  
The URL appears as a link that can be clicked on. 

I know a URL can be attached to the task. The problem is that the attachment does not contextual information describing the URL. That might be ok for people used to tools like bugzilla, but I doubt that the majority of users are. 

These days, one simply expects to be able to click on links.
Severity: normal → enhancement
OS: Windows XP → All
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: URLs entered in the task description cannot be clicked on to go to the page → URLs entered in the task description should be clickable and open a browser

Comment 1

7 years ago
I'm not familiar with calenders code, but i guess it should be fixed in there:

http://hg.mozilla.org/comm-central/file/84961292898a/calendar/base/content/calendar-task-view.js

Comment 2

6 years ago
More than just the description field should autolinkify. Consider urls entered for event locations.

Comment 3

6 years ago
Cross-reference: bug 734072.
(Assignee)

Comment 4

4 years ago
Just want to know that if someone is still working on the bug or not? If not then I'll like to take this bug up.

Comment 5

4 years ago
I always wanted to find some time to play with this, but haven't gotten myself around to it (always something else). I'm happy to test (Linux, Windows, OS/2), if you're willing to code, Manu!
Feel free to work on this bug :-) Let me know if you have questions!
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
(Assignee)

Comment 7

4 years ago
My idea in this bug is to use regular expressions to detect URL in the task description and then return that URL as a href link. Is it a good approach or do I need to do something different?

Comment 8

4 years ago
(In reply to Manu Jain from comment #7)
> My idea in this bug is to use regular expressions to detect URL in the task
> description and then return that URL as a href link. Is it a good approach
> or do I need to do something different?

Thunderbird is already doing this for plain-text emails, thus there must be code somewhere to do precisely this.

The add-on Thunderbird Conversations (https://github.com/protz/GMail-Conversation-View/) is also doing it, and I guess the code must be somewhere here: https://github.com/protz/GMail-Conversation-View/blob/master/modules/message.js You could always ask the author.
(Assignee)

Comment 9

4 years ago
Created attachment 8564599 [details] [diff] [review]
bug_553132.patch

Patch attached.
Attachment #8564599 - Flags: review?(matthew.mecca)
Comment on attachment 8564599 [details] [diff] [review]
bug_553132.patch

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

Codewise this looks fine with a few minor comments, but we'll need to give it a test run. Matt, if you get to that before me feel free. Manu, do you think you could upload a new patch with these comments fixed?

::: calendar/base/content/calendar-task-view.js
@@ +121,5 @@
>                      rpv.value = detailsString.split("\n").join(" ");
>                  }
>              }
> +            var texttohtml = Components.classes["@mozilla.org/txttohtmlconv;1"]
> +                    .getService(Components.interfaces.mozITXTToHTMLConv);

Just a minor indentation issue here, make sure you are using spaces not tabs and align the . before getService with the . after Components. w

@@ +122,5 @@
>                  }
>              }
> +            var texttohtml = Components.classes["@mozilla.org/txttohtmlconv;1"]
> +                    .getService(Components.interfaces.mozITXTToHTMLConv);
> +            var textbox = document.getElementById("calendar-task-details-description").contentDocument;

Its not a textbox anymore, do you think you could rename this to descDocument ? Also, please use let instead of var, for all variables you've added and maybe also the remaining ones just around the code you've changed, i.e. |description|.

::: calendar/base/content/calendar-task-view.xul
@@ +221,5 @@
>                  </row>
>                </rows>
>              </grid>
>            </hbox>
> +          <iframe src="about:blank" id="calendar-task-details-description" multiline="true" flex="1"/>

You should add type="content" here to make sure the content cannot access privileged apis.
Attachment #8564599 - Flags: review?(matthew.mecca) → feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 8564604 [details] [diff] [review]
bug_553132_v2.patch

changes made.
Attachment #8564599 - Attachment is obsolete: true
Attachment #8564604 - Flags: review?(matthew.mecca)
Comment on attachment 8564604 [details] [diff] [review]
bug_553132_v2.patch

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

r- on this version of the patch, overall it's looking good, but has a few issues:

- Selecting a task without a Description set throws an error, and the description frame doesn't update from the value for the previously selected task.

- Clicking on a link loads the web page in the description frame rather than opening it in a new browser window.
Attachment #8564604 - Flags: review?(matthew.mecca)
Attachment #8564604 - Flags: review-
Attachment #8564604 - Flags: feedback+
(Assignee)

Comment 13

4 years ago
I have corrected the first issue.
But I have used iframe in the xul file of task-view and iframe opens the link in that frame itself. I have used iframe because textbox (which is earlier used) doesn't support href links. So, can you tell how to open those links on a separate browser?
Flags: needinfo?(matthew.mecca)
(Assignee)

Comment 14

4 years ago
Created attachment 8565926 [details] [diff] [review]
bug_553132_v3.patch

First issue fixed.
In second issue rather than opening in new window, its opening in new tab of that window only. I think that is fine. Please take a look.
Attachment #8564604 - Attachment is obsolete: true
Flags: needinfo?(matthew.mecca)
Attachment #8565926 - Flags: review?(matthew.mecca)
(In reply to Manu Jain from comment #14)
> Created attachment 8565926 [details] [diff] [review]
> bug_553132_v3.patch
> 
> First issue fixed.
> In second issue rather than opening in new window, its opening in new tab of
> that window only. I think that is fine. Please take a look.

I think it would be more consistant with how Thunderbird handles linkified emails if they opened in a browser window rather than a tab. 

You should be able to use the same function as the message pane uses [1]. You'll need to add a line to the scripts section of calendar-task-view.xul:

<script type="application/javascript" src="chrome://communicator/content/contentAreaClick.js"/>

Then in your iframe tag you can add:

onclick="return contentAreaClick(event);"



[1] http://mxr.mozilla.org/comm-central/source/mail/base/content/messenger.xul#583
And please don't forget to test and verify the feature in SeaMonkey too because you don't want to break Lightning for all SeaMonkey users.
(Assignee)

Comment 17

4 years ago
Created attachment 8569048 [details] [diff] [review]
bug_553132_v4.patch

Second issue also fixed.
Attachment #8565926 - Attachment is obsolete: true
Attachment #8565926 - Flags: review?(matthew.mecca)
Attachment #8569048 - Flags: review?(matthew.mecca)
Comment on attachment 8569048 [details] [diff] [review]
bug_553132_v4.patch

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

Unfortunately using contentAreaClick as I had suggested doesn't work in SeaMonkey. What we can do instead is add our own event handler to taskDetailsView, something like this:

    onContentAreaClick: function onContentAreaClick(aEvent) {
        if (aEvent.target instanceof HTMLAnchorElement) {
            launchBrowser(aEvent.target.href, aEvent);
        }
    },

and in the iframe use onclick="taskDetailsView.onContentAreaClick(event);"

I briefly tested this in Thunderbird and SeaMonkey and it seams to work as expected, but please test.

Also, another issue I found is that line break formatting is lost if the task's description has multiple lines. We should be able to fix that by doing something like

 description = texttohtml.scanTXT(description, Components.interfaces.mozITXTToHTMLConv.kURLs)
                         .replace(/\n/g, "<br>");

but again, I only tested that briefly.
Attachment #8569048 - Flags: review?(matthew.mecca) → review-
(Assignee)

Comment 19

4 years ago
Created attachment 8571001 [details] [diff] [review]
bug_553132_v5.patch

Tested on thunderbird. Works fine.
Attachment #8569048 - Attachment is obsolete: true
Attachment #8571001 - Flags: review?(matthew.mecca)
Comment on attachment 8571001 [details] [diff] [review]
bug_553132_v5.patch

Looks good to me, pending ui-review.

Richard, do you think we need any style changes? It looks like the font changed a bit with the change to an iframe.
Attachment #8571001 - Flags: ui-review?(richard.marti)
Comment on attachment 8571001 [details] [diff] [review]
bug_553132_v5.patch

Yes, the font changed as it's now like a web page. It follows now the setting in Options/Display, also with color/background color. With this change it behaves now more like a normal message.

Now the font-family and font-size can be removed in https://dxr.mozilla.org/comm-central/source/calendar/base/themes/common/calendar-task-view.css#31 because they are now no more working.

Does already a bug exist to make the URLs clickable in the event description? I think this would make more sense than the actual approach with the URL as attachment. And use the attachments for documents etc. (but this would surely be an other beast to implement).
Attachment #8571001 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8571001 [details] [diff] [review]
bug_553132_v5.patch

Sorry to change the ui-r, but I've seen there is no context menu for copy/paste/etc.
Attachment #8571001 - Flags: ui-review+ → ui-review-
Comment on attachment 8571001 [details] [diff] [review]
bug_553132_v5.patch

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

Manu, could you add a context menu for the iframe as Richard suggested? It should only need a few menu items since we're not supporting inline editing at this point.
Attachment #8571001 - Flags: review?(matthew.mecca) → review-

Comment 24

3 years ago
bug 143475 would be a superset of this.
You need to log in before you can comment on or make changes to this bug.