301804 - fastback landing broke shortcut keys during some stages of page load

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 1

[Aaron Leventhal]

Brian, can you look at this?

Comment 2

[Aaron Leventhal]

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.

Comment 3

[Brian Ryner (not reading)]

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?

Comment 4

[Aaron Leventhal]

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

Have you tried that to see if things still run normally?

Comment 5

[Asa Dotzler [:asa]]

*** Bug 303360 has been marked as a duplicate of this bug. ***

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

This fixes it for me.

Comment 7

[Aaron Leventhal]

Comment on attachment 191728 [details] [diff] [review]
patch

Cool, makes sense.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 191728 [details] [diff] [review]
patch

requesting approval... this is a very safe fix

Comment 9

[Aaron Leventhal]

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.

Comment 10

[Brian Ryner (not reading)]

checked in.

Aaron, I think you might be seeing but 298077, which regressed from the
split-window landing.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 .

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 13

[Brian Ryner (not reading)]

I can't reproduce the bug as described on Windows, is there a Linux-specific
problem?

Comment 14

[Brian Ryner (not reading)]

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.

Comment 15

[Brian Ryner (not reading)]

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.

Comment 16

[Aaron Leventhal]

David, go ahead and reopen if Brian's comment 15 is not what you're experiencing.