Closed Bug 635693 Opened 10 years ago Closed 10 years ago

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)

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(blocking-thunderbird5.0 needed)

RESOLVED FIXED
Thunderbird 5.0b1
Tracking Status
blocking-thunderbird5.0 --- needed

People

(Reporter: standard8, Assigned: squib)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
Attached patch Really hacky patch (obsolete) — Splinter Review
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. :(
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #515012 - Flags: feedback?(bugzilla)
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).
I think it's actually the addons manager itself, so it might be hard to change that without forking it.
Attached patch Better patchSplinter Review
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...
Attachment #515012 - Attachment is obsolete: true
Attachment #515012 - Flags: feedback?(bugzilla)
Attachment #516818 - Flags: review?(bugzilla)
Whiteboard: [needs review standard8][not l10n]
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.
Attachment #516818 - Flags: review?(mbanner) → review+
Whiteboard: [needs review standard8][not l10n] → [needs updated patch]
Whiteboard: [needs updated patch] → [has review][needs updated patch]
Checked in with nits fixed: http://hg.mozilla.org/comm-central/rev/4d710242ad6e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.4
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...
Attachment #516818 - Flags: approval-thunderbird3.3?
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
Attachment #516818 - Flags: approval-thunderbird3.3? → approval-thunderbird3.3+
Whiteboard: [has review][needs updated patch]
Target Milestone: Thunderbird 3.4 → Thunderbird 3.3a4
In Miramar, links in the addons manager don't appear to work at all.
(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.
Okay, looks like it's just me, and I managed to get it going.
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.
Please see comment 13.
Flags: needinfo?(squibblyflabbetydoo)
https://developer.mozilla.org/en-US/docs/Web/API/Window/content
Flags: needinfo?(squibblyflabbetydoo)
So it is some globally defined object, similar to "document" or "window"?
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?
You need to log in before you can comment on or make changes to this bug.