Sometimes I want to actually open the website for the feed message I'm looking at instead of the display in the message pane, e.g. to comment on an entry. Shredder trunk displays a "website" header there with the URL, SeaMonkey should do the same and make it possible to either copy the URL or open it (in a new window/tab, whatever is set with prefs).

I have a WIP in my tree that already displays that header but I still need to connect it with the browser as the TB implementation wants to open it externally and I don't think we want to open web pages in Firefox from SeaMonkey mailnews :)

This patch is everything we need for displaying the header, but we still don't open the website on click etc. correctly, as http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/mailWidgets.xml#197 seems to be hardwired to call it as an external URL. Still, we can do this as a first step and try to fix up the URL header widget in a second step.

This second step introduces a openUILink() function in Thunderbird and uses it in that binding. Firefox and SeaMonkey have a function with the same name (with more params available, but usually called with only the url), so having that function enables code to call URLs from UI with the same function in all those apps. I couldn't test if it actually calls the external browser because in my Shredder build the original code already results in an NS_ERROR_FAILURE returned by launchExternalURL() - which is the same with my patch, only that now the place where it throws that is openUILink() when it was messenger.xul line 1 without this patch. From that, I suspect it still works correctly in Shredder for people where it worked before my patch as well. In SeaMonkey, it works correctly in that a click on the URL actually opens it in the browser.

And here's another patch that adds the "Copy Link Location" context menu. It may be a good idea to add additional items for "open in new window" and "open in new tab" later, but I think click-open and context-copying are the most important functionalities other than actually displaying the website link.

(In reply to comment #3) > Created an attachment (id=346753) > step 2: switch mail-urlfield to openUILink() and implement it in Thunderbird Adding text-link to the class almost makes it work everywhere, although it always opens a new window which is probably suboptimal (I wonder whether it does that in Firefox to or whether they just live with it.) Speaking of class, there's probably some theme CSS missing somewhere ;-)

Comment on attachment 346759 [details] [diff] [review] step 3: add context menu with "copy link location" (checked in) >+<popup id="copyUrlPopup" popupanchor="bottomleft"> I wonder why we have popupanchor... I don't think we need it. >+ <menuitem label="&copyLinkCmd.label;" accesskey="&copyLinkCmd.accesskey;" oncommand="CopyWebsiteAddress(document.popupNode)"/> Nit: ; Some other menuitems would be nice, such as Open Link in New Tab etc. I guess it would be too hard to fake out the regular content context menu to work here.

> Created an attachment (id=346753) [details] > step 2: switch mail-urlfield to openUILink() and implement it in Thunderbird > This second step introduces a openUILink() function in Thunderbird and uses it Is the toolkit->ContentAreaUtils->openURL() function available in this context? Code inspection implies that it is.

> (From update of attachment 346759 [details] [diff] [review]) >> +<popup id="copyUrlPopup" popupanchor="bottomleft"> > I wonder why we have popupanchor... I don't think we need it. Didn't I fix a sidebar bug involving popupanchor recently by removing it?

> Didn't I fix a sidebar bug involving popupanchor recently by removing it? That's Bug 388349 . "popupanchor" seems to misbehave on trunk ever since the other Neil rewrote the XUL popup code.

(In reply to comment #7) > Is the toolkit->ContentAreaUtils->openURL() function available in this context? openURL() and openUILink() are different in being about opening internal vs. external URLs, IIRC, and we really should end up treating mailnews completely as internal, IMHO.

<xul:label onclick="if (!event.button) openUILink(event.target.value);" from code inspection, I think this should be: <xul:label onclick="if (!event.button) openUILink(event.target.value, event);" This should fix Bug 463515

> diff --git a/mail/base/content/mailWindow.js b/mail/base/content/mailWindow.js > +function openUILink(url) > diff --git a/mailnews/base/resources/content/mailWidgets.xml b/mailnews/base/resources/content/mailWidgets.xml > - <xul:label onclick="if (!event.button) messenger.launchExternalURL(event.target.value);" > + <xul:label onclick="if (!event.button) openUILink(event.target.value);" So can you guarantee that openUILink() will be available to all future usages of <mail-urlfield> (now only used in msgHdrViewOverlay.xul).

(In reply to comment #12) > So can you guarantee that openUILink() will be available to all future usages > of <mail-urlfield> (now only used in msgHdrViewOverlay.xul). Not any more or less that one can guarantee an object named "messenger" is present (which is declared in the same scope in Thunderbird). mailWindow.js on the Thunderbird side is imported in almost all mail-related windows and utilityOverlay.js on the SeaMonkey side is imported into almost any SeaMonkey window. It probably would be even nicer if we'd provide those openUILink* functions in a cleaner way, but we're following the pattern Firefox has and extensions are already using.

> we really should end up treating mailnews completely as internal, IMHO. We changed to not doing this when migrating the respective pref pane, and on purpose: if you have several browser windows/tabs open and click a link in a mail window, you are usually not aware where the content is going to be shown, just like link clicks in any other application when we are the default browser. For example: when surfing, I want normal links to open in my current browser tab, but when in a mail window, I don't want that, because I can't see what I'm replacing and where. We briefly discussed that own prefs for mailnews click behaviour would be nice, but went for using "external" then.

This new version uses utilityOverlay in Thunderbird, hands over the event so we can do different things based on middleclick and modifier keys (ignored in Thunderbird for now) and allows middleclick (event.button == 1) to be handed over to openUILink().

(In reply to comment #14) > > we really should end up treating mailnews completely as internal, IMHO. > > We briefly discussed that own prefs for mailnews click behaviour would be nice, > but went for using "external" then. Hrm. What function(s) should we be using then? I'm overlay frustrated with how hard it is to get something simple as this right. Either I'm that stupid or our code seriously sucks.

(In reply to comment #16) > I'm overlay frustrated with how hard it is to get something simple as this right. Oh, if only it was simple. Well, I guess it was way back before tabbed browser, when all we supported was open in most recent browser and open in new window. Tabbed browsing then made things harder, and internal/external preferences made things harder still. I don't think we have one function that covers all uses.

Comment on attachment 346876 [details] [diff] [review] step 2, v2: switch to openUILink(), implement it in Thunderbird, accept middle-click Thunderbird might or might not want middle-clicking to load this link.

I hate to generate even more bugmail here, but adding the text-link class to that label looks like the right thing to do to style it consistently with other links in UI. It would be so nice if we would just have one consistent function to use for opening URLs from at least parts of UI that aren't inherently browser-related, bei it openURL() from toolkit, an openUILink() variant or whatever.

This version avoids implementing a stub version of openUILink() in Thunderbird at the expense of a bit of verbosity. But messenger.launchExternalURL() is just a thin wrapper around nsIExternalProtocolService::loadUrl(uri).

Comment on attachment 346759 [details] [diff] [review] step 3: add context menu with "copy link location" (checked in) I know the rest of msgHdrViewOverlay.xul has lots of lines over 80 characters but should we be continuing that practise?

Comment on attachment 346746 [details] [diff] [review] step 1: display the header (checked in) Pushed this as http://hg.mozilla.org/comm-central/rev/bf341271788b - I agreee that we probably should get back to following the 80char limit (and being more similar to Thunderbird at the same time), but I think we should do this for the whole file in a separate bug/patch.

Comment on attachment 346759 [details] [diff] [review] step 3: add context menu with "copy link location" (checked in) Pushed as http://hg.mozilla.org/comm-central/rev/01c92ff2d3da Now with steps 1 and 3 in, we display the header and offer the "copy" context menu, I'll file further context menuitems as a followup. We still need either one of the solutions for step 2 for opening the website correctly.

> We still need either > one of the solutions for step 2 for opening the website correctly. mailWidgets.xml is preprocessed, so I guess that a terser but Q&D solution would be something like: #ifdef MOZ_THUNDERBIRD <xul:label onclick="if (event.button != 2) messenger.launchExternalURL(event.target.value);" #else <xul:label onclick="if (event.button != 2) openUILink(event.target.value, event);" #endif

Comment on attachment 346890 [details] [diff] [review] step 2, v2.1: switch to openUILink(), implement it in Thunderbird, accept middle-click, use text-link class this spacing is very odd: +// openUILink handles clicks on UI elements that cause URLs to load. +// Firefox and SeaMonkey have a function with the same name, +// so extensions can use this everywhere to open links. +// We currently ignore the event (e) in Thunderbird. I haven't tried this patch - but it looks like it will make event.button == 1 also open the link. Does adding text-link to the class affect Thunderbird's behavior or appearance?

(In reply to comment #25) > (From update of attachment 346890 [details] [diff] [review]) > this spacing is very odd: I can change it in any way you want, I just tried to make it as readable as possible :) > I haven't tried this patch - but it looks like it will make event.button == 1 > also open the link. Yes, it does, as on SeaMonkey we might want different behavior for left and middle mouse button (and some people see middle-click not working as an actual bug, see bug 463515). Is this a problem for Thunderbird? > Does adding text-link to the class affect Thunderbird's > behavior or appearance? Not from what I saw in my testing on my Shredder trunk build (Linux). Maybe you could actually remove some CSS because of this though. I'd very much like to end up with a generic solution of what function to call from mailnews and extensions for opening URLs like that, so the ifdef solution doesn't sound ideal to me. I'll leave it up to Thunderbird folks to weigh in and tell about their preference on what mechanism to use. Once we have a solution, it might be a good idea to apply it to UI links throughout mailnews to have everything work consistently (followup fodder).

+// openUILink handles clicks on UI elements that cause URLs to load. +// Firefox and SeaMonkey have a function with the same name, +// so extensions can use this everywhere to open links. +// We currently ignore the event (e) in Thunderbird. I think lines 2-4 should line up with the first line, that's all. I agree with you - I'd rather have a generic solution, not ifdefs.

I think first we need to sort out what functionality Thunderbird should have, hence cc'ing Bryan. At the moment, Thunderbird doesn't really use middle-click for anything. Bug 270434 is requesting that we enable middle-click on links (and I myself occasionally try middle-clicking on links in TB)... However, Bryan is also thinking of using middle-click to open folders, accounts and messages in new tabs (bug 270434 comment 17). Hence doing this for links may confuse the user as they may want it to open in a different tab. We should probably do a full discussion on this in bug 270434, however if Bryan has a general idea of where we may be heading with this, I think it would be useful to get that here.

For now, in Thunderbird, lets keep the middle click function to only open tabs inside Thunderbird (i.e. for folders, accounts, and conversations). I'm worried that middle clicking links will bring expectations of opening in a tab inside firefox or inside thunderbird, neither of which will happen. I'll probably need to do a bit of user testing to make sure, so I'd rather remain simple for now and change if we find out something new later.

OK, what Bryan says can easily be done in my version of the patch by checking for button==1 in openUILink() and returning without any action in that case. I still wonder what we should do in terms of what common function to use for opening URLs, or if to go for a singular function like in Philip's alternate proposal.

(In reply to comment #30) > OK, what Bryan says can easily be done in my version of the patch by checking > for button==1 in openUILink() and returning without any action in that case. Let's do this for now, and we can file a follow up bug if we want to propose some other common function for opening URLs.

Comment on attachment 346890 [details] [diff] [review] step 2, v2.1: switch to openUILink(), implement it in Thunderbird, accept middle-click, use text-link class Per previous comment

This version of the patch corrects indenting of the comment and only reacts to left click on Thunderbird.

Comment on attachment 348987 [details] [diff] [review] step 2, v2.2: switch to openUILink(), only support left click on Thunderbird bonus for renaming e to event for code clarity ;-)

pushed step 2 as http://hg.mozilla.org/comm-central/rev/9dd2c7309a13 with nit from comment #34 addressed.