focus lost when closing tab

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: bryner, Assigned: bryner)

Tracking

(Blocks: 1 bug, {access, helpwanted, sec508})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

14 years ago
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

14 years ago
Why open new tab?

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

Comment 2

14 years ago
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 ***
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE

Comment 3

14 years ago
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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Updated

14 years ago
Blocks: 83552
Keywords: access, sec508

Comment 4

14 years ago
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.

Updated

14 years ago
Keywords: helpwanted
Priority: -- → P1

Comment 5

14 years ago
Sounds similar to bug 276934. Dupe?

Comment 6

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

Comment 7

14 years ago
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

14 years ago
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.
(Assignee)

Comment 9

14 years ago
*** Bug 284361 has been marked as a duplicate of this bug. ***
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 10

14 years ago
Created attachment 176313 [details] [diff] [review]
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?
Assignee: bugs → bryner
Status: REOPENED → ASSIGNED
Attachment #176313 - Flags: superreview?(bugs)
Attachment #176313 - Flags: review?(neil.parkwaycc.co.uk)

Comment 11

14 years ago
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.
(Assignee)

Comment 12

14 years ago
(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.
(Assignee)

Comment 13

14 years ago
Created attachment 176353 [details] [diff] [review]
this works too

Yeah, looks like those assertions were from the exception accessing
contentWindow, not from not setting the focusedElement/Window.
Attachment #176313 - Attachment is obsolete: true
Attachment #176353 - Flags: superreview?(bugs)
Attachment #176353 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Attachment #176313 - Flags: superreview?(bugs)
Attachment #176313 - Flags: review?(neil.parkwaycc.co.uk)

Comment 14

14 years ago
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?
(Assignee)

Comment 15

14 years ago
Created attachment 176662 [details] [diff] [review]
use window.content
Attachment #176353 - Attachment is obsolete: true
Attachment #176662 - Flags: superreview?(bugs)
Attachment #176662 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Attachment #176353 - Flags: superreview?(bugs)
Attachment #176353 - Flags: review?(neil.parkwaycc.co.uk)

Comment 16

14 years ago
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 ;-)
Attachment #176662 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 176662 [details] [diff] [review]
use window.content

sr=ben@mozilla.org
Attachment #176662 - Flags: superreview?(bugs) → superreview+
(Assignee)

Comment 18

14 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 19

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

Comment 20

14 years ago
This caused bug 287637.
You need to log in before you can comment on or make changes to this bug.