Closed Bug 427559 Opened 12 years ago Closed 12 years ago

Gmail keyboard commands don't work after switching tabs without reclicking the page

Categories

(Firefox :: Tabbed Browser, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: Mardak, Assigned: Mardak)

References

(Blocks 1 open bug, )

Details

(Keywords: testcase)

Attachments

(3 files, 3 obsolete files)

Pages like gmail that use key(press?) event listeners don't work if you switch tabs away after clicking links.

See testcase (url)

1. Click the page
2. Hit stuff on the keyboard
3. Ctrl/cmd click this link
4. Ctrl/cmd-2 to switch to next tab
5. Ctrl/cmd-1 to return to this tab
6. Hit stuff on the keyboard

Note: after step 3, hitting the keyboard still does things correctly
Only after switching tabs and returning does things not work.

This is one of the top complaints I've heard about Firefox 3 where you have to click somewhere on the page before keyboard shortcuts work again.

I don't think this is a regression, but it's pretty annoying for advanced gmail users. Any idea if websites can work around this issue? blocking-firefox3? but it's probably too late unless there's an easy fix.

data:text/html,<html><head><script>addEventListener('keypress', function(e) {with(document.body)innerHTML=['You pressed ',e.charCode,'<br/>',innerHTML].join('');e.preventDefault()}, true)</script></head><body><a href="http://www.mozilla.com/">1. Click the page<br/>2. Hit stuff on the keyboard<br/>3. Ctrl/cmd click this link<br/>4. Ctrl/cmd-2 to switch to next tab<br/>5. Ctrl/cmd-1 to return to this tab<br/>6. Hit stuff on the keyboard</a></body></html>
Flags: blocking-firefox3?
I'm sure I saw a duplicate on this, but I can't find it ...
This this an events bug or a front end bug?
Flags: blocking-firefox3? → blocking-firefox3+
Last I checked, the front end is what does the focus saving/restoration (which is what I assume is the issue here).

Of course there could be a core focus bug too.  Wouldn't be the first time.
It seems to be the process of switching away from the tab using the keyboard that is causing problems.

click link, ctrl-tab, ctrl-shift-tab -> broken
click link, ctrl-tab, click orig tab -> broken
click link, click next tab, ctrl-shift-tab -> ok
click link, click next tab, click orig tab -> ok

I'm guessing it has something to do with tabbox.xml calling

event.stopPropagation()
event.preventDefault()

which we want to do to prevent the page from getting it, but something on the firefox side is missing out and then getting confused.
I think I might have accidentally recreated the gmail problem with how I did the testcase -- it changes innerHTML as you type stuff, so the nodes actually disappear.

The code in tabbrowser.xml for updateCurrentBrowser correctly calls focus(), but because the node actually is gone, it fails to focus the link.

And now that I think about it, I see the issue more when I archive email messages which potentially changes stuff around.. ?
Attached patch v1 (obsolete) — Splinter Review
Make sure the focused element still has a parent to consider it as a candidate for a thing to focus.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #315332 - Flags: review?(gavin.sharp)
Couldn't a node have a parent and yet not be part of a document?
Keywords: testcase
Perhaps it would be best to check for a non-null ownerDocument?
focusedElement.ownerDocument isn't null in the testcase; it's still the document

But.. focusedElement != fE.oD.lastChild.lastChild (the anchor element in the document) So they are two different elements.
You don't want ownerDocument here, you want the current document, which unfortunately isn't available through any scriptable interfaces. One way to do this would be to check whether the node is still a descendant of the owner document using document.compareDocumentPosition().
Attached patch v2 (obsolete) — Splinter Review
Thanks for the tips jst.

I finally figured out a 100% reliable STR:
1) Open a email message in gmail
2) Hit 'r' (for reply)
3) Click "Discard" (focus is on the button)
4) Change tabs then change back to gmail
5) Hit 'r' and see that it does nothing

(patch v1 didn't fix this particular case because the node still has a parent node)
Attachment #315332 - Attachment is obsolete: true
Attachment #315336 - Flags: review?(gavin.sharp)
Attachment #315332 - Flags: review?(gavin.sharp)
Attachment #315336 - Flags: review?(gavin.sharp) → review+
Comment on attachment 315336 [details] [diff] [review]
v2

a1.9? for RC1 for annoying gmail bug that's been around since... before firefox 2?

litmus test either uses the data url here or the gmail example in comment 11
Attachment #315336 - Flags: approval1.9?
Is elem.ownerDocument guaranteed to be non-null?
Comment on attachment 315336 [details] [diff] [review]
v2

We've discussed things, and Mardak's gonna come up with a test for this to ensure we don't bustificate it in the future. When that's ready, they'll both go in.
Attachment #315336 - Flags: approval1.9? → approval1.9-
> Is elem.ownerDocument guaranteed to be non-null?

At the moment, not really.  That's a bug that we'll fix one day...

Worse yet, it could have an owner document and be in that document... but that document might not be the document in that tab.  Or in any subframe.  Not sure what exactly we want to do in that case.
Attached patch v2.1 (obsolete) — Splinter Review
With extra ownerDocument check and testcase.
Attachment #315336 - Attachment is obsolete: true
Attachment #315345 - Flags: review?(gavin.sharp)
Neil, Seamonkey might need a similar change.
Attached patch tweaked testSplinter Review
This one's a bit simpler. I was seeing strange behavior with the test in v2.1, and it seemed to pass even without the patch applied.
Attachment #315345 - Attachment is obsolete: true
Attachment #315351 - Flags: review?(edilee)
Attachment #315345 - Flags: review?(gavin.sharp)
I suppose we should add a null check for ownerDocument as well. I'm not sure I care about fixing this bug when the focused node's ownerDocument isn't in the tab - when would that happen?
Whenever someone has adopted the node into a different document, say.
Comment on attachment 315351 [details] [diff] [review]
tweaked test

I've been twiddling around with this testcase, but the windows don't match up for me (os x). If I put a timeout of 1000, it works okay. Timeout of 100 doesn't seem to work.
Comment on attachment 315351 [details] [diff] [review]
tweaked test

Looks reasonable. (window.content, testBrowser.contentWindow, testBrowser.focusedWindow all point to the same thing!) I'll attach a patch with fixed up comments.
Attachment #315351 - Flags: review?(edilee) → review+
Attached patch v2.2Splinter Review
Adds test per comment #14.
Attachment #315428 - Flags: approval1.9?
Comment on attachment 315428 [details] [diff] [review]
v2.2

a=mconnor on behalf of 1.9 drivers
Attachment #315428 - Flags: approval1.9? → approval1.9+
Whiteboard: [has patch][has reviews]
Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.271; previous revision: 1.270
done
Checking in browser/base/content/test/Makefile.in;
/cvsroot/mozilla/browser/base/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/browser/base/content/test/browser_bug427559.js,v
done
Checking in browser/base/content/test/browser_bug427559.js;
/cvsroot/mozilla/browser/base/content/test/browser_bug427559.js,v  <--  browser_bug427559.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Priority: -- → P2
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → Firefox 3
Delay the test to let the page script do its job. Passes with fix applied and fails without the tabbrowser.xml change.

Checking in browser/base/content/test/Makefile.in;
/cvsroot/mozilla/browser/base/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
Checking in browser/base/content/test/browser_bug427559.js;
/cvsroot/mozilla/browser/base/content/test/browser_bug427559.js,v  <--  browser_bug427559.js
new revision: 1.2; previous revision: 1.1
done
Since this bug's test was re-enabled, it's failed twice in a row on the centos5 test box:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1208208233.1208211849.16270.gz

FAIL - content window is focused - Got [object ChromeWindow], expected [object XPCNativeWrapper [object Window]] - chrome://mochikit/content/browser/browser/base/content/test/browser_bug427559.js

Ed, can you disable the test again and/or fix whatever's wrong ASAP?
Verified with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041807 Minefield/3.0pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041804 Minefield/3.0pre ID:2008041804

This also fixes the shortcuts for Google Reader. Hitting 'j' or 'k' directly after closing an opened tab goes to the next/former post.
Status: RESOLVED → VERIFIED
I'm hitting a problem where something I do in my testcase breaks the testcase from this bug. I've posted about it in the newsgroups and if anyone has any suggestions I'd appreciate it:

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/18ba18fe0690621b#
Blocks: 459394
You need to log in before you can comment on or make changes to this bug.