If we don't handle clicks via an onclick wrapper (cf. browser.xul/browser.js in Firefox code), clicks are handled in nsWebShell.cpp. During said handling the links are tested by nsWebShell::OnLinkClickSync() to decide whether an external app will load them. This involves checking network.protocol-handler.expose-all and network.protocol-handler.expose.* to see if the scheme is handled by the host app. In Firefox Help most links are chrome links. The chrome-protocol-specific pref is unset, so *only* because network.protocol-handler.expose-all is set to true do links actually work. In Thunderbird these preferences specifically disable the chrome protocol (and rightly so), so link loading in the help viewer won't work there. Most xulrunner apps actually shouldn't have this stuff enabled, so basically as it is now the help viewer isn't usable in most xulrunner applications. I'm pretty sure there's no existing generic wrapper function in toolkit to do this, so we'll have to roll our own. I plan to model the patch off contentAreaClick() in browser.js, albeit with as greatly reduced functionality as I can manage; we aren't interested in implementing either security holes or much functionality beyond opening chrome, http, and https links inside the viewer (unless a link is targeted to a new window, depending on how easy this is to implement). This is needed to make the help viewer work with Thunderbird. Look for a patch sometime in the next month...

I tested this in both Firefox and in Thunderbird (with some work-in-progress code that makes the help viewer work there); things work correctly in both places. The security check looks like it's only useful going from web content to chrome or file, so unless someone uses a hack like <span onclick="location='url';"/> to open a malicious (it has to be malicious, because normal links to other websites are handled by the default handler) outside page it's not going to do anything. On the other hand, I have no idea what areas require security scrutiny, so I left it in just to be safe. It's also not entirely clear to me exactly how much of the wrapping code is necessary here given that the help viewer can only be loaded from chrome; I'd particularly appreciate comments on that aspect of the patch. Ideally I'd like to cut out all the wrapping that can possibly be cut out, because it makes the code less readable. I don't know how much that is, tho, so what you see is pretty much uncut. Note that this patch conflicts with patches posted to bug 262575, but the conflicts are mostly because we both implement the same onclick handler for the browser window. My implementation has more of the wrapper and security checks still in it, whereas the patch there doesn't. Either one probably fixes this bug, but I haven't tested the patch there to know for sure.

Comment on attachment 206226 [details] [diff] [review] Patch >+ var target = aEvent.target; ... >+ target = wrapper.getAttribute("target"); Separate the variables please. >+ if (!target) { ... >+ } >+ else >+ return true; // normal handling will use a new window What's the desired behavior for http links in the help viewer (esp. if there is no "target" attribute)? Also, does this break target="content"? >+# Write debug string to javascript console and sterr. Make this a JS comment please. > function log(aText) { >- CONSOLE_SERVICE.logStringMessage(aText); >+ dump(aText); Is this necessary?

(In reply to comment #2) > What's the desired behavior for http links in the help viewer (esp. if there > is no "target" attribute)? Also, does this break target="content"? The desired behavior is that all non-chrome links (both with and without a target attribute, even if it's "_content" or "_main" -- doc writers should leave out the attribute if they want in-viewer loading of a link) will load in an external application. This updated version of the patch actually makes that a little clearer, because apparently we were mangling external links before in a way which Necko caught and somehow parsed through; it won't have to do any un-mangling now. It's possible to load some URIs in the viewer right now, but they don't work well enough for me to feel good allowing those loads to happen right now. For example, neither http nor file pages support loading chrome URLs, and http pages tend to want to load http links externally for reasons which are currently beyond me. Note that "external" shouldn't be a problem during normal Firefox use, because the load will briefly trigger a new Firefox until the existing process is found, at which point the URL will be sent to the process to load; there shouldn't be any real outward difference from what happens now. > > function log(aText) { > >- CONSOLE_SERVICE.logStringMessage(aText); > >+ dump(aText); > > Is this necessary? Probably not, although I have a feeling I may be adding and removing this from my own tree as I make further changes here.

Comment on attachment 208250 [details] [diff] [review] Patch, v2 Mm, working on testcases is tasty -- all those tasty little bugs just magically appear, ripe for the eating; there'll be a fix within the next few days at worst...

The bug was that relative links wouldn't work unless they were appendable to the base URI for the content pack. Firefox Help doesn't demonstrate this problem because all files are kept within the nc:base directory. Without the changes in this version, relative links would be resolved with respect to the nc:base URI, which simply won't work. I *really* need to get that content pack testcase done soon...

Comment on attachment 210666 [details] [diff] [review] Make relative links in help pages work >+ if (uri.indexOf(":") < 0) { Nit: use !/:/.test(uri) >+ var wrapper = null; XPCNativeWrappers are obsolete. Just use linkNode. >+ if (aEvent.button == 0 && >+ !(aEvent.ctrlKey || aEvent.shiftKey || aEvent.altKey || aEvent.metaKey)) { You should make this early return on failure. And this particular check can be done before detecting the link node. >+ if (!wrapper.href) >+ return true; // no URL, so nothing to open You already checked for href earlier. >+ if (wrapper.getAttribute("onclick")) >+ return true; // onclick is handled by the default handler If onclick wants to prevent the link opening, it will call event.preventDefault(); and you should call event.getPreventDefault() to tell. (And you should do this earlier, too). >+ aEvent.preventDefault(); >+ return false; Nit: These should be equivalent.

Comment on attachment 210666 [details] [diff] [review] Make relative links in help pages work On issues from comment 6.

Adding dependency on bug 262575 per comment 1.