254056 - focus lost when closing tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Brian Ryner (not reading)]

Steps:

1. Open a URL from history in a new tab using middle-click.
2. Close the history sidebar
3. Close the newly-opened tab with ctrl+w or by clicking the close box
4. Try to use pgdown in the now-current tab.

Result: keyboard navigation (tab, up/down) don't work.  If you hit tab a couple
of times focus will return to the URL bar

Exepcted result: the page content should have the keyboard focus

One clue to what's happening may lie in what happens if you don't close the
history sidebar.  If you do that, focus returns to the history sidebar instead
of going to the next tab.  Perhaps we're still trying to focus the history
sidebar for some reason when it's closed?

Comment 1

[Manish Jethani]

Why open new tab?

1. Open history sidebar (Ctrl + H)
2. Close history sidebar (click on "x").
3. Focus not returned.

Comment 2

[Aaron Leventhal]

Or just hit Ctrl+H twice

Marking dup of the newer bug since that one has a patch in it.

*** This bug has been marked as a duplicate of 255187 ***

Comment 3

[Steffen Wilberg]

Comment 1 and comment 2 will be fixed by my patch in bug 255187.
Comment 0 is another problem. -> reopening.

I suspect this code:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.36.6.16&mark=593-610#593

Comment 4

[Ronald W. Heiby]

Under Windows 2000 Pro, with FF 1.0, I opened several tabs by using the
right-click / Open Link in New Tab mechanism.

I used the Ctrl-PgDn and Ctrl-PgUp keystrokes to navigate among these tabs. I
ended up on one a couple from the right (we'll call it "Tab A"), went into print
preview, fiddled around with print options, and printed the tab's contents.

I left print preview, and used Ctrl-PgUp to move one tab to the left (Tab B). I
read the information there and dismissed it with Ctrl-W.

Now, I'm looking at Tab A again. But, I still don't want to do anything with
this (since I haven't verified that the printer printed it). So, I Ctrl-PgUp
again and am positioned at a different tab than Tab B (which was dismissed), but
one which is just to the left of Tab A.

I press PgDn. "Nothing" happens. I press it again a couple of times. The same
"nothing" happens. Hitting Ctrl-PgDn, I am looking at Tab A, but *it* has
scrolled down a few pages.

Now, Tab A does not seem to be doing anything particularly fancy. It's basically
just a bunch of text with various kinds of formatting attached. Its URL is
<http://www.puschitz.com/SecuringLinux.shtml>. So, I don't think this has
anything to do with some of the bug reports that talk about Java and stealing
focus. I think this is purely within Firefox, and it not keeping focus attached
to the tab being displayed.

Comment 5

[Aaron Leventhal]

Sounds similar to bug 276934. Dupe?

Comment 6

[David Tenser [:djst]]

*** Bug 276934 has been marked as a duplicate of this bug. ***

Comment 7

[David Tenser [:djst]]

Bug 276934, which I filed and now duped against this one covered the focus issue
when closing a tab without using the history sidebar. The steps to reproduce:

1. Open two tabs. Verify that Space scrolls down
2. Close one of the tabs. Try to scroll with Space... doesn't work.
3. Focus the page with the mouse and you can scroll using Space again.

Comment 8

[Doron Rosenberg (IBM)]

FC3 Gtk2 and latest trunk CVS:

I open 2 cnn.com tabs, scroll down one with space, close the tab (ctrl-w), press
space and it scrolls fine.

The original report is reproducable though.

Comment 9

[Brian Ryner (not reading)]

*** Bug 284361 has been marked as a duplicate of this bug. ***

Comment 10

[Brian Ryner (not reading)]

Attachment: patch

The problem is that there's a bug in the tabbrowser focus memory code.	When we
save the focused element or window when you switch tabs, we should not be
saving a focused element that's elsewhere in the browser window, outside of the
selected tab.  This situation arises here because the history sidebar has
focus, and when it opens a new tab, the original tab grabs the
currently-focused element for the toplevel window, which is the history sidebar
tree.  It then restores focus there when you switch back to that tab, despite
the fact that the sidebar is closed.

This patch makes it so that we only save a focused element or window if it's
either:

(1) contained inside of the tab we're switching away from
(2) contained in the same window as the tabbrowser widget, or an ancestor

(2) is necessary to save focus in the URL bar, which we want.  It also allows
focus to be saved i.e. in the search box, but I'm not sure what a better
solution would be for this problem.  Maybe we could restrict the check to (1)
and then special-case the URL bar?

Comment 11

[[e-mail]]

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

>+              while (focusedWindow) {
>+                if (focusedWindow == contentWindow) {
>+                  saveFocus = true;
>+                  break;
>+                }
>+
>+                if (focusedWindow.parent == focusedWindow) {
>+                  break;
>+                }
>+
>+                focusedWindow = focusedWindow.parent;
>+              }
This looks as if it's comparing focusedWindow.top to contentWindow (assuming
bug 281988 lands, so you don't have to fiddle with Components.lookupMethod).

>+                focusedWindow = document.commandDispatcher.focusedWindow;
>+                contentWindow = window;
>+
>+                while (contentWindow) {
>+                  if (contentWindow == focusedWindow) {
>+                    saveFocus = true;
>+                    break;
>+                  }
>+
>+                  if (contentWindow.parent == contentWindow) {
>+                    break;
>+                  }
>+
>+                  contentWindow = contentWindow.parent;
>+                }
And this appears to compare focusedWindow to window (window == window.parent in
navigator.xul/browser.xul).

>+                // Something in an unrelated DOM window has the focus, so we
>+                // don't preserve that for this tab.  Preserve the current
>+                // focus memory, or if there is none, use the content area.
I don't see why you have to preseve anything; the restoration code already
knows how to deal with the current values. Just overwriting the values only
when saveFocus is true would appear to suffice.

Comment 12

[Brian Ryner (not reading)]

(In reply to comment #11)
> (From update of attachment 176313 [details] [diff] [review] [edit])
> This looks as if it's comparing focusedWindow.top to contentWindow (assuming
> bug 281988 lands, so you don't have to fiddle with Components.lookupMethod).

Sure, that works.

> And this appears to compare focusedWindow to window (window == window.parent in
> navigator.xul/browser.xul).

That may be true for those two XUL files, but I was trying to make things
generic, i.e. if you had nested XUL iframes in the chrome.

> 
> >+                // Something in an unrelated DOM window has the focus, so we
> >+                // don't preserve that for this tab.  Preserve the current
> >+                // focus memory, or if there is none, use the content area.
> I don't see why you have to preseve anything; the restoration code already
> knows how to deal with the current values. Just overwriting the values only
> when saveFocus is true would appear to suffice.
> 

I should have commented on that I guess.  I originally did what you suggest, and
started getting exceptions from browser.js due to the null focus state when
switching back to the tab.  I figured I'd stir the pot the least by preserving
the behavior of always saving _something_.  Come to think of it though, the
exception may have been related to the |try| I needed to add around accessing
contentWindow, so I'll verify that this is still the case.

Comment 13

[Brian Ryner (not reading)]

Attachment: this works too

Yeah, looks like those assertions were from the exception accessing
contentWindow, not from not setting the focusedElement/Window.

Comment 14

[[e-mail]]

Comment on attachment 176353 [details] [diff] [review]
this works too

The xpfe version never reaches that assertion, because removeTab clears
mCurrentBrowser as necessary. However more usefully window.content refers to
the same window and returns null without asserting when removing a tab, which
must be good?

Comment 15

[Brian Ryner (not reading)]

Attachment: use window.content

Comment 16

[[e-mail]]

Comment on attachment 176662 [details] [diff] [review]
use window.content

>+              var contentWindow = window.content;
>+              var saveFocus = false;
>+
>+              if (focusedWindow.top == contentWindow) {
>+                saveFocus = true;
>+              } else {
>+                contentWindow = window;
I'd suggest if (focusedWindow.top == window.content) and var contentWindow =
window; ... wait, it's no longer a content window ;-)

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 176662 [details] [diff] [review]
use window.content

sr=[e-mail]

Comment 18

[Brian Ryner (not reading)]

checked in

Comment 19

[Aaron Leventhal]

*** Bug 286362 has been marked as a duplicate of this bug. ***

Comment 20

[Jason Barnabe (np)]

This caused bug 287637.

Comment 21

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381