416771 - Allow window.focus() to switch tabs at times when popups are allowed

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase showing inconsistency between tabs and windows

Many sites have links or buttons that open popups, and avoid opening duplicates by calling focus() if the requested window already exists.  But Firefox ignores window.focus() for tabs, which makes the links seem like they're broken if you forget you already have the window open.  This is even more confusing when sites reuse windows to load different content.

Firefox's current behavior is the result of the drastic fix for bug 310825.  I think we should do something more like what we now do for top-level windows (see bug 355482).  That is, allow window.focus() to switch tabs at times when popups are allowed.

(This is a proposed solution to bug 292314 / bug 357379.)

Comment 1

[Don Kleppinger]

I concur.  If bringing a window into focus is allowed then bringing a tab into focus must be allowed when the user has it set to open windows in a tab.

Comment 2

[Jesse Ruderman]

I suggested the same thing for scripted-resizing in bug 454779 comment 6.

Comment 3

[David Karger]

I hope someone is still watching this bug.  As web apps become more complicated, blocking window.focus() on tabbed windows becomes even less of a good idea.  I'm working on a web app that will naturally run in two distinct tabs, and it would like to be able to manage switching between them.  Right now the only way I can do that is to have the window that wants focus post an alert() message (which always surfaces that window).  A rather drastic solution!

I think it's reasonable to argue that the "permissions" on windows in tabs should be exactly the same as the "permissions" for (html) windows in separate windows.  If a user empowers js to window.focus a different window, it should also be able to window.focus a different tab.

Comment 4

[Joe Theuerkauf]

The original bug for this (#292314) was  filed in 2005!

It's obviously a low-traffic bug, but would it really be that hard to fix so that a tab's Window Object is the same as a new window's Window Object? Or is this a fundamental limitation in ECMA/Javascript that requires accounting for tabs as something different from stand-alone windows? Frankly, there are some things that might be advantageous if the distinction existed, but that's another discussion.

Really sorry to find this inconsistency in Firefox, especially one that's been around so long.

Comment 5

[Boris Zbarsky [:bzbarsky]]

There is no difference in window objects.  The differences are all in the UI code...

Comment 6

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> There is no difference in window objects.  The differences are all in the UI
> code...

Attachment 199614 [details] [diff] seems to disagree.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, interesting.  Well, that should certainly make it easy to fix: just check the popup state there.  Does that do the trick?

Comment 8

[David Karger]

Boris, I don't understand the proposed fix.  If you're talking about *detecting* whether something is in a window or a popup, that's insufficient. The goal is to be able to bring a tab to the "front" using window.focus() that same way as it is currently possible to bring a window to the front.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The "popup state" is whether popups are allowed right now, not whether something is a popup.

In fact, we already check that for toplevel window.focus below, so since we disallow window raising/lowering by default (with the "opener window" exception in popup-allowed conditions) perhaps the entire chunk of code linked to in comment 6 can just go away.

Comment 10

[Mario Blazevic]

Now that bug #332195 is fixed, the only (****) workaround for this bug has been made largely inoperable as well. It used to be possible to switch focus to a tab by sending a message to that tab to invoke window.alert(). That's gone as well now, unless we make users flip browser.tabs.dontfocusfordialogs setting in about:config.

Comment 11

[Anna]

Is anyone looking at this bug?

Comment 12

[:Harald Kirschner :digitarald]

Panos, Mike, I'd like to nominate this webcompat:p2, as it is confirmed to affect Outlook and possibly other multi-window web apps. Not sure how actionable this is, but :bz laid out next steps a few years ago. Thoughts?

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

I agree if Mike doesn't see any downsides.

Comment 14

[Andrew Overholt [:overholt]]

I spoke off-bug with Boris and Gijs about this and the consensus is that we should probably have UX weigh in here on what we should do.

Comment 15

[Mike Taylor [:miketaylr]]

(In reply to Andrew Overholt [:overholt] from comment #14)
> I spoke off-bug with Boris and Gijs about this and the consensus is that we
> should probably have UX weigh in here on what we should do.

Let's wait and hear what UX has to say. Harald, can you link the Outlook bug? (I might be missing an obvious link somewhere...)

Comment 16

[Andrew Overholt [:overholt]]

Philipp, can you weigh in from a UX perspective here?

Comment 17

[:Harald Kirschner :digitarald]

> Harald, can you link the Outlook bug? (I might be missing an obvious link somewhere...)

So far this was only an email thread. Will follow up with a bug.

Comment 18

[:Harald Kirschner :digitarald]

Outlook notified us (again) that they are shipping a feature soon that depends on this, switching between multiple modules that open in their own tab. Firefox will not focus the existing tab. I would nominate this to be a p1.

This bug would align focus behavior with Safari and Chrome. Should UX be a blocker for this, as there is prior art?

Comment 19

[Peter Saint-Andre [:stpeter]]

Agree with Harald.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

As discussed offline with Andrew, UX feedback would be most welcome, but it isn't a blocker.

Comment 21

[Marcos Caceres [:marcosc]]

I'd like to work on this, but noticed that nsGlobalWindow.cpp is now gone (looking at an old patch above). 

Boris, could you point me in the right direction as I'm not familiar with the part of the code base... I'm guessing nsGlobalWindowOuter I need to be. I presume "popup state" can be derived easily enough via maybe GetPopupControlState?

Any tips so I don't go off in the wrong direction would be appreciated.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm not sure what patch you mean, but generally speaking the implementation of window.focus() lives in nsGlobalWindowOuter::FocusOuter.

There's the GetVisibility() check which filters out non-frontmost tabs and would need to be modified to take the popup state into account.  There's a canFocus boolean below that which does take popup state into account (via some machinery around GetPopupControlState()) and can maybe just be used as-is, depending on how we want to handle the opener thing.

Comment 23

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

While we currently don't have enough bandwidth on the UX team to look into this in more depth, it should be safe to follow the prior art from Safari and Chrome here.

Comment 24

[bradfulton]

Any progress on a fix?

Comment 25

[Marcos Caceres [:marcosc]]

Not yet. Finishing a few other things. If someone else has cycles, feel free to take it. Otherwise, I'll try to get to it within the next 4-8 weeks.

Comment 26

[Marcos Caceres [:marcosc]]

Unassigning myself for now as I don't want to block someone else from taking it... if no one takes in time, I will come back to it.

Comment 27

[Andrea Marchesini [:baku]]

PopupBlocking is already used and it works fine for window.focus():

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/base/nsGlobalWindowOuter.cpp#4850-4857

The problem is elsewhere: the content process sends a request to have the window focused:

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/base/nsFocusManager.cpp#2168
https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/widget/PuppetWidget.cpp#263
https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/ipc/TabChild.cpp#2755-2758

But, on the parent side, we stop here:
https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/base/nsFocusManager.cpp#1126-1130
https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/base/nsFocusManager.cpp#1462

CheckIfFocusable(Element* aElement, uint32_t aFlags) returns nullptr and we don't set the focus to the tab.

NI enndeakin, the reviewer of bug 902715 which implemented window.focus() in the content process.

Comment 28

[Neil Deakin]

When a window.focus() request is made from the child process, but the window is not frontmost, does the window get raised? That's the part the focus manager would handle.

Some other new code, probably something added to TabParent::RecvRequestFocus, needs to also inform the tabbrowser to switch tabs, before the focus occurs. Tab switching happens asynchronously though, so the focus would need to happen after that.

Comment 29

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating webcompat priority whiteboard tags to project flags.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: new testcase

The previous testcase didn't work because of the use of data: URLs. This test uses blob URLs instead.

Comment 31

[Andrea Marchesini [:baku]]

I asked Emilio if he has time to take a look. The issue seems to be in layout. In my debugging I see that tabs and windows have a similar path when window.focus() is called:

BrowserParent::RecvRequestFocus
https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/dom/ipc/BrowserParent.cpp#2122,2143

nsFocusManager::SetFocus -> nsFocusManager::SetFocusInner
https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/dom/base/nsFocusManager.cpp#1130-1137

In here, for tabs, FlushAndCheckIfFocusable() returns null, because the element is not focusable:
https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/layout/generic/nsFrame.cpp#9705,9712

In particular, nsIFrame::IsVisibleConsideringAncestors() return false because of this:
https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/layout/generic/nsFrame.cpp#391,408

Moving the NI to emilio because I'm not familiar with layout code.

Comment 32

[Neil Deakin]

(In reply to Andrea Marchesini [:baku] from comment #31)

In particular, nsIFrame::IsVisibleConsideringAncestors() return false because of this:
https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/layout/generic/nsFrame.cpp#391,408

Yes, as mentioned in comment 28, the focus is occurring but cannot because the tab content is not visible. Some code needs to be added to switch tabs in response to a window focus request.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Ah, I was thinking of just teaching stuff inside <deck> to be shown and focus it, then the browser would have a chance to react to the focus event and fix up the tab bar.

But I guess that's a no-go if we ever want to move away from <deck> and such, and just an async message to the browser that switches tabs should work and be a bit simpler.

WDYT Neil, is that correct? If so I'd go for firing an event at the browser element (tabfocused or such maybe?) and make the browser listen to that and react accordingly.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 416771 - Remove OwnerDoc() null-checks. r=NeilDeakin

OwnerDoc() never returns null.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 416771 - Fix non-unified build in BrowserParent. r=NeilDeakin

Depends on D31640

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 416771 - Use RefPtr<Element> rather than nsCOMPtr in BrowserParent. r=NeilDeakin

Depends on D31641

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 416771 - Allow window.focus() to switch tabs. r=NeilDeakin

I think this should just work as-is, but probably a test would be on point.

Depends on D31642

Comment 38

[Emilio Cobos Álvarez (:emilio)]

I think those patches should at least work for Desktop-e10s, it's not clear to me what the setup is for Fennec or non-e10s, haven't investigated that yet.

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Made it work for non-e10s as well.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bedfab1ba206
Remove OwnerDoc() null-checks. r=NeilDeakin

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Landed the first couple patches since they were just cleanups.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/491fc6abedc6
Fix non-unified build in BrowserParent. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/27d8ed079e63
Use RefPtr<Element> rather than nsCOMPtr in BrowserParent. r=NeilDeakin

Comment 43

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/bedfab1ba206
https://hg.mozilla.org/mozilla-central/rev/491fc6abedc6
https://hg.mozilla.org/mozilla-central/rev/27d8ed079e63

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5acccb49a668
Allow window.focus() to switch tabs. r=NeilDeakin,dao

Comment 45

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/118d5e15de39
Backed out changeset 5acccb49a668 for gecko decision bustage CLOSED TREE

Comment 46

[Emilio Cobos Álvarez (:emilio)]

The gecko decision stuff is not related to my patch, so I put it back in the queue.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f5394545457
Allow window.focus() to switch tabs. r=NeilDeakin,dao CLOSED TREE

Comment 48

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4f5394545457

Comment 49

[:Gijs (he/him)]

Would this be appropriate / small enough to uplift to 68 (which AIUI will be next esr)? As per the whiteboard, I think outlook (and probably other sites) are waiting for this fix.

Comment 50

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9065728 [details]
Bug 416771 - Allow window.focus() to switch tabs. r=NeilDeakin

Yes, I planned to clean up a bit on top and add some automated tests for this, but these patches should be reasonably safe to uplift to 68 I'd think, specially given we're early in the cycle.

Beta/Release Uplift Approval Request

• User impact if declined: Important compat issue.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See the test-case in comment 30.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's focus code, so there's always some risk, but these patches are mostly moving code around, and disabling the behavior change in my patch is literally one line, so given we're early in the cycle I think we should try to get this into the next ESR.
• String changes made/needed: none

Comment 51

[Julien Cristau [:jcristau]]

Comment on attachment 9065725 [details]
Bug 416771 - Remove OwnerDoc() null-checks. r=NeilDeakin

webcompat fix, approved for 68.0b4

Comment 52

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37d3185821e9
https://hg.mozilla.org/releases/mozilla-beta/rev/c42e0bb77074
https://hg.mozilla.org/releases/mozilla-beta/rev/b147035a8d92
https://hg.mozilla.org/releases/mozilla-beta/rev/3cc54d1aa9d2

Comment 53

[Dricks]

https://bugzilla.mozilla.org/show_bug.cgi?id=292314#c6
11 years later.
Too bad, my company decided to switch to Chromium 7 months ago.
And i couldn't even try to fight against this decision because they were right.
11 years to focus a tab.
Guys!

Comment 54

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 69.0a1 (2019-05-23) and on Firefox 68.0b4 on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.