920118 - Make "visited link" coloring work in thunderbird (enable and use Places for history)

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[David E. Ross]

Attachment: Colors window showing my selections

In an attempt to work around bug #919797, I changed the color for Unvisited Links.  At the same time, I changed the color for Visited Links to be a bit more red.  However, visited links have the same color as unvisited links.  

Attached are screen prints:
(1) My Colors window showing my selection for visited and unvisited links.  
(2) The body of a newsgroup message.  The link in the line beginning "See bug" was visited.  The color did not change.  Even after terminating Thunderbird and then launching it again, the link has the unvisited color.

Comment 1

[David E. Ross]

Attachment: Message with visited link having unvisited color.

Comment 2

[rsx11m]

Hmm, I'm wondering if Thunderbird uses the browser history at all. A places.sqlite file is present but doesn't seem to be used. In this case, the "visited" status of any link would always be false and the setting/style for visited links never applied.

Comment 3

[rsx11m]

FWIW, SeaMonkey /does/ apply the visited-link color for mail/news messages.

Comment 4

[David E. Ross]

Netscape 4 also distinguished visited and unvisited Web links by color.

Comment 5

[rsx11m]

That makes sense given that both SeaMonkey and Netscape 4 have direct access to the browser history within the same application. In contrast, if Thunderbird calls Firefox to open the link, it would maximally know that this link has been clicked on and store that in its local "browsing" history for later reference. Apparently this is not done, thus all links show as not visited.

Comment 6

[David E. Ross]

This bug report should be resolved by one of the two following implementations:  

1.  Thunderbird should eliminate the choice of color for visited links.  This would also eliminate the editor.followed_link_color preference variable (I think this is the one).  This would be the easiest to implement.  

or

2.  The suggestion in the second sentence of comment #5 should be implemented.  This should include a user interface to specify an age in days when old entries should be deleted (as requested for browsers in bug #660646).  This would best serve end users.

Comment 7

[Wayne Mery (:wsmwk)]

is bug 1005507 a duplicate?

Comment 8

[David E. Ross]

(In reply to Wayne Mery (:wsmwk) from comment #7)
> is bug 1005507 a duplicate?

Yes.  Since I am an end-user and not a developer or tester, I prefer to leave marking someone else's bug report as Duplicate or otherwise Closed to one of those Mozillians.

Comment 10

[rsx11m]

Attachment: Interim fix combining the options

(In reply to David E. Ross from comment #6)
> 1.  Thunderbird should eliminate the choice of color for visited links. 
> This would also eliminate the editor.followed_link_color preference variable
> (I think this is the one).  This would be the easiest to implement.  

So, unless the plan is to utilize the places backend in the future, this appears to be the best solution for now, as a minimum for the upcoming 31.x releases to remove the non-functional UI there.

This patch removes the hbox for browser.visited_color (the backend code has to remain, it's Core code) and combines it with the browser.anchor_color preference (the only one that counts right now). The UI doesn't look as balanced any more as before with that hole, but without the vertical spacer to compensate for the now missing center option it looks awkward.

At least on trunk, the label has to be changed, I'm suggesting "All links:" here after first thinking of "Any link:" or something like "All link types" or maybe even just "Links:" for brevity. Note that for 31.x, I'd post a branch patch without changes to colors.dtd and without entity/id changes:

>-          <label value="&linkColor.label;" accesskey="&linkColor.accesskey;" control="unvisitedlinkmenu"/>
>+          <label value="&anyLinkColor.label;" accesskey="&anyLinkColor.accesskey;" control="anylinkmenu"/>
>           <spacer flex="1"/>
>-          <colorpicker type="button" id="unvisitedlinkmenu" palettename="standard"
>+          <colorpicker type="button" id="anylinkmenu" palettename="standard"

Drive-by fixes: There are several accesskey definitions using lower-case letter even though the respective letter in the label is upper case. This results for "Text:" to underline the trailing 't' rather than the leading 'T'. I didn't modify any entity names for these cases given that it's an en-US local change only without change in semantics.

I was wondering if I should leave the old strings in the DTD file. Thus, if in the future the places backend is used, the unvisited/visited distinction can be easily re-established even on the branches. However, if it doesn't get used in the future, we would keep two unused strings around.

Comment 11

[rsx11m]

Attachment: Screenshot

Comment 12

[Magnus Melin [:mkmelin]]

Well, we already include places, so maybe we should just call PlacesUtils.asyncHistory.updatePlaces before we open external links, and it things would likely work.

Comment 13

[rsx11m]

I don't know where to look for, and I don't find this used in SeaMonkey so that I could use it as template. Since you seem to know what's necessary, do you want to take it from here, and I open a separate bug for the accesskey cases?

Comment 14

[Magnus Melin [:mkmelin]]

I think it would be the couple of cases we call messenger.launchExternalURL, like http://mxr.mozilla.org/comm-central/source/mail/base/content/utilityOverlay.js#173

Comment 15

[rsx11m]

Comment on attachment 8432170 [details] [diff] [review]
Interim fix combining the options

Ok, I'll give it a try (cancelling the review requests for this one).

Comment 16

[rsx11m]

(In reply to rsx11m from comment #10)
> Drive-by fixes: There are several accesskey definitions using lower-case
> letter even though the respective letter in the label is upper case. This
> results for "Text:" to underline the trailing 't' rather than the leading
> 'T'. I didn't modify any entity names for these cases given that it's an
> en-US local change only without change in semantics.

This part is now bug 1018718, let's concentrate on activating places here.

(In reply to Magnus Melin from comment #14)
> I think it would be the couple of cases we call messenger.launchExternalURL,

http://mxr.mozilla.org/comm-central/ident?i=launchExternalURL&filter=mail/ gives me a handful of results where an external handler is invoked (also context menu and the address book).

Comment 17

[rsx11m]

Attachment: Draft patch (v2, incomplete)

So, this is how far I got for now. First of all, places.history.enabled has to default to "true" for anything to happen (that pref should probably be exposed in the UI to satisfy privacy concerns, along with the browser's "Clear History" button, but that can go into a follow-up bug), otherwise updatePlaces() will just prompt an NS_ERROR_ILLEGAL_VALUE error.

The occurrences for launchExternalURI are restricted to the message-viewer UI and the address book (map location and the contact's websites). To test the WIP patch you'll have to define some header like "Content-Base: http://..." which creates a link in the header pane. Clicking the link does indeed add the URL to places.sqlite and hence makes it show up colorized as "visited" in content.

The code is a bit verbose given that I declared intermediate variables and callback functions for easier debugging. The most compact and still working representation is something like:

function openUILink(url, event)
{
  if (!event.button) {
    PlacesUtils.asyncHistory.updatePlaces({
      uri: makeURI(url),
      visits:  [{
        visitDate: Date.now() * 1000,
        transitionType: Components.interfaces.nsINavHistoryService.TRANSITION_LINK
      }]
    });
    messenger.launchExternalURL(url);
  }
}

So, this should be portable to the address-book and context-menu instances.

BUT, and this is the main issue I'm stuck with, it /doesn't/ work for links in the message content even with the pref enabled, and there doesn't seem to be any instance of the launchExternalURL() function for the message itself. Maybe nothing specific to mailnews handles this, but rather the core's content or toolkit code?

Thus, either I'm missing something to make that part work, or that capability needs to be added in the external-handler code if it doesn't automatically add links which are supposed to be handled externally to the history.

Comment 18

[rsx11m]

(In reply to rsx11m from comment #17)
> BUT, and this is the main issue I'm stuck with, it /doesn't/ work for links
> in the message content even with the pref enabled, 

I was trying to reproduce this in SeaMonkey (where the history works for links handled internally), creating network.protocol-handler.external.http as true, which doesn't quite work (upon clicking, instead of opening the default browser only a new empty tab is opened). However, as in Thunderbird, content links clicked on in this way and to be handled externally aren't added to the history either (which may not happen due to the external handling not working entirely as expected, though).

Comment 19

[rsx11m]

Never mind, guess I've found where the message links are handled:

http://mxr.mozilla.org/comm-central/source/mail/base/content/contentAreaClick.js#107
> 107   // We want all about, http and https links in the message pane to be loaded
> 108   // externally in a browser, therefore we need to detect that here and redirect
> 109   // as necessary.
> 110   let uri = makeURI(href);
> 111   if (Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
> 112                 .getService(Components.interfaces.nsIExternalProtocolService)
> 113                 .isExposedProtocol(uri.scheme) &&
> 114       !uri.schemeIs("http") && !uri.schemeIs("https"))
> 115     return true;

Thus, I'll have to sneak in an updatePlaces on that uri after this check for the link type.

Comment 20

[rsx11m]

Attachment: Proposed patch (v3)

This works for me (i.e., URL added to places.sqlite) with the following cases:

 - opening a link from a message (contentAreaClick.js)
 - using "Open link in browser" from the context menu (nsContextMenu.js)
 - clicking on a link in a message header (utilityOverlay.js)
 - clicking the "Map It" button in an address book card (abCardViewOverlay.js)
 - clicking a contact's link in an address book card (abCardViewOverlay.js)

I didn't cover the occurrence in mailContextMenus.js as it relates to opening a message ID in a browser based on a pref-specified URL template, which sounds like a strictly internal usage.

In abCardViewOverlay.js, no global makeURI() function is made available, thus I'm generating the URI object from scratch. Also, exception handling is a bit inconsistent (in some instances they are ignored, in others not; I left the handling as is).

Places history is switched on by default, no change in the preferences UI.

Comment 21

[rsx11m]

(In reply to rsx11m from comment #20)
> In abCardViewOverlay.js, no global makeURI() function is made available,
> thus I'm generating the URI object from scratch.

Change to attachment 8433815 [details] [diff] [review] made locally:

> @@ -557,10 +557,8 @@
>  function OpenURLWithHistory(url)
>  {
>    try {
> -    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
> -                              .getService(Components.interfaces.nsIIOService);
>      PlacesUtils.asyncHistory.updatePlaces({
> -      uri: ioService.newURI(url, null, null),
> +      uri: Services.io.newURI(url, null, null),
>        visits:  [{
>          visitDate: Date.now() * 1000,
>          transitionType: Components.interfaces.nsINavHistoryService.TRANSITION_LINK

Since we have Services.io, no need to reinvent the wheel here...

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8433815 [details] [diff] [review]
Proposed patch (v3)

Sorry for the delay. This looks really good to me (with the Services change you already did locally) r=mkmelin

Comment 23

[rsx11m]

Attachment: Patch for check-in

Thanks, final patch with comment #21 applied and the bug-title change reflected in the commit message.

Comment 24

[Jonathan Protzenko [:protz]]

Checked-in, now that the tree is open

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