External links from within app tabs should always open in new tabs instead of replacing the app tab's page

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: dao, Assigned: Margaret)

Tracking

(Blocks: 3 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

8 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.

Updated

8 years ago
Duplicate of this bug: 577042

Comment 2

8 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...

Updated

8 years ago
Blocks: 579874
(Reporter)

Updated

8 years ago
Summary: External links from within app tabs should always open in new tabs instead of replacing the app tab's page. → External links from within app tabs should always open in new tabs instead of replacing the app tab's page
(Reporter)

Updated

8 years ago
Duplicate of this bug: 580832
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...
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.

Updated

8 years ago
blocking2.0: --- → ?
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.
blocking2.0: ? → betaN+

Comment 7

7 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.
We will, but only for links inside the same domain.
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"?
(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).
(Assignee)

Updated

7 years ago
Assignee: nobody → margaret.leibovic
Duplicate of this bug: 598587
(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.
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?
(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.
(Assignee)

Comment 15

7 years ago
Created attachment 484895 [details] [diff] [review]
patch

This patch works. Who should review it?
Attachment #484895 - Flags: feedback?(paul)
Attachment #484895 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 16

7 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?
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.
Attachment #484895 - Flags: feedback?(gavin.sharp) → feedback-
(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.
(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.
(Assignee)

Comment 20

7 years ago
Created attachment 484927 [details] [diff] [review]
patch v2

Addressed gavin's comments.
Attachment #484895 - Attachment is obsolete: true
Attachment #484927 - Flags: review?(gavin.sharp)
Attachment #484927 - Flags: review?(bzbarsky)
Attachment #484895 - Flags: feedback?(paul)
(Assignee)

Comment 21

7 years ago
Created attachment 484929 [details] [diff] [review]
patch v2 (for real)

Oops! Left in some debugging code.
Attachment #484927 - Attachment is obsolete: true
Attachment #484929 - Flags: review?(gavin.sharp)
Attachment #484929 - Flags: review?(bzbarsky)
Attachment #484927 - Flags: review?(gavin.sharp)
Attachment #484927 - Flags: review?(bzbarsky)
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!
Attachment #484929 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 23

7 years ago
Created attachment 484975 [details] [diff] [review]
patch v3

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?
Attachment #484929 - Attachment is obsolete: true
Attachment #484975 - Flags: review?(gavin.sharp)
Attachment #484975 - Flags: review?(bzbarsky)
Attachment #484929 - Flags: review?(gavin.sharp)
(Assignee)

Comment 24

7 years ago
Created attachment 484978 [details] [diff] [review]
patch v4

Gavin answered my last question on IRC. Let me know if this works.
Attachment #484975 - Attachment is obsolete: true
Attachment #484978 - Flags: review?(gavin.sharp)
Attachment #484978 - Flags: review?(bzbarsky)
Attachment #484975 - Flags: review?(gavin.sharp)
Attachment #484975 - Flags: review?(bzbarsky)
(Reporter)

Comment 25

7 years ago
Comment on attachment 484978 [details] [diff] [review]
patch v4

How about Services.eTLD rather than Services.eTLDService?
Comment on attachment 484978 [details] [diff] [review]
patch v4

>+    nsAutoString newTarget(aTargetSpec);

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

The rest looks great.
Attachment #484978 - Flags: review?(bzbarsky) → review+
Given the nsIXULBrowserWindow change, btw, this should probably block b7.
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...
(Assignee)

Comment 29

7 years ago
Created attachment 485401 [details] [diff] [review]
patch v5

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).
Attachment #484978 - Attachment is obsolete: true
Attachment #485401 - Flags: review?(gavin.sharp)
Attachment #485401 - Flags: review?(bzbarsky)
Attachment #484978 - Flags: review?(gavin.sharp)
(Assignee)

Comment 30

7 years ago
Created attachment 485410 [details] [diff] [review]
patch v6

Responded to some IRC comments from gavin.
Attachment #485401 - Attachment is obsolete: true
Attachment #485410 - Flags: review?(gavin.sharp)
Attachment #485410 - Flags: review?(bzbarsky)
Attachment #485401 - Flags: review?(gavin.sharp)
Attachment #485401 - Flags: review?(bzbarsky)
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).
Attachment #485410 - Flags: review?(gavin.sharp) → review-
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?
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.
(Assignee)

Comment 34

7 years ago
Created attachment 485839 [details] [diff] [review]
patch v7

I agree with Gavin that links with an explicit target should be left alone.
Attachment #485410 - Attachment is obsolete: true
Attachment #485839 - Flags: review?(gavin.sharp)
Attachment #485839 - Flags: review?(bzbarsky)
Attachment #485410 - Flags: review?(bzbarsky)
Comment on attachment 485839 [details] [diff] [review]
patch v7

r=me
Attachment #485839 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review gavin]
blocking2.0: betaN+ → beta7+

Comment 36

7 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.
(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.
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.
Attachment #485839 - Flags: review?(gavin.sharp) → review+
Tests for the browser-specific behavior (and maybe even for the core support in general) would be good, too.
Whiteboard: [needs review gavin]
Should home button also open homepage in a new tab?
(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.

Comment 42

7 years ago
that would be Bug 607089
Whiteboard: [needs updated patch]
(Assignee)

Comment 43

7 years ago
Created attachment 486654 [details] [diff] [review]
tests
Attachment #486654 - Flags: review?(gavin.sharp)
(Assignee)

Comment 44

7 years ago
Created attachment 486656 [details] [diff] [review]
patch v8

Updated to address review comments.
Attachment #485839 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [needs updated patch] → [needs review gavin]
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.
Attachment #486654 - Flags: review?(gavin.sharp) → review-
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"?
(Assignee)

Comment 47

7 years ago
Created attachment 486742 [details] [diff] [review]
tests v2
Attachment #486654 - Attachment is obsolete: true
Attachment #486742 - Flags: review?(gavin.sharp)
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.
Attachment #486742 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 49

7 years ago
http://hg.mozilla.org/mozilla-central/rev/ecd1948992eb
http://hg.mozilla.org/mozilla-central/rev/069ccd107c32
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [needs review gavin]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 608189
(Assignee)

Comment 52

7 years ago
Created attachment 486953 [details] [diff] [review]
tests v3

The test failures were caused by an aero peek issue (bug 608306). Adding executeSoon calls to my tests acts as a workaround.
Attachment #486742 - Attachment is obsolete: true
(Assignee)

Comment 53

7 years ago
Tree is green!

http://hg.mozilla.org/mozilla-central/rev/b642534688f8
http://hg.mozilla.org/mozilla-central/rev/91a749304ec9
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
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.
(Assignee)

Updated

7 years ago
Depends on: 608791

Updated

7 years ago
Blocks: 609034
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.

Updated

7 years ago
Depends on: 615866
You need to log in before you can comment on or make changes to this bug.