1396988 - [OOP] onBeforeLinkTraversal is not handled correctly for subframes

Status: NEW

(Firefox :: Tabbed Browser, defect, P2)

Description

[Shane Caraveo (:mixedpuppy)]

While digging into bug 1392210 further, I was using a pinned tab for behavior comparison.  I noted that placing google into a pinned tab, doing a search, then clicking a link result replaces the pinned tab rather than opening in a new tab.

This is due to the use of iframes.  OnBeforeLinkTraversal receives isAppTab for the docshell that received the click.  tabs (and extension panels) need to use the isAppTab value for the top level frame.  Chatting with bz on irc, it's not clear what docshell itself should send, but it's easier (and gives a choice of behavior) to just handle this in OnBeforeLinkTraversal.

I'll address extension panels in bug 1392210.  This is for the general tab handling, someone should make a determination of the correct behavior for (pinned) tabs.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Meant to put this under Firefox originally since that's where the UX issue and code fix would be.

[Tracking Requested - why for this release]: this does affect 57, and e10s in general.

Comment 2

[Julien Cristau [:jcristau]]

Guessing this is wontfix for 56.  Who should make that determination for 57?

Comment 3

[Dão Gottwald [:dao]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> While digging into bug 1392210 further, I was using a pinned tab for
> behavior comparison.  I noted that placing google into a pinned tab, doing a
> search, then clicking a link result replaces the pinned tab rather than
> opening in a new tab.
> 
> This is due to the use of iframes.

Google uses iframes in the search results page? That would be surprising. My understanding is that search result links actually go to the same google domain and redirect to the destination from there, so onBeforeLinkTraversal can't do anything about this.

(In reply to Julien Cristau [:jcristau] from comment #2)
> Guessing this is wontfix for 56.  Who should make that determination for 57?

Maybe I'm missing some context, but it sounds like this isn't a recent regression and as such doesn't need to be a priority for 57.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Yeah, google is doing something different when in a tab, and I'm not certain it is solvable.  But this particular issue is more generic.

Given a url to a page that loads a same-origin page in an iframe:

a.com/page.html

<iframe src="/framed.html">

a.com/framed.html

<a href="http://b.com/linked.html">

The onBeforeLinkTraversal code ends up using isAppTab for the iframe rather than the top level docshell.  So rather than popping the linked page out into a tab, it replaces content in the iframe.  

For webextensions it was appropriate to have that pop out into a tab.  It may or may not be for a regular pinned page.

The "fix" is rather simple:

browser/base/content/tab-content.js

 var WebBrowserChrome = {
   onBeforeLinkTraversal(originalTarget, linkURI, linkNode, isAppTab) {
-    return BrowserUtils.onBeforeLinkTraversal(originalTarget, linkURI, linkNode, isAppTab);
+    return BrowserUtils.onBeforeLinkTraversal(originalTarget, linkURI, linkNode, docShell.isAppTab);
   },