937569 - Peek tab doesn't work for links with new targets

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Jim Mathies [:jimm]]

Attachment: linktarget.html

See the test case. Links with new targets get injected into new tabs without the user's knowledge. Not sure if this is by design or if it's a bug, I think we should be doing a tab peek here?

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Seems like this nsBrowserAccess thing should be over with BrowserUI, but I'm not sure about the history here.  

- I cleaned up the code a bit, and removed some stuff related to apps.
- I'm concerned about removing that call to BrowserUI.showContent() but it doesn't seem to break anything. It was calling ContextUI.dismissTabs so I had to remove it.

Comment 2

[Matt Brubeck (:mbrubeck)]

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

Review of attachment 830812 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/browser-ui.js
@@ +440,5 @@
>      return Browser.addTab(aURI || kStartURI, true, aOwner);
>    },
>  
> +  peekTabs: function () {
> +    ContextUI.peekTabs(kNewTabAnimationDelayMsec);

ContextUI.peekTabs already uses kNewTabAnimationDelayMsec if you don't pass in a value, so I don't feel the need to add this alias.

Also, for this use case it should be kOpenInNewTabAnimationDelayMsec.

::: browser/metro/base/content/browser.js
@@ +1100,5 @@
>    openURI: function browser_openURI(aURI, aOpener, aWhere, aContext) {
> +    let where = this._getOpenAction(aURI, aOpener, aWhere, aContext);
> +    // Link clicks in content need to trigger peek tab functionality
> +    if (where == Ci.nsIBrowserDOMWindow.OPEN_NEWTAB) {
> +      BrowserUI.peekTabs();

I'd like to put the peekTabs call next to the addTab call, in _getBrowser.

I'll be refactoring this a bit in bug 888353, but I'm happy to rebase on top of this if you land first.  After bug 888353 lands, this code can call BrowserUI.openLinkInNewTab which will call both addTab and peekTabs.

Comment 3

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/0c50caf7ab6a

updated per comments.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c50caf7ab6a