Closed Bug 110718 Opened 23 years ago Closed 21 years ago

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

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.3alpha

People

(Reporter: homere, Assigned: aaronlev)

References

(
URL
)

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

Don't erase old focus outline or mess with focus until a key is pressed in the zombie document
7.59 KB, patch
bryner
: review+
jst
: superreview+
Details | Diff | Splinter Review 
I have to click on a loaded tab to uses CTRL-T and open a new one.
dup of bug 76495 by way of bug 103697

*** This bug has been marked as a duplicate of 76495 ***
Status: UNCONFIRMED → RESOLVED
Closed: 23 years ago
Resolution: --- → DUPLICATE
Reopening due to bug 76495 comment 119
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Summary: CTRL-T doesn't work when current tab is loading → CTRL-T, CTRL-N, and some menus / apps don't work / respond when current tab / page is loading.
*** Bug 111677 has been marked as a duplicate of this bug. ***
*** Bug 111825 has been marked as a duplicate of this bug. ***
*** Bug 115109 has been marked as a duplicate of this bug. ***
*** Bug 122596 has been marked as a duplicate of this bug. ***
*** Bug 145597 has been marked as a duplicate of this bug. ***
*** Bug 162652 has been marked as a duplicate of this bug. ***
*** Bug 154437 has been marked as a duplicate of this bug. ***
*** Bug 152551 has been marked as a duplicate of this bug. ***
*** Bug 145660 has been marked as a duplicate of this bug. ***
*** Bug 144507 has been marked as a duplicate of this bug. ***
*** Bug 131635 has been marked as a duplicate of this bug. ***
*** Bug 129232 has been marked as a duplicate of this bug. ***
*** Bug 124425 has been marked as a duplicate of this bug. ***
*** Bug 118797 has been marked as a duplicate of this bug. ***
*** Bug 116458 has been marked as a duplicate of this bug. ***
*** Bug 103697 has been marked as a duplicate of this bug. ***
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 ?
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.
CTRL-H is the same issue.  This makes Mozilla have a bad "feel" for the user.
*** Bug 165230 has been marked as a duplicate of this bug. ***
Keywords: access
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.
Status: NEW → ASSIGNED
Taking
Assignee: hyatt → aaronl
Status: ASSIGNED → NEW
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
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=
Component: Tabbed Browser → Keyboard Navigation
QA Contact: blaker → sairuh
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);

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.
*** Bug 178164 has been marked as a duplicate of this bug. ***
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
Attachment #106147 - Flags: superreview+
Attachment #106147 - Flags: review?(bryner)
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?
Yeah, that might make sense. I can try to make that recursive.
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?
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.
Attachment #106147 - Attachment is obsolete: true
Attachment #107197 - Flags: superreview?(hyatt)
Attachment #107197 - Flags: review?(bryner)
Attachment #106147 - Flags: review?(bryner) → review-
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.
Attachment #107197 - Flags: review?(bryner) → review+
Filed spinoff bug 181910
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?
*** Bug 183715 has been marked as a duplicate of this bug. ***
Attachment #107197 - Flags: superreview?(hyatt) → superreview?(scc)
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 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
Attachment #107197 - Flags: superreview?(scc) → superreview+
*** Bug 184278 has been marked as a duplicate of this bug. ***
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
fixed!
Status: NEW → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED
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?

Who is sheriffing?

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
I wonder why this would have such an effect on Luna, but not btek.
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);
            }
        }
    }


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.


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).
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.
Actually, I just saw a window raise on load on win2k.
someone just filed bug 185448 on the raise-on-load issue.
Someone back out the patch, please.

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

/be
Blocks: 185448
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...

Tp and Txul are back down.

Thanks for backing it out -- I'll work on an improved patch without the perf
problem.
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=
Attachment #107197 - Attachment is obsolete: true
Attachment #109463 - Attachment is obsolete: true
Attachment #109463 - Flags: review?(bryner)
Attachment #109463 - Flags: review?(bryner) → review-
Attachment #109463 - Flags: review-
Attachment #109464 - Flags: superreview?(brendan)
Attachment #109464 - Flags: review?(bryner)
*** Bug 186125 has been marked as a duplicate of this bug. ***
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.
Attachment #109464 - Flags: review?(bryner) → review+
Attachment #109464 - Flags: superreview?(brendan) → superreview?(scc)
Flags: blocking1.3b?
Attachment #109464 - Flags: superreview?(scc) → superreview?(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
Attachment #109464 - Flags: superreview?(jst) → superreview+
checked in
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
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
Sarah -- no, that's another problem.
Flags: blocking1.3b?
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!
Status: RESOLVED → VERIFIED
*** Bug 190702 has been marked as a duplicate of this bug. ***
*** Bug 190889 has been marked as a duplicate of this bug. ***
*** Bug 191404 has been marked as a duplicate of this bug. ***
*** Bug 192993 has been marked as a duplicate of this bug. ***
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.
Linux 20030422 also has it. Reopening bug (see comment 72 for how to reproduce it).
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.4?
Flags: blocking1.4? → blocking1.4-
*** Bug 207849 has been marked as a duplicate of this bug. ***
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.
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?
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.

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.
I'm not sure which checkin fixed it but this is definitely fixed in mozilla 1.4
on Macintosh.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 21 years ago
Resolution: --- → FIXED
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.
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: