263433 - 'text-link' XUL widget should open tabs rather than windows

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Ger Teunis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041007 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041007 Firefox/0.10

Links in items listed in Firefox Update open a new window for "Visit Homepage"
even when "Force links that open new windows" is selected and set to "a new tab"
in Tools -> Options -> Advanced Options -> Tabbed Browsing.


Reproducible: Always
Steps to Reproduce:
1. Go to Tools -> Options -> Advanced Options -> Tabbed Browsing
2. Select "Force links that open new windows" and set to "a new tab"
3. Go to Tools -> Options -> Advanced Options -> Software update -> Check Now
Actual Results:  
A new window with the requested page.

Expected Results:  
A new tab with the requested page.

Similar to bug #262575 and #262808.

Comment 1

[José Jeria]

Do you mean the popup with the "checking for update"? That is no window, that is
a dialog.

Comment 2

[Ger Teunis]

(In reply to comment #1)
> Do you mean the popup with the "checking for update"? That is no window, that is
> a dialog.

My bad. Modified Summary.

Comment 3

[Nickolay_Ponomarev]

Could you explain what 'links' in firefox update dialog you are talking about?
do I have to have some non-up-to-date extensions/themes to reproduce this bug?

Comment 4

[Ger Teunis]

When I posted this bugreport the "Firefox 1.0 Preview Release" was available.
Had to mention this in the steps to reproduce indeed.

I noticed the "More information ..." link in the software update opens a new
windows. This link is shown under "Firefox 1.0 Preview Release" item.
(manually set your app.version to i.e. 0.8 will trigger this "Firefox 1.0
Preview Release" update to appear).

I am assuming all links in update information will open new windows, but I do
not know for sure.

Comment 5

[Nickolay_Ponomarev]

confirming. The "more info" link opens a new window despite tab-prefs set.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0

Comment 6

[Anbo Motohiko]

(In reply to comment #4)
> When I posted this bugreport the "Firefox 1.0 Preview Release" was available.
> Had to mention this in the steps to reproduce indeed.
> 
> I noticed the "More information ..." link in the software update opens a new
> windows. This link is shown under "Firefox 1.0 Preview Release" item.
> (manually set your app.version to i.e. 0.8 will trigger this "Firefox 1.0
> Preview Release" update to appear).
> 
> I am assuming all links in update information will open new windows, but I do
> not know for sure.
Since Bug 273417 is fixed, pref 'app.version' is no longer used. How can I check
this?

I attached a patch for this bug to Bug 262575 attachment 184172 [details] [diff] [review].

Comment 7

[Ryan VanderMeulen [:RyanVM]]

*** Bug 298849 has been marked as a duplicate of this bug. ***

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I adjusted the summary to mention software update so hopefully further dupes can
be avoided.

Comment 9

[Darin Fisher]

This bug exists with the new software update system as well.  Adding to meta bug
for software update UI improvements.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This is a problem for all "text-link" xul widgets, not just those in software 
update (check out the 'privacy policy' link in the reporter extension, for 
another example). Updating product, summary, and other flags.

Comment 11

[Robert Accettura [:raccettura]]

*** Bug 294516 has been marked as a duplicate of this bug. ***

Comment 12

[Jason Barnabe (np)]

Correct me if I'm wrong, but doesn't the current code
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/text.xml#297
assume that it'll always be used from within a browser application that is the system default? Isn't that an incorrect assumption? Shouldn't it be more like what Thunderbird is doing here?
http://lxr.mozilla.org/seamonkey/source/mail/base/content/mailCore.js#299

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> Correct me if I'm wrong, but doesn't the current code
> http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/text.xml#297
> assume that it'll always be used from within a browser application that is the
> system default?

No, it doesn't assume that it is - it checks to see whether the protocol is exposed. If it is, it opens it directly. The code in Thunderbird doesn't do that because it assumes that http[s] isn't handled internally. Always using the external protocol service might get the correct result by passing all URIs through the system and then back to us through the existing tab/window handling code, but that will only work if we are the default http[s] handler, which isn't something we should rely on.

Comment 14

[Jason Barnabe (np)]

(In reply to comment #13)
> Always using the
> external protocol service might get the correct result by passing all URIs
> through the system and then back to us through the existing tab/window handling
> code, but that will only work if we are the default http[s] handler, which
> isn't something we should rely on.
If by "work" you mean "open in the current XUL browser app". I was just wondering whether that's really expected behaviour or whether it's expected to load in the default browser, regardless of what that is, in the general case. Should there a distinction in behaviour between true external apps and XUL apps loaded through Firefox?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> If by "work" you mean "open in the current XUL browser app". I was just
> wondering whether that's really expected behaviour or whether it's expected to
> load in the default browser, regardless of what that is, in the general case.
> Should there a distinction in behaviour between true external apps and XUL apps
> loaded through Firefox?

Well, for the XUL apps I know of that handle https, yes, I think the desired behavior is to have it open in that app. If you changed the binding to always use the EPS, then it wouldn't be suitable for Firefox's use. Maybe it could have an "alwaysExternal" property, but I'm not sure who would want to use it.

Comment 16

[Bradley Chapman (not reading bugmail, still gone but not forever)]

What is the best way to implement this?

After a quick read of attachment 205927 [details] [diff] [review] in bug 262575 I'm curious as to how much intelligence the binding needs to have in order to make this work.

Right now the binding is used in parts of the UI where a link to a Web site is needed (e.g. the software update dialog). Does this mean that the binding should be restricted to explicit knowledge of http/https/ftp links?

What about mailto links? Or data URLs? Or javascript URLs? Directly implementing attachment 205927 [details] [diff] [review]'s functionality in the text-link binding seems like overkill to me, so I'd like to know what sort of functionality is needed by the consumers before attempting this.

CCing Robert Strong for feedback.

Comment 17

[Robert Strong (they/them - no direct email)]

(In reply to comment #16)
> What is the best way to implement this?
> 
> After a quick read of attachment 205927 [details] [diff] [review] [edit] in bug 262575 I'm curious as to how
> much intelligence the binding needs to have in order to make this work.
> 
> Right now the binding is used in parts of the UI where a link to a Web site is
> needed (e.g. the software update dialog). Does this mean that the binding
> should be restricted to explicit knowledge of http/https/ftp links?
> 
> What about mailto links? Or data URLs? Or javascript URLs? Directly
> implementing attachment 205927 [details] [diff] [review] [edit]'s functionality in the text-link binding seems
> like overkill to me, so I'd like to know what sort of functionality is needed
> by the consumers before attempting this.
I see no reason it should be restricted as long as this is only available to chrome which could just as easily call a function in an onclick handler to accomplish what ever it wanted to with any scheme. I haven't read all of the comments in bug 262575 for quite some time now and there may be reasons for restricting stated there or someone else might have a reason but I don't.

Comment 18

[Robert Strong (they/them - no direct email)]

(In reply to comment #16)
> What is the best way to implement this?
If I knew the answer to that I'd implement it. :P

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> What is the best way to implement this?

Ideally we could just replace our binding with something resembling "Neil's approach" in bug 344500, and get the window targeting stuff for free. In my testing/tweaking I wasn't able to get it to open the URL in a new window, though, which obviously needs to be resolved (it would open addons.mozilla.org in the Add-ons manager window, for example). I'm not sure that it's possible to get an html:a to work in chrome windows.

Comment 20

[[e-mail]]

(In reply to comment #19)
>I'm not sure that it's possible to get an html:a to work in chrome windows.
Well, I realise that the build I was using has all sorts of cruft, and I should really have tried my patch for bug 344500 in a cleaner tree, but my understanding of the current state of the retargetting code is that a link into _blank will find a tabbed browser and ask it to return a content window, or failing that it will open a new browser window.
Or do we need to hack the default content listener to prevent link clicks from loading into chrome (which change I can't believe would impact anyone)?

Comment 21

[Boris Zbarsky [:bzbarsky]]

If we have an <html:a target="_blank"> in a chrome window, we will get the following sequence of events:

1)  window.open() called on the chrome window
2)  OpenWindow() called on the window watcher
3)  Window watcher tries to get a window provider from the tree owner
4)  nsChromeTreeOwner::GetInterface does not produce a window provider
5)  We just open a new window

I didn't put a window provider on the nsChromeTreeOwner because I wasn't quite sure whether, say, I wanted to affect what happens when chrome calls window.open() on a chrome window.  Should that always open a new window?  Or should it sometimes open a new tab, based on user prefs?  Should this depend on whether the "chrome" feature is passed in?

I don't really have a good feel for the various window.open() users in our chrome and extensions, basically.  If people work with me on speccing out the behavior of both <html:a> that's targeted to something that doesn't exist yet and window.open(), then we can modify the chrome tree owner accordingly, I hope.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Put another way, the question is:  should chrome be able to force opening of a new content window (even if the user prefers tabs in general)?  If so, how?

Comment 23

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: patch, v0.5

This was the sort of thing I had in mind when I posted comment #16. It works, but I think it's too complicated and can be greatly simplified, especially with regards to how external protocols vs. internal handling is implemented in the patch.

Comment 24

[[e-mail]]

bz, what about the alternative of using <html:a> without a target?
(but note that I don't want every text-link to install a content listener)

Comment 25

[Boris Zbarsky [:bzbarsky]]

<html:a> without a target will just load in the current chrome window, no?  Or is the question about something else?

Comment 26

[[e-mail]]

(In reply to comment #25)
><html:a> without a target will just load in the current chrome window, no?
>Or is the question about something else?
The question is how easy is it to alter the behaviour just for chrome windows and without having to lumber the text-link binding with a bunch of code?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Changing the behavior of completely targetless <html:a> would take a fair amount of work...

We could introduce a new target name that would only be special in chrome docshells, I suppose (for either the "force in window" case or the "do user pref" case, depending on what's desired).  That might take some detail work, but at least would be conceptually simple.

Comment 28

[[e-mail]]

Do you know if the behaviour of an <a target="_content"> in an editor sidebar (remember that editor disallows link loads into its content area) is governed by preferences or does it just open a window too?

Comment 29

[Boris Zbarsky [:bzbarsky]]

If there's no browser window open, it opens a new window.  Otherwise it finds the _content in one of the existing browser windows and uses that.

Comment 30

[Michael Ventnor]

Attachment: Patch 1.0

This is a cleaner version of Patch 0.5, as well as a cleanup of the function in general. It's quite messy.

I don't really have a clue of the right person to review this, so I just looked at the CC list and chose Gavin. :)

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 250043 [details] [diff] [review]
Patch 1.0

I'd rather you not do the Cc/Ci cleanup.

Since this binding should work from remote XUL where you can't use XPCOM, the window mediator stuff should be in the try/catch, and you can't remove the uri null check. https://bugzilla.mozilla.org/attachment.cgi?id=203756 is a good testcase for that aspect.

Comment 32

[Michael Ventnor]

Attachment: Patch 2

I can't see that bug, I get Access Denied. But I think I know what you mean.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Have you tested this in Thunderbird? Have you tested links in modal windows? I suspect the removal of walking over the .opener chain regresses that case (windows opened by modal windows are made modal, using the opener was a workaround to avoid that problem).

Comment 34

[:Gavin Sharp [email: [e-mail]]]

I'm also not sure that OPEN_DEFAULTWINDOW/OPEN_EXTERNAL really gives the correct behavior for in-chrome links, especially when the browser.link.open_external pref is set to "the most recent tab or window".

Comment 35

[Michael Ventnor]

Attachment: Patch 3

(In reply to comment #33)
> Have you tested this in Thunderbird? Have you tested links in modal windows? I
> suspect the removal of walking over the .opener chain regresses that case
> (windows opened by modal windows are made modal, using the opener was a
> workaround to avoid that problem).
> 

Thunderbird works because it goes through the external protocol handler. As for modal dialogs, thanks for making me aware of that. I restored the opener loop.

(In reply to comment #34)
> I'm also not sure that OPEN_DEFAULTWINDOW/OPEN_EXTERNAL really gives the
> correct behavior for in-chrome links, especially when the
> browser.link.open_external pref is set to "the most recent tab or window".
> 

Nowadays both the open_external and open_newwindow pref are kept in sync with each other, look at the javascript file of the tabs preferences pane to see. Passing it as an external link just makes things easier without needing to read preferences.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #35)
> Nowadays both the open_external and open_newwindow pref are kept in sync with
> each other, look at the javascript file of the tabs preferences pane to see.
> Passing it as an external link just makes things easier without needing to read
> preferences.

That's true in Firefox, sure, but this binding isn't only used by Firefox. There's also the rarer case of people who use about:config to change the prefs themselves.

Comment 37

[Michael Ventnor]

Attachment: Patch 4

That's true. I guess I worry too much about code size.

Comment 38

[Michael Ventnor]

Attachment: Patch 5

I found a method which doesn't use any constant numbers so this should be completely app-agnostic.

Comment 39

[Asaf Romano (gone)]

Comment on attachment 254701 [details] [diff] [review]
Patch 5

Gavin should continue this review.

I don't like the "navigator:browser" dependency. An alternative would be to loop over all windows (by z-order) and take the first window which has a browserDOMWindow object (via nsIDOMChromeWindow).

Comment 40

[Michael Ventnor]

Comment on attachment 254701 [details] [diff] [review]
Patch 5

(In reply to comment #39)
> I don't like the "navigator:browser" dependency. An alternative would be to
> loop over all windows (by z-order) and take the first window which has a
> browserDOMWindow object (via nsIDOMChromeWindow).
> 

Are you sure? There are many other references to navigator:browser in toolkit. Also, Z-order enumeration is still broken under Linux (bug 156333).

Comment 41

[[e-mail]]

Comment on attachment 254701 [details] [diff] [review]
Patch 5

> newestNavigator
> .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> .getInterface(Components.interfaces.nsIDOMChromeWindow)
FYI this is a no-op.

Comment 42

[Boris Zbarsky [:bzbarsky]]

So it's OK if this code doesn't work in, say, remote XUL?

Comment 43

[Michael Ventnor]

(In reply to comment #41)
> (From update of attachment 254701 [details] [diff] [review])
> > newestNavigator
> > .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> > .getInterface(Components.interfaces.nsIDOMChromeWindow)
> FYI this is a no-op.
> 

What do you mean?

(In reply to comment #42)
> So it's OK if this code doesn't work in, say, remote XUL?
> 

This is all wrapped around a "try" block. If it doesn't work it will fall back to the method we've always been using for text links.

Comment 44

[[e-mail]]

(In reply to comment #43)
>(In reply to comment #41)
> > (From update of attachment 254701 [details] [diff] [review] [details])
> > > newestNavigator
> > > .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> > > .getInterface(Components.interfaces.nsIDOMChromeWindow)
> > FYI this is a no-op.
> What do you mean?
I mean that this window is already a chrome window.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> If it doesn't work it will fall back to the method we've always been using

Yes, I know that.  But the whole point is we don't like that method, which is why this bug exists.

If what we need are better APIs to do the right thing, then we should introduce said APIs instead of solving the problem only in certain cases.  Or we should decide that certain other cases don't matter and make that clear (e.g. disallow the use of this binding in those cases).

Comment 46

[Michael Ventnor]

(In reply to comment #45)
> > If it doesn't work it will fall back to the method we've always been using
> 
> Yes, I know that.  But the whole point is we don't like that method, which is
> why this bug exists.
> 
> If what we need are better APIs to do the right thing, then we should introduce
> said APIs instead of solving the problem only in certain cases.  Or we should
> decide that certain other cases don't matter and make that clear (e.g. disallow
> the use of this binding in those cases).
> 

Gavin said (comment 31) that this binding needs to be able to work in places where XPCOM is not available. This is why the try/catch was implemented in the first place. Those said "certain cases" where this bug will be fixed are cases where XPCOM can be used, since this bug basically requires XPCOM to be fixed (to the best of my knowledge, I'm sure you know more about the codebase than I do). I really don't see an alternative for the non-XPCOM cases.

I probably should have asked this beforehand. Can remote XUL handle XPCOM? Is there a browserDOMWindow available? If so, I'll change to the method Asaf proposed in comment 39.

Comment 47

[Michael Ventnor]

(In reply to comment #46)

> I probably should have asked this beforehand. Can remote XUL handle XPCOM? Is
> there a browserDOMWindow available? If so, I'll change to the method Asaf
> proposed in comment 39.
> 

Please ignore this. Gavin already answered this in comment 31 and the answer is no. Since XPCOM cannot be used I can't think of a way to handle that case without general window javascript. Can anyone else?

Comment 48

[Boris Zbarsky [:bzbarsky]]

> Can remote XUL handle XPCOM?

Yes, if it requests the permissions to do so (UniversalXPConnect).  So it may or may not be able to use XPCOM things depending on what the user decides and whether the site requests expanded privileges.

But see more below.

> Is there a browserDOMWindow available? 

In general, no.  For example, there's no such beastie in Camino, or in general XULRunner apps (there might be one, or there might not be one; kind of up to the app).  And those can _definitely_ do XPCOM stuff without too much trouble.

Hence my concern -- I'd like to have toolkit work in XULRunner apps in general.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Though come to think of it, the window.open() only fails to do the right thing in a docshell that has the nsChromeTreeOwner as its treeowner, doesn't it?  So only in a type="chrome" shell in a XULRunner app (doesn't happen in Camino, for example).

Given that, it's probably OK; we should just require XULRunner apps to implement nsIBrowserDOMWindow.

Comment 50

[Mark Finkle (:mfinkle) (use needinfo?)]

Happening in Fennec now too (bug 490070)

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sadly this is was getting lost for nearly 2 years. Michael, can we get an un-bitrotted patch so we can get progress on this bug?

Comment 53

[Mark Banner (:standard8)]

Just to comment that the patch here is now insufficient for Thunderbird. Whereas Thunderbird used to use the external protocol service, it now doesn't, and falling back to the old method is probably also inappropriate - Thunderbird could do with a decent hook where we can actually process the links and shove them externally or internally as we feel like.

Comment 54

[Axel Grude [:realRaven]]

Does anybody know what Postbox does in this case? It seems the linking to external apps is working there although its based on the same Mozilla version?

Comment 55

[Mark Banner (:standard8)]

(In reply to comment #54)
> Does anybody know what Postbox does in this case? It seems the linking to
> external apps is working there although its based on the same Mozilla version?

There are various ways around this, and most of the special Postbox UI wouldn't need this to be fixed.

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

I updated this to trunk, and removed the unnecessary "clean up" changes. I also added a call to window.focus(), since otherwise the window where the load occurred didn't necessarily get focus. Tested this with Fennec (with its workaround for this bug removed) and Firefox.

In reviewing this I happened to notice the alternative technique used by openURL in contentAreaUtils.js (which is what the extension manager "Get more themes" link uses). It's not particularly pretty, and I don't understand entirely how it works yet (the URI loader is a bit convoluted, so I haven't found where it actually finds a window to target), but it seems like we should probably investigate that further to avoid doing this too many different ways. It might also make things work for apps that don't use the "navigator:browser" convention.

Comment 57

[[e-mail]]

(In reply to comment #56)
> (the URI loader is a bit convoluted, so I haven't
> found where it actually finds a window to target)
That's actually done by the browser content handler, if the URL turns out to have a content type that the browser content handler admits to supporting. (Otherwise the URI loader opens the unknown content type dialog.)

Comment 58

[Mark Banner (:standard8)]

(In reply to comment #57)
> (In reply to comment #56)
> > (the URI loader is a bit convoluted, so I haven't
> > found where it actually finds a window to target)
> That's actually done by the browser content handler, if the URL turns out to
> have a content type that the browser content handler admits to supporting.
> (Otherwise the URI loader opens the unknown content type dialog.)

The main issue with the URI loader is that you a) have to be online for anything to happen when clicking the link (which potentially isn't too much of an issue), and b) have to wait for your app to load the URI, then pass it to the URI loader and then hand-off to wherever for it to be loaded a second time.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Second time?  Why is there a second time involved?  If there is, you're using the uriloader wrong.

Comment 60

[Mark Banner (:standard8)]

(In reply to comment #59)
> Second time?  Why is there a second time involved?  If there is, you're using
> the uriloader wrong.

Click Link -> uri is loaded/checked/something which afaict is a server access -> uri loader then hands off to browser content handler -> browser content handler then decides to load in an external application (e.g. Firefox) where the second load is required.

I guess if browser content handler passes back into the same application it may be different...

Comment 61

[Mark Banner (:standard8)]

Comment on attachment 423208 [details] [diff] [review]
updated patch

>+            var winMediator = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                                .getService(Components.interfaces.nsIWindowMediator);
>+            var browserWin = winMediator.getMostRecentWindow("navigator:browser");
...
>+              browserDOMWindow.openURI(uri, null,
>+                                       browserDOMWindow.OPEN_DEFAULTWINDOW,
>+                                       browserDOMWindow.OPEN_NEW);

Is there a way we could pref this bit? browser.chromeURL wouldn't be enough. However, allowing the app to decide on the most recent window type, i.e. not specific to "navigator:browser" and some detection of the openURI function would then allow this to be handled in a way that any app could hook into it without going via the uri loader.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> browser content handler then decides to load in an external application (e.g.
> Firefox) where the second load is required.

Ah, I see.  If you plan to hand off the _uri_, not the data, then uriloader is just the wrong thing to use.  uriloader is designed for data dispatch.

Comment 63

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 64

[Please Ignore This Troll (Account Disabled)]

the bug was forgotten?

Comment 65

[Alfred Kayser]

Bug 579547 - New About window for Firefox 4 adds a number of text-links which are not following the open-in-tab preference, making this bug more relevant.
Given that there is a patch (possibly bitrotted) available, why not fix this one?

Comment 66

[Mark Banner (:standard8)]

(In reply to comment #65)
> Bug 579547 - New About window for Firefox 4 adds a number of text-links which
> are not following the open-in-tab preference, making this bug more relevant.
> Given that there is a patch (possibly bitrotted) available, why not fix this
> one?

It needs a more complex patch that fixes it for everyone, and no-one has had time to do that yet.

Comment 69

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Mark Banner (:standard8) from comment #66)
> (In reply to comment #65)
> > Bug 579547 - New About window for Firefox 4 adds a number of text-links which
> > are not following the open-in-tab preference, making this bug more relevant.
> > Given that there is a patch (possibly bitrotted) available, why not fix this
> > one?
> 
> It needs a more complex patch that fixes it for everyone, and no-one has had
> time to do that yet.

I know it's been a while, but would you mind explaining in what ways the current patch fails to fix this bug for everyone? (I think you mention some issues in comments 60 and 61, but I don't quite follow what you're saying there.)

Also, as a side note, the patch applies just fine (and appears to fix the issue) on m-c.

Comment 70

[Dão Gottwald [:dao]]

Attachment: patch

This doesn't add anything Firefox-specific to text.xml but depends on the host application to do extra work if it wants to handle links in any special way.

Comment 71

[Dão Gottwald [:dao]]

Note that my patch:

- doesn't care for remote XUL. Is this still relevant to us?

- doesn't actually make text-links in Firefox "respect tabbed browsing preferences", it just lets them open a new tab unconditionally like e.g. menu items and buttons opening a web page and most text-links where we worked around this bug do.

Comment 72

[Dão Gottwald [:dao]]

Attachment: patch

minor cleanup

Comment 73

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Dão Gottwald [:dao] from comment #71)
> - doesn't actually make text-links in Firefox "respect tabbed browsing
> preferences", it just lets them open a new tab unconditionally like e.g.
> menu items and buttons opening a web page and most text-links where we
> worked around this bug do.

Wouldn't it make more sense to have it open a tab/window according to the user's preferences? Is there some technical reason that makes this difficult/undesirable?

Comment 74

[Dão Gottwald [:dao]]

The preference says "Open new windows in a new tab instead". Unchecking it doesn't exactly mean "Open new tabs in a new window instead" (i.e. never open new tabs), so we're not really violating it -- as opposed to opening a new window when "Open new windows in a new tab instead" is checked. Also, as I said, we don't care about that pref in many more places where the UI unconditionally loads pages in new tabs.

Comment 75

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Ah - I see. Sounds good.

Comment 76

[Neil Deakin]

Comment on attachment 691324 [details] [diff] [review]
patch

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -276,16 +276,26 @@ BrowserGlue.prototype = {
>         }
>         break;
>       case "initial-migration-will-import-default-bookmarks":
>         this._migrationImportsDefaultBookmarks = true;
>         break;
>       case "initial-migration-did-import-default-bookmarks":
>         this._initPlaces(true);
>         break;
>+      case "handle-xul-text-link":
>+        let linkHandled = subject.QueryInterface(Ci.nsISupportsPRBool);
>+        if (!linkHandled.data) {
>+          let win = this.getMostRecentBrowserWindow();
>+          if (win) {
>+            win.openUILinkIn(data, "tab");
>+            linkHandled.data = true;
>+          }
>+        }
>+        break;
>     }
>   }, 
> 
>   // initialization (called on application startup) 
>   _init: function BG__init() {
>     let os = Services.obs;
>     os.addObserver(this, "xpcom-shutdown", false);
>     os.addObserver(this, "prefservice:after-app-defaults", false);
>@@ -307,16 +317,17 @@ BrowserGlue.prototype = {
>     os.addObserver(this, "places-init-complete", false);
>     this._isPlacesInitObserver = true;
>     os.addObserver(this, "places-database-locked", false);
>     this._isPlacesLockedObserver = true;
>     os.addObserver(this, "distribution-customization-complete", false);
>     os.addObserver(this, "places-shutdown", false);
>     this._isPlacesShutdownObserver = true;
>     os.addObserver(this, "defaultURIFixup-using-keyword-pref", false);
>+    os.addObserver(this, "handle-xul-text-link", false);
>   },
> 
>   // cleanup (called on application shutdown)
>   _dispose: function BG__dispose() {
>     let os = Services.obs;
>     os.removeObserver(this, "xpcom-shutdown");
>     os.removeObserver(this, "prefservice:after-app-defaults");
>     os.removeObserver(this, "final-ui-startup");
>@@ -337,16 +348,17 @@ BrowserGlue.prototype = {
>       this._idleService.removeIdleObserver(this, BOOKMARKS_BACKUP_IDLE_TIME);
>     if (this._isPlacesInitObserver)
>       os.removeObserver(this, "places-init-complete");
>     if (this._isPlacesLockedObserver)
>       os.removeObserver(this, "places-database-locked");
>     if (this._isPlacesShutdownObserver)
>       os.removeObserver(this, "places-shutdown");
>     os.removeObserver(this, "defaultURIFixup-using-keyword-pref");
>+    os.removeObserver(this, "handle-xul-text-link");
>     UserAgentOverrides.uninit();
>     webappsUI.uninit();
>     SignInToWebsiteUX.uninit();
>     webrtcUI.uninit();
>   },
> 
>   _onAppDefaults: function BG__onAppDefaults() {
>     // apply distribution customizations (prefs)
>diff --git a/toolkit/content/widgets/text.xml b/toolkit/content/widgets/text.xml
>--- a/toolkit/content/widgets/text.xml
>+++ b/toolkit/content/widgets/text.xml
>@@ -345,29 +345,37 @@
>               return;
>             }
> 
>           }
>           catch (ex) {
>             Components.utils.reportError(ex);
>           }
> 
>+          aEvent.preventDefault();
>+          href = uri ? uri.spec : href;
>+
>+          // Try handing off the link to the host application, e.g. for
>+          // opening it in a tabbed browser.
>+          var linkHandled = Components.classes["@mozilla.org/supports-PRBool;1"]
>+                                      .createInstance(Components.interfaces.nsISupportsPRBool);
>+          linkHandled.data = false;
>+          Components.classes["@mozilla.org/observer-service;1"]
>+                    .getService(Components.interfaces.nsIObserverService)
>+                    .notifyObservers(linkHandled, "handle-xul-text-link", href);
>+          if (linkHandled.data)
>+            return;
>+
>           // otherwise, fall back to opening the anchor directly
>           var win = window;
>           if (window instanceof Components.interfaces.nsIDOMChromeWindow) {
>             while (win.opener && !win.opener.closed)
>               win = win.opener;
>           }
>-
>-          if (uri)
>-            win.open(uri.spec);
>-          else
>-            win.open(href);
>-
>-          aEvent.preventDefault();
>+          win.open(href);
>         ]]>
>         </body>
>       </method>
>     </implementation>
> 
>     <handlers>
>       <handler event="click" phase="capturing" button="0" action="this.open(event)"/>
>       <handler event="keypress" preventdefault="true" keycode="VK_ENTER"  action="this.click()" />

Comment 77

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb7e17d0e3d

Comment 78

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4cb7e17d0e3d

Comment 80

[tabmix.onemen]

The link open by the new code always opens in foreground tab
why don't you use here browser.tabs.loadDivertedInBackground

- win.openUILinkIn(data, "tab");
+ win.openLinkIn(data, "tab", {inBackground: Services.prefs.getBoolPref("browser.tabs.loadDivertedInBackground")});

Comment 81

[Dão Gottwald [:dao]]

(In reply to onemen.one from comment #80)
> The link open by the new code always opens in foreground tab
> why don't you use here browser.tabs.loadDivertedInBackground

Because these links aren't diverted like target=_blank content links. It makes less sense for UI links to be opened in the background for reading later or some such thing.

Comment 82

[tabmix.onemen]

User can't tell the different between all link types
Some expect this links to open in the background

Comment 83

[Mike Kaply [:mkaply]]

I realize this is a really old bug, but it caused some bad behavior for modal dialogs.

See bug 1227989.