Last Comment Bug 249136 - Focus lost when key pressed in newly loading foreground tab
: Focus lost when key pressed in newly loading foreground tab
Status: RESOLVED FIXED
[ETA: perf #s are back to normal, it ...
: access, fixed1.8, helpwanted, sec508
Product: Core
Classification: Components
Component: Keyboard: Navigation (show other bugs)
: Trunk
: x86 All
: -- minor (vote)
: ---
Assigned To: Aaron Leventhal
:
Mentors:
: 269278 278527 301761 (view as bug list)
Depends on: 306076 311607 352446 416710
Blocks: focusnav 218216 deera11y 303679 303715
  Show dependency treegraph
 
Reported: 2004-06-29 16:17 PDT by Tony Bolton
Modified: 2014-05-06 08:30 PDT (History)
26 users (show)
cbeard: blocking1.8b5+
bugs: blocking‑aviary1.5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
I have a hunch that this bandaid fixes the problem (604 bytes, patch)
2004-10-29 07:49 PDT, Aaron Leventhal
no flags Details | Diff | Review
Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document (3.97 KB, patch)
2004-12-20 10:18 PST, Aaron Leventhal
no flags Details | Diff | Review
169217: Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document (3.40 KB, patch)
2004-12-21 05:00 PST, Aaron Leventhal
bryner: review+
jst: superreview+
Details | Diff | Review
A nice easy workaround in tabbrowser.xml using setTimeout(0, ...) to select new tabs (1.25 KB, patch)
2005-07-29 20:47 PDT, Aaron Leventhal
mconnor: review+
mconnor: approval1.8b4+
Details | Diff | Review
Similar to last approach, but only use setTimeout for setting selected tab when actually loading a new page in a new foreground tab (5.61 KB, patch)
2005-08-04 21:24 PDT, Aaron Leventhal
mconnor: review+
timeless: review+
benjamin: approval1.8b4+
Details | Diff | Review
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window (2.12 KB, patch)
2005-08-19 07:34 PDT, Aaron Leventhal
no flags Details | Diff | Review
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window (1.07 KB, patch)
2005-08-19 09:05 PDT, Aaron Leventhal
mats: review+
bryner: superreview+
Details | Diff | Review
wip (untested) (3.61 KB, patch)
2005-08-23 20:16 PDT, Mats Palmgren (:mats)
aaronlev: review-
brendan: superreview+
Details | Diff | Review
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events (1.07 KB, patch)
2005-08-24 09:07 PDT, Aaron Leventhal
no flags Details | Diff | Review
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events. Remove unnecessary NS_IMETHOD and in param. Initialize mNeedUpdateCommands. (5.17 KB, patch)
2005-08-24 09:18 PDT, Aaron Leventhal
mats: review+
bryner: superreview+
asa: approval1.8b4+
Details | Diff | Review

Description Tony Bolton 2004-06-29 16:17:37 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040616
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040616

When multiple tabbed pages are present some tabs have the keyboard movement keys
disabled. Mouse and scroll bars are OK. Re-load is no help, click to a linked
page on that tab restores operation for the new page.

Reproducible: Always
Steps to Reproduce:
1. Open a link onto a new tab
2. Immediately press some scroll keys (arrows etc) before any part of the new
page appears.
3.  When page appears test scroll key function
Actual Results:  
Scroll keys action is disabled in that tab

Expected Results:  
Scroll keys should move around page

Notes...
(1) Problem occurs occasionally even if keys are not touched till page is complete.
(2) My dialup connection gives sufficient time to press lots of keys before even
the page title appears in the tab.
(3) Cached pages do not show the problem so each test needs a fresh page.
(4) Forms entry and Tab key work normally even if scroll keys do not.
Comment 1 Bogdan Stroe 2004-06-30 16:14:00 PDT
It could be bug 97283 that you are seeing. Can you provide specific pages where
this is happening?
Comment 2 Tony Bolton 2004-06-30 18:00:07 PDT
(In reply to comment #1)
> It could be bug 97283 that you are seeing. Can you provide specific pages where
> this is happening?

Without understanding all the detail of 97383 and not having a scroll wheel it
is a bit unlikely. In particular...

Whether the scrolling works seems to depend on pressing a key at the right point
in the loading of the page, not its particular content. I tried pages from
several sites including mozilla.org and all could be made to fail, but they did
also include div elements.

I have never observed a failure without tabbed browsing, everthing I tried in a
normal window worked, irrespective of content.

The site where I first noticed the intermittant problem independent of key
pressing, ie waiting for a load to complete then no scroll key action was on
www.ebay.com - viewing items, but this may just be the number of uses rather
than a particular problem with eBay.
Comment 3 Bogdan Stroe 2004-06-30 21:09:33 PDT
(In reply to comment #2)
> Whether the scrolling works seems to depend on pressing a key at the right point
> in the loading of the page, not its particular content. I tried pages from
> several sites including mozilla.org and all could be made to fail, but they did
> also include div elements.
> 
> I have never observed a failure without tabbed browsing, everthing I tried in a
> normal window worked, irrespective of content.

Are you sure the focus is in that tab? If you click inside the window, then does
the scroll start working at that point? 
Comment 4 Tony Bolton 2004-07-01 14:43:41 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Whether the scrolling works seems to depend on pressing a key at the right point
> > in the loading of the page, not its particular content. I tried pages from
> > several sites including mozilla.org and all could be made to fail, but they did
> > also include div elements.
> > 
> > I have never observed a failure without tabbed browsing, everthing I tried in a
> > normal window worked, irrespective of content.
> 
> Are you sure the focus is in that tab? If you click inside the window, then does
> the scroll start working at that point? 

Focus is fine, on a working tab I can swap between mouse and scroll keys and
both work, in a problem tab I can swap all I like and only the mouse works.
The only way discovered so far to restore the keys is to click through a link on
the page or close the tab. 
Comment 5 Aaron Leventhal 2004-07-01 20:09:29 PDT
It's most likely something related to focus.

Following your steps I am able to duplicate a bug where I can't scroll
temporarily. However, scrolling keys work again once I click in the browser content.

With that problem I've verified that focus is indeed in a strange place -- on
the XUL window as a whole.

However, it sounds much different from what's being reported here:
(4) Forms entry and Tab key work normally even if scroll keys do not.

Does part (4) of your bug still happen when you upgrade to a recent trunk build?
I'm using Mozilla 1.8a2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040625
Comment 6 Tony Bolton 2004-07-02 14:37:20 PDT
(In reply to comment #5)
> It's most likely something related to focus.
> 
> Following your steps I am able to duplicate a bug where I can't scroll
> temporarily. However, scrolling keys work again once I click in the browser
content.
> 
> With that problem I've verified that focus is indeed in a strange place -- on
> the XUL window as a whole.
> 
> However, it sounds much different from what's being reported here:
> (4) Forms entry and Tab key work normally even if scroll keys do not.
> 
> Does part (4) of your bug still happen when you upgrade to a recent trunk build?
> I'm using Mozilla 1.8a2
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040625
> 

This sounds more promising, still with version 1.7

I can also restore the scroll keys by clicking the tab window content, but can
also confirm that clicking a forms entry field and entering characters does not
restore the key action. There must be a subtle difference in the concept of
focus between an entry field and window content.
Comment 7 Aaron Leventhal 2004-07-02 14:41:50 PDT
(In reply to comment #6)
> (In reply to comment #5)
> This sounds more promising, still with version 1.7

What happens if you try it with a recent 1.8 trunk build?
Comment 8 Tony Bolton 2004-07-04 04:07:47 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > This sounds more promising, still with version 1.7
> 
> What happens if you try it with a recent 1.8 trunk build?

I am now trying 1.8a1, latest there is on the public site, but operating system
is now up-to-date Win XP pro.

It behaves exactly the same as Moz 1.7 on Win98 as listed above.
(I'm glad this is an alpha, it seems to react violently to all ebay CGI output
with an 'Alert no data' message.)
Comment 9 Jess Thrysoee 2004-07-10 02:49:38 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040708

I'm seeing something similar on Linux. It might have something to do with the
address field, because the caret is present (and blinking) on all tabs no matter
what I do. The arrow/pgup/pgdown works fine in just one tab, but when trying to
scroll a page in anther tab, the page in that one tab is scrolled instead.
Comment 10 Tony Bolton 2004-07-12 16:38:45 PDT
(In reply to comment #9)
> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040708
> 
> I'm seeing something similar on Linux. It might have something to do with the
> address field, because the caret is present (and blinking) on all tabs no matter
> what I do. The arrow/pgup/pgdown works fine in just one tab, but when trying to
> scroll a page in anther tab, the page in that one tab is scrolled instead.

This is similar but different to my problem, in particular...

(1) Pressing scroll keys in a faulty tab has no effect on the content of any
other tab.
(2) It is possible to have multiple normal tabs and only 1 faulty one. 
Comment 11 Jason Slaughter 2004-09-29 12:19:44 PDT
I'm seeing a very clearly reproducible bug that may be related to this:

1) open several pages in tabs
2) scroll to the top of the page in tab 1
3) click on tab 2
4) press the "down" or "page down" keys. The page will not scroll.
5) click back to tab 1

You'll notice that your keypresses were sent to (the background) tab 1, as tab 1
is no longer at the top of the page as was set in step 2. The keyboard focus
isn't being set to the foreground tab.

I'm seeing this on Mozilla Firefox 1.0PR on Linux.
Comment 12 Aaron Leventhal 2004-09-29 12:27:41 PDT
Hmm sounds very much like bug 247323, which was fixed in the trunk only. Does
this bug occur on nightly trunk builds of Firefox?
Comment 13 Jason Slaughter 2004-10-06 07:06:27 PDT
This is not fixed in the trunk it seems. I'm still seeing both problems (they're
probably the same thing anyhow): the reported problem (no keyboard focus on
clicked tabs) and my problem (keyboard presses are being sent to background tab)
on the latest nightly trunk.
Comment 14 Aaron Leventhal 2004-10-29 07:49:20 PDT
Created attachment 163842 [details] [diff] [review]
I have a hunch that this bandaid fixes the problem

Will someone test this?
Comment 15 Tony Bolton 2004-11-04 11:53:59 PST
I tried to find the file tabbrowser.xml listed in the attachment but it seems
not to present in the 1.7.3 installation I am using now after the upgrade
attempt to 1.8.4 trashed my profile.
If this can be applied to 1.7.3 can you hint as to where ? If so I will need to
apply it by hand so a little more context with the diff will ensure correct
application.
Comment 16 Aaron Leventhal 2004-11-05 07:05:16 PST
(In reply to comment #15)
> I tried to find the file tabbrowser.xml listed in the attachment but it seems
> not to present in the 1.7.3 ...
Sorry, I accidentally posted a patch for Firefox.

The same file exists in Mozilla 1.7.3 in
mozilla/xpfe/global/resources/content/bindings

It's just a matter of adding that line as shown in the patch. Let me know what
you find. Thanks.



Comment 17 Tony Bolton 2004-11-05 10:53:47 PST
I searched the entire disk for several variations of parts of the name and
directory and nothing surfaced. A search for *.xml returned about 300 files, but
none had anything to do with mozilla.
Comment 18 Aaron Leventhal 2004-11-05 11:03:03 PST
(In reply to comment #17)
> I searched the entire disk for several variations of parts of the name and
> directory and nothing surfaced. A search for *.xml returned about 300 files, but
> none had anything to do with mozilla.

Do you have the source code or did you just install the binary?
Comment 19 Tony Bolton 2004-11-05 11:15:38 PST
Always just the binary...
Comment 20 Aaron Leventhal 2004-11-05 12:05:09 PST
(In reply to comment #19)
> Always just the binary...

Okay, sorry. We have to find someone to build it with the patch, which requires
a source code version.
Comment 21 Sebastian Redl 2004-12-15 03:20:32 PST
Experiencing the same problem, Mozilla 1.8a5, WinXP SP2. Any way of switching
tabs (both by mouse and keyboard) leaves the keyboard focus on the original tab.
This means that, for example, scrolling by keyboard affects the background tab,
not the one I'm viewing. Clicking in the content area of the active tab will
shift the keyboard focus - but the point is that I don't want to use the mouse,
obviously.

I'm fetching a source update right now, so I can test this patch.
Comment 22 Aaron Leventhal 2004-12-15 06:18:45 PST
If someone who builds from source is experiencing this, please try my patch.
Comment 23 Sebastian Redl 2004-12-15 08:54:34 PST
I just built from source with the patch, and the problem still occurs. But the
original bug was more complicated, because it only occurs if a tab is loaded in
the background, and perhaps only if keys are pressed during that loading. At one
point, the loading page seems to grab focus and not release it again. I observed
the same behaviour in the patched custom build and in an official 1.8a5.
Comment 24 Aaron Leventhal 2004-12-15 09:00:03 PST
It still sounds like focus suppressions are out of whack again. That's what
caused the same symptoms in bug 247323.

Remove my patch, but uncomment the #ifdef DEBUG_hyatt lines (and remove the
ifdef), so that the focus suppression status gets printed.
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsFocusController.cpp#471
Then you can run:
firefox -console
and see those messages.

What should happen when Firefox is working properly, is that the value always
goes to 0.
Comment 25 Aaron Leventhal 2004-12-15 09:06:39 PST
I can reproduce this bug now, but the following Firefox setting is required:
[x] Select new tabs opened from links
Comment 26 neil@parkwaycc.co.uk 2004-12-16 15:39:16 PST
Comment on attachment 163842 [details] [diff] [review]
I have a hunch that this bandaid fixes the problem

Note that whatToFocus isn't necessarily a window.
Comment 27 Aaron Leventhal 2004-12-20 04:13:17 PST
(In reply to comment #26)
> (From update of attachment 163842 [details] [diff] [review] [edit])
> Note that whatToFocus isn't necessarily a window.
> 

True, but that patch was marked obsolete already.

I believe whatToFocus is a window in the case where new tabs load in the
foreground, which is when this bug occurs.
Comment 28 Aaron Leventhal 2004-12-20 09:33:52 PST
The event is getting handled by the xul document instead of the html doc.
Comment 29 Aaron Leventhal 2004-12-20 10:18:36 PST
Created attachment 169217 [details] [diff] [review]
Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document

This fixes the problem because documents loading in a foreground tab ran this
code for keystrokes, which ended up switching focus to the parent XUL window.

The original zombie case from bug 110718 is still fixed. In that case a link is
clicked to load a new document in the current window, and the focus outline
needs to disappear.
Comment 30 Aaron Leventhal 2004-12-21 05:00:16 PST
Created attachment 169289 [details] [diff] [review]
169217: Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document
Comment 31 Aaron Leventhal 2004-12-23 10:37:05 PST
*** Bug 269278 has been marked as a duplicate of this bug. ***
Comment 32 Aaron Leventhal 2004-12-23 16:46:51 PST
Might fix bug 273650.
Comment 33 Bob Coleman 2004-12-23 18:19:48 PST
I found my way here as a result of the discussion in bug 273650.  While the
symptoms are not exactly the same and the cause may or may not be the same, I
think I've encountered the bug being discussed here in circumstances not yet
reported here.  I often do one of two things:

1  Open in tabs all the sites in a bookmark folder.

2. From some kind of scripting language run in succession multiple commands of
the                  
   form:

          ...firefox url

Either of these actions results in multiple firefox tabs.  Most of the time, but
I think not always, they result in Page Down affecting some background tab
rather than the one made foreground/active as a result of the above actions.

In this case clicking in the window of the foreground tab does cause the
keyboard to subsequently affect that tab.

As I start closing these tabs one by one, I will sometimes come to another one
in which Page Down is dead.

I don't think the exact symptoms are reliably repeatable.  This sort of feels
like a timing problem affected at least in part by what else might be running or
not running at the time, but I certainly don't know that to be the case.
 
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-07 13:02:21 PST
Comment on attachment 169289 [details] [diff] [review]
169217: Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document

sr=jst
Comment 35 Brian Ryner (not reading) 2005-01-13 11:45:05 PST
Comment on attachment 169289 [details] [diff] [review]
169217: Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document

>@@ -5512,40 +5512,42 @@ nsresult PresShell::RetargetEventToParen
>+  if (aZombieFocusedContent) {
>+    // If sub-content within current document is focused,
>+    // eliminate the focus ring, because the current docshell
>+    // is now a zombie. If we key navigate, it won't be within this
>+    // docshell, until the newly loading document is displayed.
>+    // Note: if document itself has focus, leave the focus alone so that 
>+    // we don't mess up the "open link in new foreground tab" feature --
>+    // there is no focus outline in that case anyway.
>+    esm->SetContentState(nsnull, NS_EVENT_STATE_FOCUS);
>+    esm->SetFocusedContent(nsnull);

Do we really need SetFocusedContent(nsnull) if we've already called
SetContentState? (SendFocusBlur should reset the current focus, unless I'm
misreading)

r=me if you can explain why we need that, or if you remove it.
Comment 36 Brian Ryner (not reading) 2005-01-13 14:21:29 PST
Comment on attachment 169289 [details] [diff] [review]
169217: Wrap code in if(). When key events retargeted to parent of zombie, only reset focus outline when focus is on child content of document

Ok, given that this is just being shuffled around, r=me.
Comment 37 Aaron Leventhal 2005-01-17 06:45:38 PST
Checking in nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.802; previous revision: 3.801
done
Comment 38 Aaron Leventhal 2005-02-05 08:30:52 PST
Had to back this out because it caused bug 279285.
Comment 39 Aaron Leventhal 2005-02-05 08:33:53 PST
This one is difficult. Help would be appreciated. My first fix broke bug 279285. 
Comment 40 Jeff Rivett 2005-02-17 13:46:44 PST
I'm experiencing this problem but may have a few observations as yet unreported.
 It definitely seems to occur when configured for external links to open up a
new tab.  Basically, sometimes a bunch of weird behaviours start occurring in
the new tab:

[1] Bookmarks toolbar weirdness: sometimes all the bookmarks are off-screen.
[2] Keyboard not working for page navigation, form field editing, address
editing, etc. (arrow keys, pg keys, etc.)
[3] Right-click on page not showing correct context menu.
[4] Right-click on address not showing correct context menu.
[5] Single quote keypress brings up the search bar.

Note that ALL of these are resolved by restoring the Firefox window (Alt-Space,
R), then maximizing it again.
Comment 41 Bug Filler 2005-03-05 01:23:52 PST
Also see bug #264815
Comment 42 Aaron Leventhal 2005-04-19 19:26:08 PDT
This is one I think Brian Ryner needs to look at.
Comment 43 Asa Dotzler [:asa] 2005-06-15 12:05:30 PDT
bryner, can you take a look at this? do you think this is in the deep dark
places we don't wanna go this late in the game? 
Comment 44 Chris Beard 2005-07-29 17:27:57 PDT
if we're going to fix this, we need to do so before b4, otherwise it's not going
to make it given the high-risk for regression with focus changes.

/cb
Comment 45 Aaron Leventhal 2005-07-29 18:03:11 PDT
(In reply to comment #44)
> if we're going to fix this, we need to do so before b4, otherwise it's not going
> to make it given the high-risk for regression with focus changes.

If we're not going to fix it, we should remove the option for open link in a new
tab in the options dialog. Otherwise this breaks accessibility for anyone who
uses the feature without warning.
Comment 46 Aaron Leventhal 2005-07-29 19:14:09 PDT
What if we do a workaround for this release, and tweak "select new tabs opened
from links" option. We can wait until some event shows the presshell is ready
before actually selecting the new tab. Question is, what even -- use a web
progress listener or pageshow, or what?
Comment 47 Aaron Leventhal 2005-07-29 20:47:11 PDT
Created attachment 191022 [details] [diff] [review]
A nice easy workaround in tabbrowser.xml using setTimeout(0, ...) to select new tabs
Comment 48 Aaron Leventhal 2005-08-01 11:17:20 PDT
*** Bug 278527 has been marked as a duplicate of this bug. ***
Comment 49 Aaron Leventhal 2005-08-01 11:17:47 PDT
*** Bug 301761 has been marked as a duplicate of this bug. ***
Comment 50 Aaron Leventhal 2005-08-04 14:11:00 PDT
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.97; previous revision: 1.96
done
Comment 51 Peter van der Woude [:Peter6] 2005-08-04 17:15:56 PDT
this one most probably caused 
bug 303475 and bug 303477
Comment 52 Aaron Leventhal 2005-08-04 17:40:32 PDT
Sounds like we should back it out as an unsuccessful attempt to fix this bug
without creating more problems.
Comment 53 Aaron Slunt 2005-08-04 18:30:04 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050804
Firefox/1.0+ ID:2005080416

This is WFM, even with the last patch removed from my zip build. Was this patch
trying to fix something that wasn't broken anymore?
Comment 54 Aaron Leventhal 2005-08-04 19:17:06 PDT
(In reply to comment #51)
> this one most probably caused 
> bug 303475 and bug 303477

Probably also caused bug 303479.

I think I'll back it out.
Comment 55 Aaron Leventhal 2005-08-04 21:24:44 PDT
Created attachment 191669 [details] [diff] [review]
Similar to last approach, but only use setTimeout for setting selected tab when actually loading a new page in a new foreground tab
Comment 56 Mike Connor [:mconnor] 2005-08-04 22:54:08 PDT
Comment on attachment 191669 [details] [diff] [review]
Similar to last approach, but only use setTimeout for setting selected tab when actually loading a new page in a new foreground tab

looks good, but let's get a second pair of eyes on this since the last patch
broke so much.
Comment 57 timeless 2005-08-05 12:22:18 PDT
Comment on attachment 191669 [details] [diff] [review]
Similar to last approach, but only use setTimeout for setting selected tab when actually loading a new page in a new foreground tab

this style really shouldn't be needed for formal declared params, but whatever:

+	     var bgLoad = (typeof(aLoadInBackground) != "undefined") ?
aLoadInBackground :
+			 
this.mPrefs.getBoolPref("browser.tabs.loadInBackground");
Comment 58 Adam Guthrie 2005-08-05 14:32:13 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050802 Firefox/1.0+

The new patch doesn't seem to fix the problem for me.
Comment 59 Benjamin Smedberg [:bsmedberg] 2005-08-05 19:58:10 PDT
Comment on attachment 191669 [details] [diff] [review]
Similar to last approach, but only use setTimeout for setting selected tab when actually loading a new page in a new foreground tab

a=me presuming that this actually does fix the problem and comment #58 is
mistaken.
Comment 60 Marc Bejarano 2005-08-05 21:03:20 PDT
i wonder if this will also fix firefox bug 218216?
Comment 61 Aaron Leventhal 2005-08-05 21:20:22 PDT
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.100; previous revision: 1.99
done
Checking in toolkit/content/contentAreaUtils.js;
/cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.75; previous revision: 1.74
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.468; previous revision: 1.467
done
Checking in browser/components/bookmarks/content/bookmarks.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v  <-- 
bookmarks.js
new revision: 1.103; previous revision: 1.102
done
Comment 62 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-08-06 06:50:30 PDT
Comment on attachment 191669 [details] [diff] [review]
Similar to last approach, but only use setTimeout for setting selected tab when actually loading a new page in a new foreground tab

>Index: toolkit/content/widgets/tabbrowser.xml

>+              function selectNewForegroundTab(browser, tab) {
>+                this.selectedTab = tab;
>+              }
>+              setTimeout(selectNewForegroundTab, 0, getBrowser(), tab);

That second line was meant to be |browser.selectedTab = tab;|, right? This
patch broke the selection of new tabs created by openNewTabWith when
browser.tabs.loadInBackground is false, and that change fixes it for me.
Comment 63 Aaron Leventhal 2005-08-06 07:13:49 PDT
Oy, thanks. Correction checked in.
Comment 64 Aaron Leventhal 2005-08-12 19:57:47 PDT
This is still fixed, but keyboard scrolling doesn't always seem to work right
away in the new tab.
Comment 65 Adam Guthrie 2005-08-14 20:34:35 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050814 Firefox/1.0+
ID:2005081410

This still isn't fixed on Linux.
Comment 66 Aaron Leventhal 2005-08-19 07:34:37 PDT
Created attachment 193158 [details] [diff] [review]
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window

This really works, and doesn't seem to break anything. I think the
mCurrentElement condition was only a small optimization.

Either way we'll need to back out the previous solution which only worked when
the page loaded very fast, and was a hack around approach.
Comment 67 Aaron Leventhal 2005-08-19 08:08:47 PDT
Please ignore the printfs.
Comment 68 Aaron Leventhal 2005-08-19 09:05:41 PDT
Created attachment 193171 [details] [diff] [review]
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window
Comment 69 Mats Palmgren (:mats) 2005-08-19 15:03:33 PDT
Comment on attachment 193171 [details] [diff] [review]
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window

Looks good. r=mats
Comment 70 Aaron Leventhal 2005-08-23 17:32:45 PDT
Fix checked in on trunk
Comment 71 Boris Zbarsky [:bz] 2005-08-23 19:19:35 PDT
This has regressed Tp on btek and probably Txul and Ts on comet; still waiting
on other tinderboxes to cycle...
Comment 72 Aaron Leventhal 2005-08-23 20:01:20 PDT
I guess we can improve it. Perhaps, either UpdateCommands needs to be more
clever about whether it has any work to do, or nsFocusController needs to be
smarter about when it's already called UpdatedCommands.
Comment 73 Mats Palmgren (:mats) 2005-08-23 20:16:34 PDT
Created attachment 193651 [details] [diff] [review]
wip (untested)

Does this fix Tp?
Comment 74 Boris Zbarsky [:bz] 2005-08-23 20:39:08 PDT
No way to tell without checking in -- those tests are not really publically
available.
Comment 75 Brendan Eich [:brendan] 2005-08-23 20:45:40 PDT
Comment on attachment 193651 [details] [diff] [review]
wip (untested)

Rubberstamping. Mats, or anyone: please check in and see what happens.

/be
Comment 76 Aaron Leventhal 2005-08-24 01:54:46 PDT
Cool Mats -- that's exactly the solution I thought of. I'll check it in.
Comment 77 Aaron Leventhal 2005-08-24 01:57:50 PDT
Mats, what do you think about putting the test for mNeedUpdateCommands in
UpdateCommands() instead of SetSuppressFocus. Might that end up getting us a
performance improvement overall instead of just gett us back to 0?
Comment 78 Aaron Leventhal 2005-08-24 06:07:04 PDT
Well, I see why you did the mNeedsUpdateCommands check where you did actually.

However, now that I'm testing I'm seeing this perf patch makes the bug fix no
longer work.
Comment 79 Aaron Leventhal 2005-08-24 07:44:13 PDT
Comment on attachment 193651 [details] [diff] [review]
wip (untested)

Patch breaks original fix. Looking into why.
Comment 80 Aaron Leventhal 2005-08-24 09:07:54 PDT
Created attachment 193707 [details] [diff] [review]
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events
Comment 81 Aaron Leventhal 2005-08-24 09:16:24 PDT
Comment on attachment 193707 [details] [diff] [review]
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events

WRong patch
Comment 82 Aaron Leventhal 2005-08-24 09:18:08 PDT
Created attachment 193709 [details] [diff] [review]
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events. Remove unnecessary NS_IMETHOD and in param. Initialize mNeedUpdateCommands.
Comment 83 Asa Dotzler [:asa] 2005-08-24 11:45:26 PDT
does that old approval request still apply? If not, please unset it. this should
land on the trunk and bake before we consider it. 
Comment 84 Mats Palmgren (:mats) 2005-08-24 13:53:41 PDT
Comment on attachment 193709 [details] [diff] [review]
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events. Remove unnecessary NS_IMETHOD and in param. Initialize mNeedUpdateCommands.

Looks good. r=mats

But please keep the bug open since I still see the original
problem in a Firefox/WinXP debug build (both with and
without the latest patch). (updated ~2 hours ago)
It also occurs in SeaMonkey 2004-08-24-05 (WinXP).

STEPS TO REPRODUCE
1. start Firefox and set pref "select new tab opened from links" on
2. load http://plugindoc.mozdev.org/faqs/
3. middle-click the last link on the page:
   "Embedded Windows Media in Firefox 1.0" (which links to
   http://forums.mozillazine.org/ which is really slow)
4. press the DOWN key as soon as the new tab opens before
   the page is loaded.
5. wait for the page to load
6. press DOWN -- it does not work until you click
   on the page background.

If I skip step 4 then it works as expected.
Comment 85 Aaron Leventhal 2005-08-24 17:56:41 PDT
Perf fix checked into the trunk. Let's see how it does.
Comment 86 Aaron Leventhal 2005-08-24 18:47:54 PDT
Yes! The Tp, Txul and Ts numbers went back to normal.
Comment 87 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-25 09:23:59 PDT
Is this all fixed on the trunk?  If so, please mark FIXED so we can start
watching for reports of regression, etc.
Comment 88 Aaron Leventhal 2005-08-25 10:19:47 PDT
I'll file a new bug for what Mats reported in comment 84.
Comment 89 Aaron Leventhal 2005-08-25 10:34:27 PDT
Filed bug 305939 for comment 84
Comment 90 Aaron Leventhal 2005-08-26 11:56:43 PDT
Comment on attachment 193171 [details] [diff] [review]
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window

I used the a= for the 2nd patch is a blanket for the solution, since that patch
made no sense without this one.
Comment 91 Boris Zbarsky [:bz] 2005-08-26 19:53:57 PDT
Could this possibly have caused bug 306076?
Comment 92 Boris Zbarsky [:bz] 2005-08-26 21:43:57 PDT
OK, I've verified via explicit backout that the last patch in this bug caused
bug 306076.
Comment 93 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-09-09 07:15:35 PDT
Aaron, now that you checked in the "real fix", can the setTimeout hack that was
initially checked in be backed out? That would fix bug 303715.
Comment 94 Aaron Leventhal 2005-09-09 11:42:52 PDT
(In reply to comment #93)
> Aaron, now that you checked in the "real fix", can the setTimeout hack that was
> initially checked in be backed out? That would fix bug 303715.

Yes.
Comment 95 Ilja Sekler 2006-01-22 09:52:42 PST
I can still reproduce the problem using the testcase from Comment #84 with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060122 Firefox/1.6a1

More: opening a new tab from a link found by find-as-you-type with Ctrl+Enter opens the page always without focus on Windows (but always with focus on Linux, which is IMHO correct), it is necessary to press the tabulator key numerous times or to click into the page. FF 1.5 is also affected.

Shall I file a new bug on this issue? Not seeing a bug matching the behavior exactly.
Comment 96 Boris Zbarsky [:bz] 2006-01-22 10:00:58 PST
Yeah, that sounds like food for a new bug...
Comment 97 Ilja Sekler 2006-01-22 12:01:25 PST
OK; https://bugzilla.mozilla.org/show_bug.cgi?id=324349
Comment 98 Boris Zbarsky [:bz] 2006-09-13 18:17:22 PDT
The last patch in this bug also caused bug 249136.

Note You need to log in before you can comment on or make changes to this bug.