Closed Bug 301804 Opened 15 years ago Closed 15 years ago

fastback landing broke shortcut keys during some stages of page load

Categories

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

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: dbaron, Assigned: bryner)

References

Details

(Keywords: regression)

Attachments

(1 file)

The fastback landing on 2005-05-04 broke the use of shortcut keys during some
stages of pageload.  I'm presuming it's fastback; in theory it could have been
some other checkin the same day.  This is a regression of a rather old and very
noisy bug, bug 110718.  The bug is present whether or not fastback is enabled. 
It seems to me that all shortcut keys simply do not work during some stages of
pageload.  I think it's irrelevant whether it's a new load or a load from
history (back/forward), but I'm not sure, since it's a little difficult to
reproduce reliably and I've so far only found a way to reproduce reliably for
history loads (see below).

This is a rather annoying user problem:  I've noticed it mostly when I try to
use Ctrl-T (new tab) or Ctrl-W (close tab) while a page is loading.  (I'll be
describing GTK2 build shortcuts throughout; please translate as appropriate to
your platform.  I haven't actually tested whether the bug is present on other
platforms.)  However, I've found that a much easier way to reproduce the problem
is to press the shortcut for back (alt-leftarrow) repeatedly:  this should go
back the number of pages that the shortcut is pressed; however, since
2005-05-05, it has gone back fewer pages.

Steps to reproduce:
 1. load this page
 2. Press Ctrl-L to focus the location bar
 3. Enter www.nytimes.com, press enter, and wait for it to load
 4. repeat (2)
 5. repeat (3) for www.cnn.com
 6. repeat (2)
 7. repeat (3) for www.washingtonpost.com
 8. hold down the alt key and press the left arrow quickly three times

Expected results:
 8. you arrive back at this page

Actual results:
 8. you generally end up at www.cnn.com or www.nytimes.com since some of the
shortcut key keypresses were ignored

Works in:
 Linux firefox nightly, 2005-05-04-09-trunk, clean profile

Broken in:
 Linux firefox nightly, 2005-05-05-11-trunk, clean profile (i.e., fastback
   disabled)
 Linux firefox build from source, sometime in the past few days, with fastback
   enabled (browser.sessionhistory.max_viewers == 3) or disabled (... == 0)
Flags: blocking1.8b4?
Brian, can you look at this?
Assignee: events → bryner
Blocks: deera11y
Blocks: branching1.8
Flags: blocking1.8b4? → blocking1.8b4+
Target Milestone: --- → mozilla1.8beta4
Another unresponsive keyboard issue probably from fastback is bug 299514.
See comment 12 -- https://bugzilla.mozilla.org/show_bug.cgi?id=299514#c12
However, the commenters do not suggest this happens only during page load.
I suspect the difference is because of the timing of the UnbindFromTree calls in
the DOM tree.  We used to call this when the docviewer was closed, now we don't
do it until the document is destroyed (which isn't until the new content viewer
is shown and destroys its mPreviousViewer).  This would directly impact the
results of PresShell::InZombieDocument.  My feeling is that we could solve this
by changing the condition to:

{
nsIDocument *doc = aContent->GetDocument();
return !doc || !doc->GetScriptGlobalObject();
}

Any thoughts on this?
Where is this condition currently?
> nsIDocument *doc = aContent->GetDocument();
> return !doc || !doc->GetScriptGlobalObject();

Have you tried that to see if things still run normally?
*** Bug 303360 has been marked as a duplicate of this bug. ***
Attached patch patchSplinter Review
This fixes it for me.
Attachment #191728 - Flags: review?(aaronleventhal)
Comment on attachment 191728 [details] [diff] [review]
patch

Cool, makes sense.
Attachment #191728 - Flags: review?(aaronleventhal) → review+
Comment on attachment 191728 [details] [diff] [review]
patch

requesting approval... this is a very safe fix
Attachment #191728 - Flags: approval1.8b4?
Brian, it can also be done as a separate fix but I noticed that we're not
focusing the document after an alt+left arrow like we used to. This means you
can't even scroll the page with arrow keys. The lack of focus event is also
messing with the accessibility API module.
Attachment #191728 - Flags: superreview+
Attachment #191728 - Flags: approval1.8b4?
Attachment #191728 - Flags: approval1.8b4+
checked in.

Aaron, I think you might be seeing but 298077, which regressed from the
split-window landing.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Hmmm.  This doesn't seem to fix the original steps to reproduce, and it also may
have triggered a crash (which I've only seen once) with a null container in
PresShell::RetargetEventToParent .
Reopening, since I can still reproduce as described in the original steps to
reproduce.  (Though I haven't ended up going back only once -- I've ended up
going back twice all three times I tried, but I should go back all three pages.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer depends on: 303726
I can't reproduce the bug as described on Windows, is there a Linux-specific
problem?
I do see the problem on Linux... as best I can tell, what happens is that the
third Go[Back|Forward] call happens before we've ever had CreateContentViewer
called for the second navigation.  That means nsSHistory::UpdateIndex is never
called for the second navigation, so the third GoForward just loads the same entry.

I'm not sure exactly what would have changed here as a result of the fastback
landing though.  I'm pulling a pre-fastback tree to investigate.
I'm able to reproduce the problem I described above with a tree pulled before
fastback landed.  I think you're actually noticing bug 227466.
No longer blocks: branching1.8
David, go ahead and reopen if Brian's comment 15 is not what you're experiencing.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.