110718 - CTRL-T, CTRL-N, and some menus / apps don't work / respond when current tab / page is loading.

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Frédéric VANNIÈRE]

I have to click on a loaded tab to uses CTRL-T and open a new one.

Comment 1

[R.K.Aa.]

dup of bug 76495 by way of bug 103697

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

Comment 2

[R.K.Aa.]

Reopening due to bug 76495 comment 119

Comment 3

[Jason Bassford]

Since bug 764595 comment 121 reopened many bugs that are dupes of each other
(but not bug 764595 apparently), trying to better resolve the situation.

Marking others dupes of this one.

Platform/OS -> All.
Modifying Summary to include the situation / better support searches.
Confirming.

Comment 4

[Jason Bassford]

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

Comment 5

[Jason Bassford]

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

Comment 6

[Jason Bassford]

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

Comment 7

[Jason Bassford]

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

Comment 8

[Jason Bassford]

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

Comment 9

[Jason Bassford]

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

Comment 10

[Jason Bassford]

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

Comment 11

[Jason Bassford]

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

Comment 12

[Jason Bassford]

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

Comment 13

[Jason Bassford]

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

Comment 14

[Jason Bassford]

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

Comment 15

[Jason Bassford]

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

Comment 16

[Jason Bassford]

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

Comment 17

[Jason Bassford]

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

Comment 18

[Jason Bassford]

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

Comment 19

[Jason Bassford]

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

Comment 20

[Peter Lockhart]

I thought this bug would be better in 1.1, but I see its often worse than 1.0.
sometimes its impossible to get ctrl-T to work, you have to use the icon.

Any news on progress here ?

Comment 21

[Jason Bassford]

As far as I can tell, no progress has been made on this bug at all.  Only bug
76495 has had any progress - and this one (and its dupes) is no longer
considered to be part of that.  Nobody's discussed it or checked anything in for
it yet.

Comment 22

[Jay Davis]

CTRL-H is the same issue.  This makes Mozilla have a bad "feel" for the user.

Comment 23

[Jason Bassford]

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

Comment 24

[William Sherwin]

I cannot use CTRL+N to start a new browser window from Mail.  However, I can use
CTRL+SHIFT+N to start a Composer window, from which I can use CTRL+N to start a
Browser window.  Also, if the keyboard shortcut doesn't work, the button in the
lower left-hand corner doesn't work.

Comment 25

[Aaron Leventhal]

Taking

Comment 26

[Aaron Leventhal]

Attachment: Fixes problem by sending key events to root pres shell if the current doc is a zombie

This also fixes a problem where if a user changes their mind and clicks on a
link while in the middle "Transferring" to a new page, onload wasn't firing in
the page that comes up. For example, if you were waiting for a really long page
and decided to click on a link to www.google.com instead, the textfield
wouldn't get automatically focused once you go there.

Unfortunately, this patch does not fix bug 76495 "Can't interact with zombie
pages any more [keyboard, links, scrollbars don't respond]" as well. However
this patch does seem to make our behavior exactly like IE 6.

Seeking r=/sr=

Comment 27

[Asko Tontti]

BTW the following comment might need some rework?

     // Stop any current network activity.  Do not stop the content until
     // data starts arriving from the new URI...
     //
-    rv = Stop(nsIWebNavigation::STOP_NETWORK);
+    rv = Stop(nsIWebNavigation::STOP_ALL);

Comment 28

[Aaron Leventhal]

Yeah, I should have read that comment :)

However, if we don't stop the content then, the onload for the new document
won't fire. So I still think we should do it, and fix the comment.

Comment 29

[sairuh (rarely reading bugmail)]

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

Comment 30

[David Hyatt]

Comment on attachment 106147 [details] [diff] [review]
Fixes problem by sending key events to root pres shell if the current doc is a zombie

sr=hyatt

Comment 31

[Brian Ryner (not reading)]

Comment on attachment 106147 [details] [diff] [review]
Fixes problem by sending key events to root pres shell if the current doc is a zombie

I think this will work, but would it make more sense to walk up the docshell
tree and dispatch it to the first non-zombie document we find, instead of
jumping all the way to the top?

Comment 32

[Aaron Leventhal]

Yeah, that might make sense. I can try to make that recursive.

Comment 33

[Aaron Leventhal]

Brian, I tried your idea of using the parent instead, and ran into a problem.

The problem occurs if you are focused in an iframe that contains a zombie
document and hit a keyboard navigation command such as Tab or a printable
character that will start type aheadfind. The parent HTML document will make use
of that navigation command, and focus a new element. However, the iframe
document does not release focus, so it still has the dotted outline on
something. You end up with 2 focus outlines at the same time. Would you rather
have me work that problem out, or should we stay with the first patch?

Comment 34

[Aaron Leventhal]

Attachment: Implements bryner's suggestion of moving the key events up to the next non-zombie parent docshell, instead of straight to the top

I did a lot of testing with this by having a page with an iframe doc that
contained several links. With this patch, keyboard navigation makes sense after
you click on a link in the iframe. You can navigate around the parent document
or the top XUL UI doc. Tab navigation will avoid zombie documents, but work in
their parents.

I also tested to make sure that my patch to docshell doesn't impact initial
focus on the document when it gets loaded.

Comment 35

[Brian Ryner (not reading)]

Comment on attachment 107197 [details] [diff] [review]
Implements bryner's suggestion of moving the key events up to the next non-zombie parent docshell, instead of straight to the top

>Index: layout/html/base/src/nsPresShell.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v
>retrieving revision 3.577
>diff -u -r3.577 nsPresShell.cpp
>--- layout/html/base/src/nsPresShell.cpp	9 Nov 2002 00:25:21 -0000	3.577
>+++ layout/html/base/src/nsPresShell.cpp	23 Nov 2002 00:55:01 -0000
>+  // Sent this events straight up to the parent pres shell.
>+  // We do this for non-mouse events in zombie documents.
>+  // That way at least the UI key bindings can work.
>+
>+  nsCOMPtr<nsISupports> container;
>+  mPresContext->GetContainer(getter_AddRefs(container));
>+  nsCOMPtr<nsIDocShellTreeItem> treeItem = 
>+    do_QueryInterface(container);
>+  if (treeItem) {
>+    nsCOMPtr<nsIDocShellTreeItem> parentTreeItem;
>+    treeItem->GetParent(getter_AddRefs(parentTreeItem));
>+    nsCOMPtr<nsIDocShell> parentDocShell = 
>+      do_QueryInterface(parentTreeItem);
>+    if (parentDocShell && treeItem != parentTreeItem) {
>+      nsCOMPtr<nsIPresShell> parentPresShell;
>+      parentDocShell->GetPresShell(getter_AddRefs(parentPresShell));
>+      nsCOMPtr<nsIViewObserver> parentViewObserver = 
>+        do_QueryInterface(parentPresShell);

I'd suggest a NS_STATIC_CAST to PresShell here, that lets you avoid the QI.


>@@ -5056,10 +5073,11 @@
>         }
>     }
>     //
>-    // Stop any current network activity.  Do not stop the content until
>-    // data starts arriving from the new URI...
>+    // Stop any current network activity.  
>+    // Stop the content as well, otherwise if we are switching to
>+    // load a different doc, the onload won't fire for the new document.
>     //
>-    rv = Stop(nsIWebNavigation::STOP_NETWORK);
>+    rv = Stop(nsIWebNavigation::STOP_ALL);
>     if (NS_FAILED(rv)) return rv;
>     
> 

As we discussed on AIM, let's file a separate bug for this problem.  r=bryner
with those changes.

Comment 36

[Aaron Leventhal]

Filed spinoff bug 181910

Comment 37

[Jay Davis]

I want to put in my own fresh comments on this bug:
It would certainly be nice to not feel like one's browser is broken or clunky
when moving around it quickly.  Right now, if you are just starting to launch a
url and then you want to open a new tab (Ctrl-T), it is frustrating.  The same
goes for Ctrl-H, Ctrl-B, Ctrl-N, and Ctrl-tab.  Am I missing something?  Does
there have to be a big rewrite of Mozilla to get this stuff to work?

Comment 38

[R.K.Aa.]

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

Comment 39

[George K.]

I came across this bug while looking for a slightly different issue (which bug
159908 covers).  This bug, being so similar in impact to the bug I was looking
for, might belong in the blocking list for bug 83552, the meta-tracking bug for
all Focus (Tab Navigation) issues.

Then again, maybe not.  At any rate, I only found 159908 because of 83552 which
is why I suggested this...  

Comment 40

[Scott Collins]

Comment on attachment 107197 [details] [diff] [review]
Implements bryner's suggestion of moving the key events up to the next non-zombie parent docshell, instead of straight to the top

Hmmm, it looks fine... you have a general trend though of declaring a pair of
|nsCOMPtr|s at a time, when one of them should be declared inside the following
conditional expressions, e.g.,

  nsCOMPtr<...> doc, subDoc;

wait to declare |subDoc| inside the |if (doc)| statement.  Does this make a big
difference?  Well, for |nsCOMPtr|s, in this case, where the alternative path is
that they are never assigned, no.  I'm sure here the impact is entirely
negligible.  But that isn't always true, and certainly for objects with more
costly construction/destruction costs, you need to be more concerned.

You can have my sr for this.  I would appreciate it if you would move the
declarations inward where appropriate (|subDoc|, |zombieViewer|).  It is
appropriate when they don't need to be used outside of the controlled
statement.

sr=scc

Comment 41

[Aaron Leventhal]

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

Comment 42

[Aaron Green, MD]

To be clear about how the problem affects the user: from my other post that was
marked a dupe:

--clip--
The problem is when mozilla has established connection with a site, and status
bar says "transferring data from site" but has not yet started displaying the
page: it's no longer reading keyboard input, specifically CTRL-T and CTRL-TAB or
CTRL-PAGEDOWN...This is frustrating because while waiting for a page to load,
I'm wanting to go on and do something in another tab.

The good news is that if I use the MOUSE, Mozilla does indeed respond (e.g.,
switch to another tab), so Mozilla just needs to be checking for keyboard input
as well.
--clip--

Thanks for working on this

Comment 43

[Aaron Leventhal]

fixed!

Comment 44

[Daniel Bratell]

There's a 3% page load performance regression on luna after checking this in.
First I thought it might have been jag's checkin because he warned for a slight
perf degradation, but the regression seems to have come half an hour before he
checked in. 

If I read the performance regression policy correctly this should not be
accepted.  Maybe you could back it out to see if times go back down if you're
not sure I'm wrong?

Comment 45

[Brendan Eich [:brendan]]

Who is sheriffing?

/be

Comment 46

[R.K.Aa.]

i built with this checkin on Linux, and the browser window now alway raise to
top when a page has loaded. (adding
user_pref("mozilla.widget.raise-on-setfocus", false); has no effect either)

Comment 47

[Aaron Leventhal]

I wonder why this would have such an effect on Luna, but not btek.

Comment 48

[Aaron Leventhal]

I think both the Tp problem and the problem with the browser window always
raising have to do with this code in SetupNewViewer


    // Eliminate the focus ring in the current docshell, which is 
    // now a zombie. If we key navigate, it won't be within this
    // docshell, until the newly loading document is displayed.
    if (mItemType == typeContent) {
        nsCOMPtr<nsIPresContext> presContext;
        GetPresContext(getter_AddRefs(presContext));
        if (presContext) {
            nsCOMPtr<nsIEventStateManager> esm;
            presContext->GetEventStateManager(getter_AddRefs(esm));
            if (esm) {
                esm->SetFocusedContent(nsnull);
                esm->SetContentState(nsnull, NS_EVENT_STATE_FOCUS);
            }
        }
    }

Comment 49

[John Morrison]

I am currently building win32/linux opt. builds to see if I can confirm what
we are seeing on the tinderboxen (and whether backing out this checkin will 
drop the times back down again).

I had also noticed that btek was less affected (or unaffected) the way that 
other tinderboxen were. (By the way, I think there is also a Txul regression,
but that's probably just reflecting the increase in cost to load a page).

One notable difference in the build configuation for btek is that it does 
not build venkman, and so it does not get the JS debugging hooks installed.
(Actually, I only vaguely remember what I'm trying to describe, but I disabled
venkman on btek when it caused about a 20ms increase in pageload when rginda
turned "something" on by default.)

Otherwise, btek differs from the other tinderboxes in that it disables crypto,
and it is testing against a localhost http server (on a dual cpu box). I've
look at that page load server that is serving the other tinderbox clients,
and it looks normal.

Comment 50

[John Morrison]

In the builds I made, I see about a 1.5% degradation on Linux rh6.1 and 
about a 1.0% degradation on win2k (both 500MHz/128MB machines). The data
on the simpler pages (e.g., google, yahoo) suggest that there is a fixed
cost of about 10msec that has been added to the load (and/or unload) of 
a document. (Well, I wouldn't want to right a paper based on that data. 
So, that's data plus a hunch).

Comment 51

[John Morrison]

I can also see the window raise on load on Linux, as R.K.Aa pointed out
(although I don't see it on Windows). Since almost nothing creates as much
wailing and gnashing of teeth in bugzilla than this type of focus bug, 
I'd strongly advise, independent of the perf issue, that you (aaronl) back 
this patch out until we know how to fix that problem.

Comment 52

[John Morrison]

Actually, I just saw a window raise on load on win2k.

Comment 53

[R.K.Aa.]

someone just filed bug 185448 on the raise-on-load issue.

Comment 54

[Brendan Eich [:brendan]]

Someone back out the patch, please.

/be

Comment 55

[Brendan Eich [:brendan]]

Or just #if 0 the offending paragraph aaronl identified in comment #48, if that
relieves all the symptoms.

/be

Comment 56

[Daniel Bratell]

I just backed out the complete patch before I read the #if 0-suggestion but I
would have done the same anyway. Aaron can check in a fixed patch when he's
back. IMHO it would be bad to have only a part of patch in the tree unless sure
that the part works well on its own. Better the state you know...

Comment 57

[Daniel Bratell]

Tp and Txul are back down.

Comment 58

[Aaron Leventhal]

Thanks for backing it out -- I'll work on an improved patch without the perf
problem.

Comment 59

[Aaron Leventhal]

Attachment: Don't erase old focus outline or mess with focus until a key is pressed in the zombie document

This removes the code I added to nsDocShell::SetupNewViewer() and moves it into
PresShell::RetargetEventToParent(). In this new way of erasing the focus
outline when it needs to be, I have to get the old viewer and force it to get
displayed via nsIContentViewer::Show().

Seeking new r=/sr=

Comment 60

[Aaron Leventhal]

Attachment: Don't erase old focus outline or mess with focus until a key is pressed in the zombie document

Comment 61

[Olivier Cahagne]

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

Comment 62

[Brian Ryner (not reading)]

Comment on attachment 109464 [details] [diff] [review]
Don't erase old focus outline or mess with focus until a key is pressed in the zombie document

>@@ -4684,7 +4706,11 @@
> {
>   // Focus an element in the current document, but don't switch document/window focus!
> 
>-  if (gLastFocusedDocument == mDocument) { // If we're already in this document, use normal focus method
>+  if (gLastFocusedDocument == mDocument || 
>+      !gLastFocusedContent) { 

I don't see any reason to break this if statement up into 2 lines.


>@@ -5887,6 +5892,83 @@
>   }
> }
> 
>+PRBool PresShell::InZombieDocument(nsIContent *aContent)
>+{
>+  // If a content node points to a null document, it is a
>+  // zombie document, about to be replaced by a newly loading document.
>+  // Such documents cannot handle DOM events.
>+  nsCOMPtr<nsIDocument> doc;
>+  mCurrentEventContent->GetDocument(*getter_AddRefs(doc));
>+  return !doc;
>+}

This comment isn't entirely accurate.  It's possible to create an element via
the DOM, and it will have a null document until it is inserted into a document.
 I'm fairly sure this won't affect your patch, but you should verify that if
you create an element, don't insert it into a document, and then create a key
event and call dispatchEvent on the element, that it doesn't end up going into
a document of some sort.

>+
>+nsresult PresShell::RetargetEventToParent(nsIView         *aView,
>+                                          nsGUIEvent*     aEvent,
>+                                          nsEventStatus*  aEventStatus,
>+                                          PRBool          aForceHandle,
>+                                          PRBool&         aHandled,
>+                                          nsIContent*     aZombieFocusedContent)
>+{
>+  // Sent this events straight up to the parent pres shell.

Did you mean "Send this event"?

>+  // Next, update the display so the old focus ring is no longer visible
>+
>+  nsCOMPtr<nsISupports> container;
>+  mPresContext->GetContainer(getter_AddRefs(container));
>+
>+  nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(container));
>+  if (docShell) {

Here (and other places) I'd prefer assertions to null-checks for things that
never really fail - for example, when we have only 1 object that implements an
interface, and we know that it also implements the interface you're QI'ing to. 
Not only do you not pay the cost of the null-check in a release build, it makes
the code more readable.

r=bryner with those comments addressed.

Comment 63

[Johnny Stenback (:jst)]

Comment on attachment 109464 [details] [diff] [review]
Don't erase old focus outline or mess with focus until a key is pressed in the zombie document

What bryner said, and...

- In PresShell::RetargetEventToParent():

+  if (docShell) {
...
+      if (zombieViewer) {
+	 nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
+	 zombieViewer->Show();
+      }
+  }

Don't you want to scope the kungFuDeathGrip to this function in stead of just
locally to this scope that's followed by a lot of code that *could* some day
try to access a member of this?

+  nsCOMPtr<nsIDocShellTreeItem> treeItem = 
+    do_QueryInterface(container);
+  if (treeItem) {
...
+      if (parentViewObserver) {
+	 PopCurrentEventInfo();
+	 return parentViewObserver->HandleEvent(aView, aEvent, 
+					      aEventStatus,
+					      aForceHandle, 
+					      aHandled);
+      }
+    }
+  }
+  return NS_ERROR_FAILURE;

How about returning early in the 'error' cases and making the call into
HandleEvent() on the parent viewer observer the last thing in this method in
stead? And fix up the indentation of the next line arguments to HandleEvent().

sr=jst

Comment 64

[Aaron Leventhal]

checked in

Comment 65

[sairuh (rarely reading bugmail)]

aaronl, is the fix you checked supposed to cover the case where the throbber
keeps spinning (even after clicking the stop button)? afaict, that still seems
to be a problem...

not sure if that issue is covered by another bug in bugzilla. however, in
bugscape there's http://bugscape.mcom.com/show_bug.cgi?id=20546

Comment 66

[Aaron Leventhal]

Sarah -- no, that's another problem.

Comment 67

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2003.01.13.08 comm trunk bits on linux rh8.0, win2k and mac
10.2.3. thanks for fixing this, aaronl!

Comment 68

[R.K.Aa.]

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

Comment 69

[R.K.Aa.]

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

Comment 70

[R.K.Aa.]

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

Comment 71

[Stefan Borggraefe]

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

Comment 72

[Daniel Clemente]

I can reproduce this bug even on Linux 2003030308. Do the following:
Open ftp.mozilla.org; then open link 'pub' on a new tab on the background.
Before it finishes loading, press Ctrl+Tab repeteadly to go to the next tab. It
gets stuck on that 'Loading' tab, and when it is loaded, Ctrl+Tab works again.

It doesn't happen only with FTP; I have also seen it at some webs.

Comment 73

[Daniel Clemente]

Linux 20030422 also has it. Reopening bug (see comment 72 for how to reproduce it).

Comment 74

[Jason Bassford]

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

Comment 75

[Autumn]

The comments imply that _some_ keyboard functionality is lost.  In fact,
versions through 1.4 appear to accept NO keystrokes.  Once in a zombie tab, one
MUST resort to the mouse to escape.

Comment 76

[Jay Davis]

Ctrl-T, B, N, H all work just fine in builds over the last few months.  Is
anyone else who's using a build from 1.5 to 1.6b having this problem?

Comment 77

[Ian Batterbee]

Even 1.4 (20030624) seems okay for me at the moment, so I assume the later
versions also work.. can someone confirm ? Steps to repeat are in various other
comments in the bug.

Comment 78

[Daniel Clemente]

I haven't seen this bug anymore and the instructions I gave in comment 72 work
ok now, so it may have been fixed. Mozilla 2003111005 (1.6b) is ok.
Does it work on Firebird too? Then we can close the bug.

Comment 79

[Kathleen :Brade]

I'm not sure which checkin fixed it but this is definitely fixed in mozilla 1.4
on Macintosh.

Comment 80

[Thadeu Cascardo]

I have firefox-0.9.3-6 on Debian and it hangs when I press Ctrl-Tab many times
while the current tab is loading.