Since bug 898161, I noticed a 'document.getElementById(...) is null' error in chatWindow.xul when I detach a SocialAPI chat window on Windows or Linux. The cause is a few elements that don't exist in the DOM because they were included as part of browser-menubar.inc: 'viewToolbarsMenu', 'viewSidebarMenuMenu', 'viewFullZoomMenu', 'pageStyleMenu', 'charsetMenu'. I took the opportunity of patching this list of disabled elements to review the whole list of enabled keyboard shortcuts in detached chat windows, and found some commands that I think we should disable: These 2 commands open dialogs that don't seem appropriate above the chat window: Browser:OpenFile, Tools:Sanitize (accel,shift,VK_DELETE) These 2 are broken because window.content is undefined in chat windows. View:PageSource View:PageInfo I think they make sense to keep enabled, so I defined a 'content' getter to make them work (this seemed easier than getting the 'content' property we get from C++ to return the right value -- that would likely involve implementing nsIBrowserDOMWindow). These commands are broken because they expect gBrowser: Tools:DevToolbox key_selectTab{1,2,3,4,5,6,7,8,LastTab} cmd_findPrevious (ctrl+shift+g or Shift+f3) cmd_fullZoomReduce, cmd_fullZoomEnlarge, cmd_fullZoomReset Irrelevant for our case (and broken: 'sidebar is null' error) : viewBookmarksSidebar (accel+b) These 2 commands work fine on Mac (because we have the menu bar), and are broken elsewhere: Browser:OpenLocation Tools:Search Note: I tested this on Linux and Mac, but haven't tested on Windows yet (I expect the behavior there to be identical to Linux).

Comment on attachment 796715 [details] [diff] [review] Patch looks good.

(In reply to Florian Quèze [:florian] [:flo] from comment #0) > These 2 are broken because window.content is undefined in chat windows. > View:PageSource > View:PageInfo > I think they make sense to keep enabled, so I defined a 'content' getter to > make them work (this seemed easier than getting the 'content' property we > get from C++ to return the right value -- that would likely involve > implementing nsIBrowserDOMWindow). I don't see why it would involve that, seems to me like window.content should work here without the need for your change.

(In reply to :Gavin Sharp from comment #4) > > I defined a 'content' getter to > > make them work (this seemed easier than getting the 'content' property we > > get from C++ to return the right value -- that would likely involve > > implementing nsIBrowserDOMWindow). > > I don't see why it would involve that, seems to me like window.content > should work here without the need for your change. I traced the window.content implementation to: http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/dom/interfaces/base/nsIDOMJSWindow.idl#l77 http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/dom/base/nsGlobalWindow.cpp#l3595 Then I likely made a few assumptions to get to: http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/xpfe/appshell/src/nsChromeTreeOwner.cpp#l254 and finally reached: http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/browser/base/content/browser.js#l4450 which can't work because gBrowser isn't defined. It's not completely clear how gBrowser.contentWindow; would result in an undefined value rather than an exception, but I guess I somehow assumed one of the C++ methods didn't propagate the nsresult error. If you think this is worth investigating further to be 100% sure of where the undefined value is created, I can file another bug, and poke at this a bit with gdb.

(In reply to Florian Quèze [:florian] [:flo] from comment #5) > I traced the window.content implementation to: > http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/dom/interfaces/ > base/nsIDOMJSWindow.idl#l77 > http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/dom/base/ > nsGlobalWindow.cpp#l3595 > Then I likely made a few assumptions to get to: > http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/xpfe/appshell/ > src/nsChromeTreeOwner.cpp#l254 > and finally reached: > http://hg.mozilla.org/mozilla-central/annotate/c7cc85e13f7a/browser/base/ > content/browser.js#l4450 > which can't work because gBrowser isn't defined. That suggests to me that the chat window should have an implementation of nsIBrowserDOMWindow, but maybe that has other implications. Can you file a followup to investigate that? (The chatwindow being a subset of browser.xul is rather complicated/confusing :/)

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6) > Can you file a followup to investigate that? Filed bug 921011.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #6) > That suggests to me that the chat window should have an implementation of > nsIBrowserDOMWindow Indeed, we need an nsIBrowserDOMWindow implementation to handle opening links in new tabs. Patch attached in bug 921011.