635693 - Clicking on links in add-on manager brings up tabs with addons.mozilla.org but you can't do anything in those tabs (e.g. Personas installation)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mark Banner (:standard8)]

If I go into the add-ons manager and click on one of the links, e.g. an author link for an add-on or a link in the Get-Add ons pane, these all open up in a new tab.

However, that tab has no browse capabilities - all clicks end up being directed to the default browser.

There are probably two issues to this:

1) iirc the code for opening the add-on manager tab is set up so that all clicks on links matching "addons.mozilla.org" are loaded in the same tab as what the add-on manager opened in.

That clearly isn't happening, and it may not be a major issue, but I'd at least like to understand why.

2) We should have a way to inherit click handlers across content tabs. So, I want to say that all clicks matching "addons.mozilla.org" should open in a new tab and that tab should handle links to "addons.mozilla.org" internally in the tab and redirect other links to the default browser.

Comment 1

[Jim Porter (:squib)]

It seems like (1) does work for some stuff, but not for others (e.g. the "see all" links for personas/addons). It looks like that's intentional because that's what happens in Firefox.

I'm not sure how to go about (2) though. Any ideas?

Comment 2

[Jim Porter (:squib)]

Attachment: Really hacky patch

This seems like a really hacky way to go about this, but it does appear to work. A less-hacky way to do this would be to use the aOpener argument in this function, but it's inside an XRayWrapper, and I have no idea how to work with those. :(

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 515012 [details] [diff] [review]
Really hacky patch

So this is actually what I thought - something is forcing these links to be opened in a new window/tab.

It'd probably be much easier to handle, if we could control that, depending on where this is set up (I've not found it yet).

Comment 4

[Jim Porter (:squib)]

I think it's actually the addons manager itself, so it might be hard to change that without forking it.

Comment 5

[Jim Porter (:squib)]

Attachment: Better patch

Here's a better patch that 1) uses the appropriate function to get the tab associated with the content element, and 2) stores the click handler on the tab (in addition to the browser) to distinguish from onclick events that extensions might set for other reasons.

I'm not sure how to test this in Mozmill, since the failure case would involve opening an external browser and that seems like an impolite thing to do to the test machines...

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 516818 [details] [diff] [review]
Better patch

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

Sorry for the delay in getting to this. I've now worked out the other issue that I thought was related to this (but wasn't as close I originally thought).

This looks fine with the nits addressed.

::: mail/base/content/mailWindow.js
@@ +700,5 @@
>      let loadInBackground =
>        Application.prefs.getValue("browser.tabs.loadDivertedInBackground", false);
>  
> +    let tabmail = win.document.getElementById("tabmail");
> +    let clickHandler = tabmail.getBrowserForDocument(content).clickHandler;

From what I can tell, tabmail.getBrowserForDocument can return null at times, so I think we should have a null check here so that we don't throw if we can't get the clickHandler.

::: mail/base/content/specialTabs.js
@@ +378,5 @@
>        aTab.browser.setAttribute("id", "contentTabBrowser" + this.lastBrowserId);
>  
> +      aTab.clickHandler = "clickHandler" in aArgs && aArgs.clickHandler ?
> +                          aArgs.clickHandler :
> +                          "specialTabs.defaultClickHandler(event);"

nit: this should have a semi-colon on the end.

Comment 7

[Jim Porter (:squib)]

Checked in with nits fixed: http://hg.mozilla.org/comm-central/rev/4d710242ad6e

Comment 8

[Jim Porter (:squib)]

Comment on attachment 516818 [details] [diff] [review]
Better patch

I guess this is probably safe for 3.3 since it's blocking, but just to be sure, asking for approval...

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 516818 [details] [diff] [review]
Better patch

Yep, definitely need this in 3.3. I've pushed this for today's nightly as well so we can start getting some more testing on it. Thanks:

http://hg.mozilla.org/releases/comm-miramar/rev/0fca81d847fe

Comment 10

[Geoff Lankow (:darktrojan)]

In Miramar, links in the addons manager don't appear to work at all.

Comment 11

[Jim Porter (:squib)]

(In reply to comment #10)
> In Miramar, links in the addons manager don't appear to work at all.

It seems to work for me in 5.0b2.

Comment 12

[Geoff Lankow (:darktrojan)]

Okay, looks like it's just me, and I managed to get it going.

Comment 13

[:aceman]

Comment on attachment 516818 [details] [diff] [review]
Better patch

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

let tabmail = win.document.getElementById("tabmail");
                    .openTab("contentTab", {contentPage: "about:blank",	
 let clickHandler = tabmail.getBrowserForDocument(content).clickHandler;

Squib, what is the "content" variable referring to here? I do not see it defined anywhere.

::: mail/base/content/mailWindow.js
@@ +700,5 @@
>      let loadInBackground =
>        Application.prefs.getValue("browser.tabs.loadDivertedInBackground", false);
>  
> +    let tabmail = win.document.getElementById("tabmail");
> +    let clickHandler = tabmail.getBrowserForDocument(content).clickHandler;

Squib, what is the "content" variable referring to here? I do not see it defined anywhere.

Comment 14

[:aceman]

Please see comment 13.

Comment 15

[Jim Porter (:squib)]

https://developer.mozilla.org/en-US/docs/Web/API/Window/content

Comment 16

[:aceman]

So it is some globally defined object, similar to "document" or "window"?

Comment 17

[Jim Porter (:squib)]

It is a property on the window (as is "document"), as the docs state. Since Javascript is a portal into hell, you can omit the "window" part when accessing its members. Chances are pretty good I copied that line from somewhere else, but that was over four years ago now, so who knows?