Dão Gottwald [:dao] Reporter
	Description • 14 years ago

External links from within app tabs should always open in new tabs instead of replacing the app tab's page. It's not quite clear what makes a link "external". We could compare the domain, but this seems like it would miss cases.

	Tiago Sá
	Comment 2 • 14 years ago

I think we could compare the "folder" the page is in to begin with. For example, if you're at www.domain.com and change into www.domain.com/folder, all would be good, and back again. But if the AppTab was made inside the www.domain.com/folder and you change into www.domain.com/anotherfolder or even www.domain.com itself, it would open on a new tab. I think this is a solid approach.

As for stuff like Hotmail that accesses 10 different sub domains on logins and stuff, maybe we could allow redirects to open inside the app tab? If mail.google.com redirects me to google.com for a re-login or something, it shouldn't open on a new tab. But if I GO to google.com, it should open on a new tab. I think that's reasonable enough.

And, of course, if AppTabs don't have the navigation bar by default, you'll be hitting lots more troubles in terms of phising, but that's a given, right? Chromeless (by default) AppTabs is a bad idea...

	Tan Wei Lin Jason [:Ryuji]
	Comment 4 • 14 years ago

Comparing domain seems difficult as some website don't give simple url like
www.simple.com/ & www.simple/inbox. 

example hotmail:
Log in page: login.live.com/login.srf?....
Our latest Improvement page: sn126w.snt126.mail.live.com/mail/MessageAtLogin...
Inbox: sn126w.snt126.mail.live.com/default.aspx...

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 5 • 14 years ago

I think we could probably hit 80% by doing a strict domain match. It's obviously not the best thing, but it's probably "good enough" for now. We wouldn't do just TLD-1, but also subdomains.

mail.google.com stays on mail.google.com
google.com/reader can go anywhere on google.com

Accounting for things like redirects & login forms could get tricky, as Tiago points out :/

We'd also want this matching in URL bar navigations, but might be best to define the heuristic here & then fix the URL bar nav in a new bug.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 6 • 14 years ago

See https://wiki.mozilla.org/Firefox/Projects/App_Tabs - I think I might be wrong about the eTLD+1 thing. We also want to make sure we protect from www3.x.y load balancing sorta stuff. eTLD is likely what we want.

	Wyatt Childers
	Comment 7 • 14 years ago

Why don't we allow the link to stay in the app tab. But make the cotrolls pop up in mini form allowing the user to go back.

	Peter Henkel [:Terepin]
	Comment 8 • 14 years ago

We will, but only for links inside the same domain.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 9 • 14 years ago

Does this only cover links, or should we morph it to cover "any navigation event that takes you out of the domain of the application"?

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 10 • 14 years ago

(In reply to comment #9)> Does this only cover links, or should we morph it to cover "any navigation> event that takes you out of the domain of the application"?This bug specifically covers links. Bug 579874 (which this bug blocks) is a meta bug with dependencies for all of the paths (bookmarks, mailto:, etc).

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 12 • 14 years ago

(In reply to comment #5)
> I think we could probably hit 80% by doing a strict domain match. It's
> obviously not the best thing, but it's probably "good enough" for now. We
> wouldn't do just TLD-1, but also subdomains.
> 
> mail.google.com stays on mail.google.com
> google.com/reader can go anywhere on google.com

Yeah, I think that's fine.

> We'd also want this matching in URL bar navigations, but might be best to
> define the heuristic here & then fix the URL bar nav in a new bug.

Agreed.

	James "Kovu" Russell
	Comment 13 • 14 years ago

Just a potentially ignorant question regarding the above, can you sniff for anything that would result in changing the favicon?

Secondly, I was thinking about this concept but also thinking of internal links as well. Hear me out. As Web apps become more and more prevalent and professional, it becomes more and more of a problem to accidentally hit a link and lose your current page. I am a writer for Demand Studios (populates eHow.com and other sites) and have had times when I'm writing an article and (using a laptop and its stupid "pad") accidentally clicked a link on the page and "poof" my story is gone. This is not acceptable. If this is not something that would make sense in this bug, perhaps I should start an RFE asking for an option to disable/reenable link behavior altogether on a given tab?

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 14 • 14 years ago

(In reply to comment #13)
> Just a potentially ignorant question regarding the above, can you sniff for
> anything that would result in changing the favicon?

Not without potentially making requests.

> Secondly, I was thinking about this concept but also thinking of internal links
> as well. Hear me out. As Web apps become more and more prevalent and
> professional, it becomes more and more of a problem to accidentally hit a link
> and lose your current page. I am a writer for Demand Studios (populates
> eHow.com and other sites) and have had times when I'm writing an article and
> (using a laptop and its stupid "pad") accidentally clicked a link on the page
> and "poof" my story is gone. This is not acceptable.

As much as possible, it should be the job of the site to set target="_blank". Sites like gmail, twitter & google reader make good app tabs currently because all of the links already target "_blank". In an ideal world all web sites that want to behave like apps should do this. We're going to target "good enough without having super complicated heuristics". If you have a problem with a specific website or (in your case) CMS vendor, you should contact the developers of that product.

Often times (but not always) hitting the back button is good enough for your case. We do try to cache form data like that. That's something we as a browser vendor can try to improve. If you'd like to file a bug for that with details, please do.

> If this is not something
> that would make sense in this bug, perhaps I should start an RFE asking for an
> option to disable/reenable link behavior altogether on a given tab?

That would be a different bug. But I'd guess that gets WONTFIXed pretty quickly. IMO that's not something the core of a browser should be responsible for.

	:Margaret Leibovic Assignee
	Comment 15 • 14 years ago

This patch works. Who should review it?

	:Margaret Leibovic Assignee
	Comment 16 • 14 years ago

When playing around with the patch applied, I also just noticed that searches from the search box will open in a new tab if the search service has a different eTLD from the app tab. I feel like this is what we want anyway, so maybe we're killing two birds with one patch?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 17 • 14 years ago

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

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>@@ -3922,17 +3922,18 @@ var XULBrowserWindow = {

>   QueryInterface: function (aIID) {

>-        aIID.equals(Ci.nsISupports))
>+        aIID.equals(Ci.nsISupports) ||
>+        aIID.equals(Ci.nsIWebBrowserChrome3))

Don't do this (it's wrong, since we don't implement the rest of nsIWebBrowserChrome*). Instead, add onBeforeLinkTraversal to nsIXULBrowserWindow, and have nsContentTreeOwner::OnBeforeLinkTraversal call it through that (without the QI).

>+  onBeforeLinkTraversal: function(originalTarget, linkURI, anchorElmt) {
>+    if (!this.eTLDService)
>+      this.eTLDService = Cc["@mozilla.org/network/effective-tld-service;1"]
>+                         .getService(Ci.nsIEffectiveTLDService);

Should probably just add this to Services.jsm.

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp

> #include "nsIWebBrowserChrome2.h"
>+#include "nsIWebBrowserChrome3.h"

Should be able to remove the 2. 

>+  nsCOMPtr<nsIWebBrowserChrome3> browserChrome3 = do_GetInterface(mTreeOwner);
>+  if (!browserChrome3) {
>+    return NS_ERROR_FAILURE;

You shouldn't return an error in this case - that will break embedders that don't implement that interface, and also Fennec since its mTreeOwners are TabChilds. Just do if (browserChrome3) browserChrome3->OnBeforeLinkTraversal.

Looks good otherwise! bz should review the core bits, I can review the Firefox changes.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 18 • 14 years ago

(In reply to comment #16)
> When playing around with the patch applied, I also just noticed that searches
> from the search box will open in a new tab if the search service has a
> different eTLD from the app tab. I feel like this is what we want anyway, so
> maybe we're killing two birds with one patch?

Er, that's odd. The search bar should just be calling browser.loadURI, which shouldn't go through the code this patch touches... Need to investigate that, I think.

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 19 • 14 years ago

(In reply to comment #18)
> (In reply to comment #16)
> > When playing around with the patch applied, I also just noticed that searches
> > from the search box will open in a new tab if the search service has a
> > different eTLD from the app tab. I feel like this is what we want anyway, so
> > maybe we're killing two birds with one patch?
> 
> Er, that's odd. The search bar should just be calling browser.loadURI, which
> shouldn't go through the code this patch touches... Need to investigate that, I
> think.

That was actually done independently, bug 579872 I think.

	:Margaret Leibovic Assignee
	Comment 20 • 14 years ago

Addressed gavin's comments.

	:Margaret Leibovic Assignee
	Comment 21 • 14 years ago

Oops! Left in some debugging code.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 14 years ago

Comment on attachment 484929 [details] [diff] [review]
patch v2 (for real)

>+++ b/docshell/base/nsDocShell.cpp

>+    nsCOMPtr<nsIDOMHTMLAnchorElement> anchorElmt = do_QueryInterface(aContent);

This could be null (e.g. aContent might be <isindex> or <embed> or <object>), in which case your chrome code breaks, I think.  I'd prefer if we just made the new API take a nsIDOMNode, since that's all you need out of it, and we can guarantee having one of those here.

>+    nsAutoString newTarget;
>+    browserChrome3->OnBeforeLinkTraversal(nsDependentString(aTargetSpec), aURI,
>+                                          anchorElmt, newTarget);

aTargetSpec can be null (and is, for <isindex>).  So you can't use nsDependentString.

I'd prefer something more like this:

  nsAutoString target;
  if (browserChrome3) {
    // copy aTargetSpec into a new nsAutoString, then call OnBeforeLinkTraversal.
  } else {
    target = aTagetSpec;
  }

and then just have one call to OnLinkClickEvent using target.get().  Or even change the event constructor to take a const nsString&, since it ends up storing into one anyway.

>+++ b/xpfe/appshell/public/nsIXULBrowserWindow.idl

You need to change the iid here.

>+++ b/xpfe/appshell/src/nsContentTreeOwner.cpp
>+NS_IMETHODIMP nsContentTreeOwner::OnBeforeLinkTraversal(const nsAString &originalTarget,

If !xulBrowserWindow (or if the call to it fails), this doesn't return originalTarget in _retval.  Not only that, but it leaves the caller with no way to handle this....

We should probably make this propagate errors from xulBrowserWindow, set _retval to originalTarget if !xulBrowserWindow, and make the docshell code handle failure by using the original target.

r-, but this looks like it's almost there!

	:Margaret Leibovic Assignee
	Comment 23 • 14 years ago

bz, I'm not sure I understood exactly what you meant about propagating errors from xulBrowserWindow. I just set _retval to originalTarget if !xulBrowserWindow. Is that enough, or is there something else I should be doing?

	:Margaret Leibovic Assignee
	Comment 24 • 14 years ago

Gavin answered my last question on IRC. Let me know if this works.

	Dão Gottwald [:dao] Reporter
	Comment 25 • 14 years ago

Comment on attachment 484978 [details] [diff] [review]
patch v4

How about Services.eTLD rather than Services.eTLDService?

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 14 years ago

Comment on attachment 484978 [details] [diff] [review]
patch v4

>+    nsAutoString newTarget(aTargetSpec);

I'd call that oldTarget, not newTarget...

The rest looks great.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 14 years ago

Given the nsIXULBrowserWindow change, btw, this should probably block b7.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 28 • 14 years ago

Comment on attachment 484978 [details] [diff] [review]
patch v4

A couple of comments:
- I agree with dao re: Services.eTLD
- we should probably only override the target if it's ""
- you should use tab.linkedBrowser.currentURI rather than window.location.host
- not strictly necessary, but wouldn't hurt to update the other nsIXULBrowserWindow implementations in toolkit/content/tests/chrome/findbar_window.xul and toolkit/components/help/content/help.js (just add stub function)

It would be really nice to avoid having to call _getTabForContentWindow for every link click... A docshell "appTab" annotation would allow that, because it could pass that state in to onBeforeLinkTraversal as a boolean and we could short-circuit in those cases. That'd require a signature change, though...

	:Margaret Leibovic Assignee
	Comment 29 • 14 years ago

I addressed all of gavin's comments, including adding a docshell "appTab" annotation like he suggested.

After implementing this I realized that maybe I should have called the boolean "mIsPinnedTab" instead of "mIsAppTab" to be more consistent with tabbrowser.xml. Let me know if I should change that (and the method names, of course).

	:Margaret Leibovic Assignee
	Comment 30 • 14 years ago

Responded to some IRC comments from gavin.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 31 • 14 years ago

Comment on attachment 485410 [details] [diff] [review]
patch v6

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

Couple of issues I noticed with testing the frontend parts using http://g4v.org/app.html:
- you need to pass 0, not 1, to getBaseDomain (the hosts might not have enough parts, as in my example)
- the getBaseDomain calls should probably be in a try/catch (falling back to just returning originalTarget), since getBaseDomain can fail for e.g. IP addresses
- you can use docURI = linkNode.ownerDocument.documentURIObject; to avoid calling _getTabForContentWindow
- nits: I'd use local variables for the base domains, and there's some trailing whitespace

This does bring up the question of what we want the behavior for subframes to be. I think maybe we don't actually want to propagate isAppTab to children docShells... I could see "apps" framing third party content wanting link clicks inside frames to navigate normally (which wouldn't normally replace the app tab).

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 14 years ago

I agree that isAppTab probably shouldn't inherit down (but see below).  In fact, I'd say it should throw if you try to set it on a non-root-of-same-type docshell...  Gavin, thoughts?  Again, see below for where this might matter.

>+    if (originalTarget != "" || !isAppTab)
>+      return originalTarget;

This will miss cases, I think.  If we assume all app tabs are roots of their type (which I think is safe to assume in this code) and that they're all targetable docshells (also safe, I think), then the following nonempty names will navigate the tab itself: "_self", "_parent", "_top", "_main", "_content", linkNode.ownerDocument.defaultView.name.  If someone's doing one of those, do we want to allow it to navigate the tab?  I'd assume no except _maybe_ for that last one.

Of the list above, everything except "_self" could navigate the app tab even if the actual link is in the subframe (well, the .name would need to be the one for defaultView.top).  How do we want to handle that situation?

Perhaps the right thing to do is to flag all the subshells as being inside an app tab and do some tests to compare against defaultView.top as needed....

isAppTab should go at the end of nsIDocShell, I think.

Followup bug to add the app tab boolean to nsIWebBrowser stuff too?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 33 • 14 years ago

If links are explicitly targeted to the current window (or to the containing window, in the frame case), then I think we just want to leave them alone. Just catching the common case of untargeted links should be fine.

	:Margaret Leibovic Assignee
	Comment 34 • 14 years ago

I agree with Gavin that links with an explicit target should be left alone.

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 14 years ago

Comment on attachment 485839 [details] [diff] [review]
patch v7

r=me

	JK
	Comment 36 • 14 years ago

Wouldn't a tag which website developers could use to define what URLs open in the app tab? Like

<meta name="apptab" content="http://www.google.com/*" />

to make all Google links open in the apptab.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 37 • 14 years ago

(In reply to comment #36)
> Wouldn't a tag which website developers could use to define what URLs open in
> the app tab? Like

Such a specification might indeed be useful, but that's not within the scope of this bug which is to define the default behaviour in the absence of that sort of specification. Web authors can get most of the way there through window naming and link targetting, fwiw.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 38 • 14 years ago

Comment on attachment 485839 [details] [diff] [review]
patch v7

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  onBeforeLinkTraversal: function(originalTarget, linkURI, linkNode, isAppTab) {
>+    if (originalTarget != "" || !isAppTab)
>+      return originalTarget;

Would be nice to add a "Called before links are navigated to to allow us to retarget them if needed" comment, and a comment making it clear exactly what cases we're ignoring with that return (including "isAppTab will be false for app tab subframes" - if we change that we'd need to add a linkWindow.top != linkWindow check).

Could you add a test for eTLD to browser_Services.js (and any missing ones while you're at it?)

I'd put the isAppTab setting before the event firing in (un)pinTab in tabbrowser.xml.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 39 • 14 years ago

Tests for the browser-specific behavior (and maybe even for the core support in general) would be good, too.

	Siddhartha Dugar [:sdrocking]
	Comment 40 • 14 years ago

Should home button also open homepage in a new tab?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 41 • 14 years ago

(In reply to comment #40)
> Should home button also open homepage in a new tab?

Yes, but I believe that's being covered in a different bug. This is just about links, as per the summary.

	eyal gruss
	Comment 42 • 14 years ago

that would be Bug 607089

	:Margaret Leibovic Assignee
	Comment 44 • 14 years ago

Updated to address review comments.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 45 • 14 years ago

Comment on attachment 486654 [details] [diff] [review]
tests

>diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in

>+                 app.html \
>+                 app_subframe.html \

A less generic name for these would be good (include the bug #, or "beforeLinkHandling", or somesuch)

>diff --git a/browser/base/content/test/app.html b/browser/base/content/test/app.html

>+  <body>
>+    <a href="http://example.com/browser/browser/base/content/test/dummy_page.html">link0</a>
>+    <a href="http://test1.example.com/browser/browser/base/content/test/dummy_page.html">link1</a>
>+    <a href="http://example.org/browser/browser/base/content/test/dummy_page.html">link2</a>
>+    <a href="http://example.org/browser/browser/base/content/test/dummy_page.html" target="foo">link3</a>

Wouldn't hurt to give these descriptive labels, rather than "linkN".

>diff --git a/browser/base/content/test/browser_bug575561.js b/browser/base/content/test/browser_bug575561.js

>+function cleanUp() {
>+  // Remove any extra tabs leftover from tests
>+  for (let i = 0; i < gBrowser.tabs.length; i++) {
>+    gBrowser.removeTab(gBrowser.tabs[i]);
>+  }

This will close all tabs, which would break things when running the full suite presumably. You should only close tabs that you open.

>+function test_link0() {

>+  function onLoad(event) {

>+    // Ignore load events from iframe
>+    let subframe = browser.contentDocument.getElementsByTagName("iframe")[0];
>+    if (subframe && event.target == subframe.contentDocument)
>+      return;

I think you should instead keep a counter and only proceed once you get both load events - otherwise you risk starting the test before the page and the subframe have both loaded.

>+    let links = browser.contentDocument.getElementsByTagName("a");
>+    appTab.linkedBrowser.addEventListener("load", function(){
>+      is(browser.contentDocument.location.href, links[0].href, "Link to the same domain should not open in a new tab");

browser.currentURI.spec is a simpler check.

You could share some code with a clickContentLink(index) method. Also it seems like the test would be simpler if you only loaded the app tab once, and ran all the tests on that (you can make the links point to the page itself or just use goBack() for the same-tab cases). Also adding a test that tests that pinning/unpinning the same tab and clicking the same link behaves differently would be good.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 46 • 14 years ago

Comment on attachment 486656 [details] [diff] [review]
patch v8

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  onBeforeLinkTraversal: function(originalTarget, linkURI, linkNode, isAppTab) {
>+    // Don't modify targets that are explicitly set by devlopers or targets

nit: remove "by devlopers". And perhaps phrase it as "non-default targets"?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 48 • 14 years ago

Comment on attachment 486742 [details] [diff] [review]
tests v2

testLink could call info("Clicking " + links[aLinkIndex].textContent) to help with debugging if this ever ends up failing.

	:Margaret Leibovic Assignee
	Comment 49 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/ecd1948992eb
http://hg.mozilla.org/mozilla-central/rev/069ccd107c32

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 50 • 14 years ago

Backed out on suspicion of causing test failures, see
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288313672.1288315248.32386.gz

That failure has happened on all three Win7opt test runs since the checkin.

	:Margaret Leibovic Assignee
	Comment 52 • 14 years ago

The test failures were caused by an aero peek issue (bug 608306). Adding executeSoon calls to my tests acts as a workaround.

	:Margaret Leibovic Assignee
	Comment 53 • 14 years ago

Tree is green!

http://hg.mozilla.org/mozilla-central/rev/b642534688f8
http://hg.mozilla.org/mozilla-central/rev/91a749304ec9

	Siddhartha Dugar [:sdrocking]
	Comment 54 • 14 years ago

On right-clicking a bookmark with an app tab open, the context menu shows "Open" in bold though the default action is "Open in a New Tab". This should be fixed.

	Siddhartha Dugar [:sdrocking]
	Comment 55 • 14 years ago

After session restore, if a link fails to load then the go button takes it to a new tab. This is trivial and perhaps cannot be helped.