Closed Bug 427559 Opened 17 years ago Closed 17 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3

People

(Reporter: Mardak, Assigned: Mardak)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: testcase)

Attachments

(3 files, 3 obsolete files)

tweaked test
(deleted), patch
Mardak
: review+
Details | Diff | Splinter Review
v2.2
(deleted), patch
mconnor
: approval1.9+
Details | Diff | Splinter Review
testcase followup v1
(deleted), patch
Details | Diff | Splinter Review
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.. ?
URL: data:text/html,<html><head><script>ad...
Attached patch v1 (obsolete) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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 test (deleted) — — Splinter 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.2 (deleted) — — Splinter 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: 17 years ago
Flags: in-testsuite+
Priority: -- → P2
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → Firefox 3
Attached patch testcase followup v1 (deleted) — — Splinter Review
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.

Attachment

General

Creator:
Created:
Updated:
Size: