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)

RESOLVED FIXED in Thunderbird 5.0b1

Status

defect
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: standard8, Assigned: squib)

Tracking

Trunk
Thunderbird 5.0b1

Firefox Tracking Flags

(blocking-thunderbird5.0 needed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

9 years ago
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.
Assignee

Comment 1

9 years ago
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?
Assignee

Comment 2

8 years ago
Posted 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)
Reporter

Comment 3

8 years ago
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).
Assignee

Comment 4

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

Comment 5

8 years ago
Posted 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)
Reporter

Updated

8 years ago
Whiteboard: [needs review standard8][not l10n]
Reporter

Comment 6

8 years ago
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+
Reporter

Updated

8 years ago
Whiteboard: [needs review standard8][not l10n] → [needs updated patch]
Reporter

Updated

8 years ago
Whiteboard: [needs updated patch] → [has review][needs updated patch]
Assignee

Comment 7

8 years ago
Checked in with nits fixed: http://hg.mozilla.org/comm-central/rev/4d710242ad6e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.4
Assignee

Comment 8

8 years ago
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?
Reporter

Comment 9

8 years ago
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+
Reporter

Updated

8 years ago
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.
Assignee

Comment 11

8 years ago
(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 13

4 years ago
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

4 years ago
Please see comment 13.
Flags: needinfo?(squibblyflabbetydoo)
Assignee

Comment 15

4 years ago
https://developer.mozilla.org/en-US/docs/Web/API/Window/content
Flags: needinfo?(squibblyflabbetydoo)

Comment 16

4 years ago
So it is some globally defined object, similar to "document" or "window"?
Assignee

Comment 17

4 years ago
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.