Closed
Bug 249136
Opened 20 years ago
Closed 19 years ago
Focus lost when key pressed in newly loading foreground tab
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: o0ngziz02, Assigned: aaronlev)
References
(Blocks 1 open bug)
Details
(Keywords: access, fixed1.8, helpwanted, 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 files, 6 obsolete files)
(deleted),
patch
|
bryner
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mconnor
:
review+
timeless
:
review+
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
bryner
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
bryner
:
superreview+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
It could be bug 97283 that you are seeing. Can you provide specific pages where this is happening?
Reporter | ||
Comment 2•20 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•20 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•20 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•20 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: focusnav
Reporter | ||
Comment 6•20 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•20 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•20 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•20 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•20 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•20 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•20 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•20 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•20 years ago
|
||
Will someone test this?
Reporter | ||
Comment 15•20 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•20 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•20 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•20 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•20 years ago
|
||
Always just the binary...
Assignee | ||
Comment 20•20 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•20 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•20 years ago
|
||
If someone who builds from source is experiencing this, please try my patch.
Comment 23•20 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•20 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•20 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•20 years ago
|
Assignee | ||
Updated•20 years ago
|
Attachment #163842 -
Attachment is obsolete: true
Assignee | ||
Updated•20 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•20 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•20 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•20 years ago
|
||
The event is getting handled by the xul document instead of the html doc.
Assignee | ||
Comment 29•20 years ago
|
||
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•20 years ago
|
Attachment #169217 -
Flags: superreview?(jst)
Attachment #169217 -
Flags: review?
Assignee | ||
Updated•20 years ago
|
Attachment #169217 -
Flags: superreview?(jst)
Attachment #169217 -
Flags: review?
Assignee | ||
Comment 30•20 years ago
|
||
Attachment #169217 -
Attachment is obsolete: true
Attachment #169289 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169289 -
Flags: review?
Assignee | ||
Updated•20 years ago
|
Attachment #169289 -
Flags: review? → review?(bryner)
Assignee | ||
Comment 31•20 years ago
|
||
*** Bug 269278 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 32•20 years ago
|
||
Might fix bug 273650.
Comment 33•20 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 34•20 years ago
|
||
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 35•20 years ago
|
||
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 36•20 years ago
|
||
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•20 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
Closed: 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
OS: Windows 98 → All
Assignee | ||
Comment 38•20 years ago
|
||
Had to back this out because it caused bug 279285.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 39•20 years ago
|
||
This one is difficult. Help would be appreciated. My first fix broke bug 279285.
Flags: blocking-aviary1.1?
Comment 40•20 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•20 years ago
|
||
Also see bug #264815
Updated•20 years ago
|
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Assignee | ||
Comment 42•20 years ago
|
||
This is one I think Brian Ryner needs to look at.
Comment 43•19 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•19 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•19 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•19 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•19 years ago
|
||
Assignee: bryner → aaronleventhal
Status: NEW → ASSIGNED
Assignee | ||
Updated•19 years ago
|
Attachment #191022 -
Flags: review?(mconnor)
Assignee | ||
Comment 48•19 years ago
|
||
*** Bug 278527 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 49•19 years ago
|
||
*** Bug 301761 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Attachment #191022 -
Flags: review?(mconnor)
Attachment #191022 -
Flags: review+
Attachment #191022 -
Flags: approval1.8b4+
Assignee | ||
Comment 50•19 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
Closed: 20 years ago → 19 years ago
Resolution: --- → FIXED
Comment 51•19 years ago
|
||
this one most probably caused bug 303475 and bug 303477
Assignee | ||
Comment 52•19 years ago
|
||
Sounds like we should back it out as an unsuccessful attempt to fix this bug without creating more problems.
Comment 53•19 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•19 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•19 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 55•19 years ago
|
||
Attachment #191022 -
Attachment is obsolete: true
Attachment #191669 -
Flags: review?(mconnor)
Comment 56•19 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 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•19 years ago
|
Attachment #191669 -
Flags: review?(benjamin) → review?(timeless)
Comment 57•19 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•19 years ago
|
Attachment #191669 -
Flags: approval1.8b4?
Comment 58•19 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 59•19 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 a=me presuming that this actually does fix the problem and comment #58 is mistaken.
Attachment #191669 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 61•19 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
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment 62•19 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 >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.
Updated•19 years ago
|
Assignee | ||
Comment 63•19 years ago
|
||
Oy, thanks. Correction checked in.
Assignee | ||
Comment 64•19 years ago
|
||
This is still fixed, but keyboard scrolling doesn't always seem to work right away in the new tab.
Comment 65•19 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•19 years ago
|
||
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•19 years ago
|
Attachment #193158 -
Flags: superreview?(bryner)
Attachment #193158 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 67•19 years ago
|
||
Please ignore the printfs.
Assignee | ||
Updated•19 years ago
|
Attachment #193158 -
Attachment is obsolete: true
Attachment #193158 -
Flags: superreview?(bryner)
Attachment #193158 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 68•19 years ago
|
||
Attachment #193171 -
Flags: superreview?(bryner)
Attachment #193171 -
Flags: review?(mats.palmgren)
Assignee | ||
Updated•19 years ago
|
Whiteboard: [ETA: as soon as I get review]
Comment 69•19 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 Looks good. r=mats
Attachment #193171 -
Flags: review?(mats.palmgren) → review+
Updated•19 years ago
|
Severity: major → minor
Updated•19 years ago
|
Attachment #193171 -
Flags: superreview?(bryner) → superreview+
Assignee | ||
Updated•19 years ago
|
Attachment #193171 -
Flags: approval1.8b4?
Assignee | ||
Comment 70•19 years ago
|
||
Fix checked in on trunk
Comment 71•19 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•19 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.
Comment 73•19 years ago
|
||
Does this fix Tp?
Comment 74•19 years ago
|
||
No way to tell without checking in -- those tests are not really publically available.
Comment 75•19 years ago
|
||
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•19 years ago
|
||
Cool Mats -- that's exactly the solution I thought of. I'll check it in.
Assignee | ||
Comment 77•19 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•19 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•19 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•19 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•19 years ago
|
||
Attachment #193707 -
Flags: superreview?(bryner)
Attachment #193707 -
Flags: review?(mats.palmgren)
Assignee | ||
Updated•19 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•19 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•19 years ago
|
||
Attachment #193709 -
Flags: superreview?(bryner)
Attachment #193709 -
Flags: review?(mats.palmgren)
Comment 83•19 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•19 years ago
|
Attachment #193171 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #193709 -
Flags: superreview?(bryner) → superreview+
Comment 84•19 years ago
|
||
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•19 years ago
|
||
Perf fix checked into the trunk. Let's see how it does.
Assignee | ||
Comment 86•19 years ago
|
||
Yes! The Tp, Txul and Ts numbers went back to normal.
Assignee | ||
Updated•19 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=]
Updated•19 years ago
|
Attachment #193709 -
Flags: superreview?(bryner) → superreview+
Assignee | ||
Updated•19 years ago
|
Attachment #193171 -
Flags: approval1.8b4?
Assignee | ||
Updated•19 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•19 years ago
|
||
I'll file a new bug for what Mats reported in comment 84.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 89•19 years ago
|
||
Filed bug 305939 for comment 84
Assignee | ||
Updated•19 years ago
|
Attachment #193651 -
Attachment is obsolete: true
Assignee | ||
Updated•19 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•19 years ago
|
Attachment #193709 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 90•19 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?
Comment 91•19 years ago
|
||
Could this possibly have caused bug 306076?
Comment 92•19 years ago
|
||
OK, I've verified via explicit backout that the last patch in this bug caused bug 306076.
Depends on: 306076
Comment 93•19 years ago
|
||
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•19 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.
Comment 95•19 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•19 years ago
|
||
Yeah, that sounds like food for a new bug...
Comment 97•19 years ago
|
||
OK; https://bugzilla.mozilla.org/show_bug.cgi?id=324349
Comment 98•18 years ago
|
||
The last patch in this bug also caused bug 249136.
Updated•5 years ago
|
Component: Keyboard: Navigation → User events and focus handling
Comment 99•5 years ago
|
||
Keywords: sec508
You need to log in
before you can comment on or make changes to this bug.
Description
•