656886 - Context menu broken in non-browser sidebars

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Install a web sidebar
2. Open an editor or compose window, and display the sidebar
3. Right-click in the sidebar

Expected result: context menu

Actual result: menu shows everything and doesn't work.

Comment 1

[Philip Chee]

Perhaps?
http://hg.mozilla.org/comm-central/rev/910bed11ccfc
Bug 655863 - Port |Bug 581475 - nsContextMenu cleanup| r=neil

Comment 2

[[e-mail]]

Attachment: Possible patch

This uses code similar to getNotificationBox() to find the browser.
(Note: this is the global one in navigator.js, not the tabbrowser one.)
The wrappedJSObject isn't needed any more because of the XRayWrapper changes.

Comment 3

[Ian Neal]

Comment on attachment 532175 [details] [diff] [review]
Possible patch

>+++ b/suite/common/nsContextMenu.js

> nsContextMenu.prototype = {
>   initMenu: function(aBrowser) {
Nit: aBrowser argument could be removed now.

r=me with that addressed

A 2.1 branch version would be good too.

Comment 4

[[e-mail]]

Attachment: Addressed review comments

Comment 5

[[e-mail]]

Attachment: Branch patch

Comment 6

[[e-mail]]

Pushed changeset ffc7c66a1797 to comm-central.

Comment 7

[Ian Neal]

Comment on attachment 532197 [details] [diff] [review]
Branch patch

Issue shows up in 2.1 branch for custom/web sidebar tabs.

Comment 8

[Justin Wood (:Callek)]

(In reply to comment #3)
> 
> r=me with that addressed
> 
> A 2.1 branch version would be good too.

Umm, why a 2.1 branch version. Is this enough in itself to make delaying 2.1 necessary with an rc2? Based on my reading I think not.

And I'd rather not land actual code changes on 2.1 without a plan to do a 2.1 update. And the idea of a chemspill here wouldn't warrant a ridealong since we would release off relbranch.

Comment 9

[Philip Chee]

Comment on attachment 532197 [details] [diff] [review]
Branch patch

a=me If we can get this in without giving Callek heartburn.

Comment 10

[Justin Wood (:Callek)]

(In reply to comment #9)
> Comment on attachment 532197 [details] [diff] [review] [review]
> Branch patch
> 
> a=me If we can get this in without giving Callek heartburn.

I'll veto temporarily...

IFF we need to respin, we can take this as a ridealong, failing a respin for another reason I think we need clear reasoning for why this needs to go in, and if its a "we'll respin for this" etc.

We will likely have a release (2.2) within the next month and a half with the rapid release cycle...

Comment 11

[Philip Chee]

Comment on attachment 532197 [details] [diff] [review]
Branch patch

Cancelling approval as per Callek.

Comment 12

[Ian Neal]

If it doesn't make it as a ridealong for 2.1 then it will probably need to be relnoted.

Comment 13

[Ian Neal]

Comment on attachment 532197 [details] [diff] [review]
Branch patch

Re-requesting if we're having a respin...

Comment 14

[Philip Chee]

Comment on attachment 532197 [details] [diff] [review]
Branch patch

a=me (second time lucky?)

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Verified with <http://edmullen.net/mozilla/sidebarpreview.php>, entering <http://edmullen.net/sidebar/clockbar.php>

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

http://hg.mozilla.org/releases/comm-2.0/rev/38c33bdf94d3
http://hg.mozilla.org/releases/comm-2.0/rev/188a5250e434

Comment 17

[therube]

Is this related? DUP?  Related but different?

Bug 104590 - Back in context menu not working correctly when in sidebar

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #17)
> Is this related? DUP?  Related but different?
> 
> Bug 104590 - Back in context menu not working correctly when in sidebar

No (other than being about the sidebar, too, this is about the context menu itself being broken, and that one is about a particular functionality available from the context menu, and is much older).