Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Focus lost when key pressed in newly loading foreground tab

RESOLVED FIXED

Status

()

Core
Keyboard: Navigation
--
minor
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: Tony Bolton, Assigned: Aaron Leventhal)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
All
access, fixed1.8, helpwanted, sec508
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ETA: perf #s are back to normal, it looks like the recent regressions involving text fields related to bug 303620, not this one])

Attachments

(4 attachments, 6 obsolete attachments)

3.40 KB, patch
Brian Ryner (not reading)
: review+
Details | Diff | Splinter Review
5.61 KB, patch
mconnor
: review+
timeless
: review+
bsmedberg
: approval1.8b4+
Details | Diff | Splinter Review
1.07 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Brian Ryner (not reading)
: superreview+
Details | Diff | Splinter Review
5.17 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Brian Ryner (not reading)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

13 years ago
It could be bug 97283 that you are seeing. Can you provide specific pages where
this is happening?
(Reporter)

Comment 2

13 years ago
(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

13 years ago
(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? 
(Reporter)

Comment 4

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

Comment 5

13 years ago
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
Blocks: 83552
(Reporter)

Comment 6

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

Comment 7

13 years ago
(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?
(Reporter)

Comment 8

13 years ago
(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

13 years ago
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.
(Reporter)

Comment 10

13 years ago
(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

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

Comment 12

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

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

Comment 14

13 years ago
Created attachment 163842 [details] [diff] [review]
I have a hunch that this bandaid fixes the problem

Will someone test this?
(Reporter)

Comment 15

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

Comment 16

13 years ago
(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.



(Reporter)

Comment 17

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

Comment 18

13 years ago
(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?
(Reporter)

Comment 19

13 years ago
Always just the binary...
(Assignee)

Comment 20

13 years ago
(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

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

Comment 22

13 years ago
If someone who builds from source is experiencing this, please try my patch.

Comment 23

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

Comment 24

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

Comment 25

13 years ago
I can reproduce this bug now, but the following Firefox setting is required:
[x] Select new tabs opened from links
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

13 years ago
Severity: minor → major
Keywords: access, sec508
(Assignee)

Updated

13 years ago
Attachment #163842 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Summary: Scrolling keys (arrows, page up,etc) disabled on some pages when tabbed browsing → Focus lost when key pressed in newly loading foreground tab

Comment 26

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

Comment 27

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

Comment 28

13 years ago
The event is getting handled by the xul document instead of the html doc.
(Assignee)

Comment 29

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

Updated

13 years ago
Attachment #169217 - Flags: superreview?(jst)
Attachment #169217 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #169217 - Flags: superreview?(jst)
Attachment #169217 - Flags: review?
(Assignee)

Comment 30

13 years ago
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
Attachment #169217 - Attachment is obsolete: true
Attachment #169289 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169289 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #169289 - Flags: review? → review?(bryner)
(Assignee)

Comment 31

13 years ago
*** Bug 269278 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 32

13 years ago
Might fix bug 273650.

Comment 33

13 years ago
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 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
Attachment #169289 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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.
Attachment #169289 - Flags: review?(bryner) → review+
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.
(Assignee)

Comment 37

13 years ago
Checking in nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.802; previous revision: 3.801
done
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
OS: Windows 98 → All
(Assignee)

Comment 38

13 years ago
Had to back this out because it caused bug 279285.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 39

13 years ago
This one is difficult. Help would be appreciated. My first fix broke bug 279285. 
Flags: blocking-aviary1.1?

Comment 40

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

13 years ago
Also see bug #264815
Flags: blocking-aviary1.1? → blocking-aviary1.1+
(Assignee)

Comment 42

12 years ago
This is one I think Brian Ryner needs to look at.
Assignee: aaronleventhal → bryner
Blocks: 291034
Status: REOPENED → NEW
Keywords: helpwanted

Comment 43

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

12 years ago
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
Flags: blocking1.8b4+
(Assignee)

Comment 45

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

Comment 46

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

Comment 47

12 years ago
Created attachment 191022 [details] [diff] [review]
A nice easy workaround in tabbrowser.xml using setTimeout(0, ...) to select new tabs
Assignee: bryner → aaronleventhal
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #191022 - Flags: review?(mconnor)
(Assignee)

Comment 48

12 years ago
*** Bug 278527 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 49

12 years ago
*** Bug 301761 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Attachment #191022 - Flags: review?(mconnor)
Attachment #191022 - Flags: review+
Attachment #191022 - Flags: approval1.8b4+
(Assignee)

Comment 50

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED
this one most probably caused 
bug 303475 and bug 303477
(Assignee)

Comment 52

12 years ago
Sounds like we should back it out as an unsuccessful attempt to fix this bug
without creating more problems.

Comment 53

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

Comment 54

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

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 55

12 years ago
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
Attachment #191022 - Attachment is obsolete: true
Attachment #191669 - Flags: review?(mconnor)
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.
Attachment #191669 - Flags: review?(mconnor)
Attachment #191669 - Flags: review?(benjamin)
Attachment #191669 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #191669 - Flags: review?(benjamin) → review?(timeless)

Comment 57

12 years ago
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");
Attachment #191669 - Flags: review?(timeless) → review+
(Assignee)

Updated

12 years ago
Attachment #191669 - Flags: approval1.8b4?

Comment 58

12 years ago
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 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.
Attachment #191669 - Flags: approval1.8b4? → approval1.8b4+

Comment 60

12 years ago
i wonder if this will also fix firefox bug 218216?
Blocks: 218216
(Assignee)

Comment 61

12 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Depends on: 303679
Blocks: 303679
No longer depends on: 303679
(Assignee)

Comment 63

12 years ago
Oy, thanks. Correction checked in.

Updated

12 years ago
Blocks: 303715
(Assignee)

Comment 64

12 years ago
This is still fixed, but keyboard scrolling doesn't always seem to work right
away in the new tab.

Comment 65

12 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 66

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

Updated

12 years ago
Attachment #193158 - Flags: superreview?(bryner)
Attachment #193158 - Flags: review?(mats.palmgren)
(Assignee)

Comment 67

12 years ago
Please ignore the printfs.
(Assignee)

Updated

12 years ago
Attachment #193158 - Attachment is obsolete: true
Attachment #193158 - Flags: superreview?(bryner)
Attachment #193158 - Flags: review?(mats.palmgren)
(Assignee)

Comment 68

12 years ago
Created attachment 193171 [details] [diff] [review]
Ooh! A real fix! When we unsupress painting we have to UpdateCommands for the currently focused window
Attachment #193171 - Flags: superreview?(bryner)
Attachment #193171 - Flags: review?(mats.palmgren)
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: as soon as I get review]
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
Attachment #193171 - Flags: review?(mats.palmgren) → review+

Updated

12 years ago
Severity: major → minor
Attachment #193171 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

12 years ago
Attachment #193171 - Flags: approval1.8b4?
(Assignee)

Comment 70

12 years ago
Fix checked in on trunk

Comment 71

12 years ago
This has regressed Tp on btek and probably Txul and Ts on comet; still waiting
on other tinderboxes to cycle...
(Assignee)

Comment 72

12 years ago
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.
Created attachment 193651 [details] [diff] [review]
wip (untested)

Does this fix Tp?

Comment 74

12 years ago
No way to tell without checking in -- those tests are not really publically
available.
Comment on attachment 193651 [details] [diff] [review]
wip (untested)

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

/be
Attachment #193651 - Flags: superreview+
Attachment #193651 - Flags: review+
(Assignee)

Comment 76

12 years ago
Cool Mats -- that's exactly the solution I thought of. I'll check it in.
(Assignee)

Comment 77

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

Comment 78

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

Comment 79

12 years ago
Comment on attachment 193651 [details] [diff] [review]
wip (untested)

Patch breaks original fix. Looking into why.
Attachment #193651 - Flags: review+ → review-
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: as soon as I get review] → [ETA: as soon as we work at perf issues of new fix -- will test on trunk ]
(Assignee)

Comment 80

12 years ago
Created attachment 193707 [details] [diff] [review]
Similar to Mats' patch, but only does an UpdateCommands() if the doc can handle the update events
Attachment #193707 - Flags: superreview?(bryner)
Attachment #193707 - Flags: review?(mats.palmgren)
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: as soon as we work at perf issues of new fix -- will test on trunk ] → [ETA: need review for perf fixes for trunk checkin -- we'll see if it works before checking in on branch]
(Assignee)

Comment 81

12 years ago
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
Attachment #193707 - Attachment is obsolete: true
Attachment #193707 - Flags: superreview?(bryner)
Attachment #193707 - Flags: review?(mats.palmgren)
(Assignee)

Comment 82

12 years ago
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.
Attachment #193709 - Flags: superreview?(bryner)
Attachment #193709 - Flags: review?(mats.palmgren)

Comment 83

12 years ago
does that old approval request still apply? If not, please unset it. this should
land on the trunk and bake before we consider it. 
(Assignee)

Updated

12 years ago
Attachment #193171 - Flags: approval1.8b4?
Attachment #193709 - Flags: superreview?(bryner) → superreview+
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.
Attachment #193709 - Flags: superreview?(bryner)
Attachment #193709 - Flags: superreview+
Attachment #193709 - Flags: review?(mats.palmgren)
Attachment #193709 - Flags: review+
(Assignee)

Comment 85

12 years ago
Perf fix checked into the trunk. Let's see how it does.
(Assignee)

Comment 86

12 years ago
Yes! The Tp, Txul and Ts numbers went back to normal.
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: need review for perf fixes for trunk checkin -- we'll see if it works before checking in on branch] → [ETA: perf #s are back to normal, will see if any regressions popup tomorrow before seeking a=]
Attachment #193709 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

12 years ago
Attachment #193171 - Flags: approval1.8b4?
(Assignee)

Updated

12 years ago
Attachment #193709 - Flags: approval1.8b4?
Is this all fixed on the trunk?  If so, please mark FIXED so we can start
watching for reports of regression, etc.
(Assignee)

Comment 88

12 years ago
I'll file a new bug for what Mats reported in comment 84.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 89

12 years ago
Filed bug 305939 for comment 84
(Assignee)

Updated

12 years ago
Attachment #193651 - Attachment is obsolete: true

Updated

12 years ago
Blocks: 305939
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: perf #s are back to normal, will see if any regressions popup tomorrow before seeking a=] → [ETA: perf #s are back to normal, it looks like the recent regressions involving text fields related to bug 303620, not this one]

Updated

12 years ago
Attachment #193709 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 90

12 years ago
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.
Attachment #193171 - Flags: approval1.8b4?
(Assignee)

Updated

12 years ago
Keywords: fixed1.8

Comment 91

12 years ago
Could this possibly have caused bug 306076?

Comment 92

12 years ago
OK, I've verified via explicit backout that the last patch in this bug caused
bug 306076.
Depends on: 306076
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.
(Assignee)

Comment 94

12 years ago
(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.

Updated

12 years ago
No longer blocks: 305939
Depends on: 311607

Comment 95

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

12 years ago
Yeah, that sounds like food for a new bug...

Comment 97

12 years ago
OK; https://bugzilla.mozilla.org/show_bug.cgi?id=324349

Updated

11 years ago
Depends on: 352446

Comment 98

11 years ago
The last patch in this bug also caused bug 249136.

Updated

10 years ago
Depends on: 416710
You need to log in before you can comment on or make changes to this bug.