Closed
Bug 910288
Opened 11 years ago
Closed 11 years ago
The list of disabled commands in detached chat windows isn't correct
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mixedpuppy
:
review+
|
Details | Diff | Splinter Review |
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).
Attachment #796715 -
Flags: review?(mixedpuppy)
Comment 1•11 years ago
|
||
Comment on attachment 796715 [details] [diff] [review]
Patch
looks good.
Attachment #796715 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
(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.
Comment 6•11 years ago
|
||
(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 :/)
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•