361698 - Navigation menuitems don't work in the sidebar's context menu

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[Asaf Romano (gone)]

Navigation menuitems (Back/Forward/Reload/Stop) don't work in the sidebar's context menu

Comment 1

[Asaf Romano (gone)]

Attachment: patch

I will fix the stop command enabled state in bug 361686. I didn't bother tracking about:blank navigations for disabling the Reload command.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 246443 [details] [diff] [review]
patch

This currently breaks because MOZ_PLACES is now on, so Ci/Cc/Cr are no longer defined in browser.js, which means they aren't defined in web-panels.xul's scope. We need to fix that somehow...

>Index: browser/base/content/web-panels.js

> function loadWebPanel(aURI) {

>+  var panelBrowser = getPanelBrowser;

oops?

>Index: browser/base/content/web-panels.xul

>   <popupset id="mainPopupSet">
>     <popup id="contentAreaContextMenu"
>            onpopupshowing="if (event.target != this) return true; gContextMenu = new nsContextMenu(this, document.getElementById('web-panels-browser')); return gContextMenu.shouldDisplay;"

This can use getPanelBrowser() too.

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 249535 [details] [diff] [review]
patch

The sidebar context menu is still broken because of Cc/Ci not being defined. For now, we should probably just add |var Cc = Components.classes; var Ci = Components.interfaces;| to the top of web-panels.js, but we should figure out a better long term solution. r=me with that change for now.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 249535 [details] [diff] [review]
patch

>Index: browser/base/content/web-panels.js

> function loadWebPanel(aURI) {

>+    if (gLoadFired) {
>+      panelBrowser.webNavigation.loadURI(aURI, nsIWebNavigation.LOAD_FLAGS_NONE,
>+                                         null, null, null);

Nit: indentation is wrong here (needs two more spaces).

Comment 6

[Asaf Romano (gone)]

I would rather overlay it with placesOverlay.js so we nsContextMenu can depend on places-functionality.

Comment 7

[Asaf Romano (gone)]

placesOverlay.xul

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[Asaf Romano (gone)]

mozilla/browser/base/content/browser.js 1.758
mozilla/browser/base/content/nsContextMenu.js 1.7
mozilla/browser/base/content/web-panels.js 1.17
mozilla/browser/base/content/web-panels.xul 1.20