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

RESOLVED FIXED in mozilla20

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
13 years ago
2 months ago

People

(Reporter: Ger Teunis, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

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

13 years ago
Do you mean the popup with the "checking for update"? That is no window, that is
a dialog.
(Reporter)

Comment 2

13 years ago
(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.
Summary: Links in Firefox Update window should repect Tabbed Browsing preferences → Links in Firefox Update dialog should repect Tabbed Browsing preferences

Comment 3

13 years ago
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?
(Reporter)

Comment 4

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

13 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

12 years ago
(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].
*** Bug 298849 has been marked as a duplicate of this bug. ***
Component: Extension/Theme Manager → Software Update
Summary: Links in Firefox Update dialog should repect Tabbed Browsing preferences → Links in Software Update dialog should repect Tabbed Browsing preferences
I adjusted the summary to mention software update so hopefully further dupes can
be avoided.

Comment 9

12 years ago
This bug exists with the new software update system as well.  Adding to meta bug
for software update UI improvements.
Blocks: 292163
Summary: Links in Software Update dialog should repect Tabbed Browsing preferences → Links in Software Update dialog should respect Tabbed Browsing preferences
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.
Component: Software Update → XUL Widgets
OS: Windows XP → All
Product: Firefox → Toolkit
Hardware: PC → All
Summary: Links in Software Update dialog should respect Tabbed Browsing preferences → 'text-link' xul widget class does not respect tabbed browsing preferences
*** Bug 294516 has been marked as a duplicate of this bug. ***

Comment 12

11 years ago
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
(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

11 years ago
(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?
(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.

Updated

11 years ago
Depends on: 262575
No longer depends on: 262575
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.
(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.
(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
(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.
Assignee: bugs → nobody
QA Contact: bugs → xul.widgets

Comment 20

11 years ago
(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

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

11 years ago
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?
Created attachment 229761 [details] [diff] [review]
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

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

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

Comment 26

11 years ago
(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

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

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

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

Updated

11 years ago
Blocks: 348120
Blocks: 349309

Comment 30

11 years ago
Created attachment 250043 [details] [diff] [review]
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. :)
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #250043 - Flags: first-review?(gavin.sharp)
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.
Attachment #250043 - Flags: first-review?(gavin.sharp) → first-review-

Comment 32

11 years ago
Created attachment 250414 [details] [diff] [review]
Patch 2

I can't see that bug, I get Access Denied. But I think I know what you mean.
Attachment #250043 - Attachment is obsolete: true
Attachment #250414 - Flags: first-review?(gavin.sharp)
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).
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

11 years ago
Created attachment 250436 [details] [diff] [review]
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.
Attachment #250414 - Attachment is obsolete: true
Attachment #250436 - Flags: first-review?(gavin.sharp)
Attachment #250414 - Flags: first-review?(gavin.sharp)
(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

11 years ago
Created attachment 250678 [details] [diff] [review]
Patch 4

That's true. I guess I worry too much about code size.
Attachment #250436 - Attachment is obsolete: true
Attachment #250678 - Flags: first-review?(gavin.sharp)
Attachment #250436 - Flags: first-review?(gavin.sharp)

Comment 38

11 years ago
Created attachment 254701 [details] [diff] [review]
Patch 5

I found a method which doesn't use any constant numbers so this should be completely app-agnostic.
Attachment #250678 - Attachment is obsolete: true
Attachment #254701 - Flags: first-review?(mano)
Attachment #250678 - Flags: first-review?(gavin.sharp)
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).
Attachment #254701 - Flags: first-review?(mano)

Comment 40

11 years ago
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).
Attachment #254701 - Flags: first-review?(gavin.sharp)

Comment 41

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

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

Comment 43

11 years ago
(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

11 years ago
(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

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

11 years ago
(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

11 years ago
(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

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

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

Updated

10 years ago
Attachment #254701 - Flags: first-review?(gavin.sharp) → review?(gavin.sharp)

Updated

10 years ago
No longer blocks: 349309
Depends on: 384139

Updated

9 years ago
Blocks: 450642
Blocks: 456916
No longer blocks: 292163
Happening in Fennec now too (bug 490070)
Blocks: 490070

Updated

8 years ago
Blocks: 497758
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?
Version: unspecified → Trunk

Updated

8 years ago
Duplicate of this bug: 497758

Updated

8 years ago
Whiteboard: [geo]
tracking-fennec: --- → 1.0-wm+
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.
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?
(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.
Created attachment 423208 [details] [diff] [review]
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.
Attachment #229761 - Attachment is obsolete: true
Attachment #254701 - Attachment is obsolete: true
Attachment #254701 - Flags: review?(gavin.sharp)
No longer blocks: 456916
No longer blocks: 497758
Assignee: ventnor.bugzilla → nobody
Severity: minor → normal
Whiteboard: [geo]
No longer blocks: 348120
(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.)
(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

8 years ago
Second time?  Why is there a second time involved?  If there is, you're using the uriloader wrong.
(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 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

8 years ago
> 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.
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.
Status: ASSIGNED → NEW
the bug was forgotten?

Comment 65

7 years ago
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?
Blocks: 579547
(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.
tracking-fennec: 1.0-wm+ → 2.0+
tracking-fennec: 2.0+ → ---
Duplicate of this bug: 626322
Blocks: 637552
(Assignee)

Updated

5 years ago
Duplicate of this bug: 797976
(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.
(Assignee)

Comment 70

5 years ago
Created attachment 691156 [details] [diff] [review]
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #691156 - Flags: review?(enndeakin)
(Assignee)

Updated

5 years ago
Blocks: 820109
(Assignee)

Comment 71

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

Comment 72

5 years ago
Created attachment 691324 [details] [diff] [review]
patch

minor cleanup
Attachment #691156 - Attachment is obsolete: true
Attachment #691156 - Flags: review?(enndeakin)
Attachment #691324 - Flags: review?(enndeakin)
(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?
(Assignee)

Comment 74

5 years ago
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.
Ah - I see. Sounds good.
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()" />
Attachment #691324 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 77

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb7e17d0e3d
Summary: 'text-link' xul widget class does not respect tabbed browsing preferences → 'text-link' XUL widget should open tabs rather than windows
(Assignee)

Updated

5 years ago
Attachment #423208 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4cb7e17d0e3d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 532567
(Assignee)

Updated

5 years ago
Duplicate of this bug: 661120

Updated

5 years ago
Depends on: 844466

Comment 80

5 years ago
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")});
(Assignee)

Comment 81

5 years ago
(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

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

Updated

2 months ago
Depends on: 1227989

Comment 83

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

See bug 1227989.
You need to log in before you can comment on or make changes to this bug.