Mark Hammond [:markh] [:mhammond] Reporter
	Description • 12 years ago

Right click on the social sidebar and nothing happens.  A context menu should appear.  What the contents of the menu should be isn't clear, but *something* should happen.  FWIW, the addon had a single "Refresh" item on the menu which turned out to be useful when things got into a confused state...

	Jared Wein [:jaws] (please needinfo? me)
	Comment 1 • 12 years ago

We could do the standard copy/cut/paste commands, but I think the rest of the commands should be left up to the provider by using the html5 context menu APIs.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 3 • 12 years ago

An initial run using the browser context menu on social frames.  This is working, but will have to pick through each menuitem and decide if it is correct for social frames.  Several things work (e.g. page info, spell check), several dont (eg. view source).  Want feedback on direction of patch, but IMO this is the way to go.  btw, html 5 menus in a provider work as well.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 4 • 12 years ago

I think it's probably better to be conservative here and have a separate, more minimal context menu for social content specifically. Having nsContextMenu support both seems like a recipe for disaster when people forget about its use in multiple contexts. We do that for the other sidebars, and I think there are a bunch of old bugs on file about stuff that doesn't work right. We should try to do better for social.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 12 years ago

Comment on attachment 695022 [details] [diff] [review]
contextmenu

feedback- given my last comment, but I may be convinced otherwise!

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 6 • 12 years ago

despite the feedback-, I thought I'd finish this out.  This version of the patch has everything working as far as mochitests and what manual testing I've been able to easily do.

	Mark Hammond [:markh] [:mhammond] Reporter
	Comment 7 • 11 years ago

I wonder if we should just support html5 context menus and rely on the provider to offer a "reload" facility if they think it worthwhile for their implementation?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 8 • 11 years ago

(In reply to Mark Hammond (:markh) from comment #7)
> I wonder if we should just support html5 context menus and rely on the
> provider to offer a "reload" facility if they think it worthwhile for their
> implementation?

We should definitely support that, and it would be nice if we could call that sufficient from our side - but I fear that won't fly, and we probably need some kind of better "default" solution on our end. And in any case, IIRC, nsContextMenu is what supports the html5 context menu stuff.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 11 years ago

Comment on attachment 696376 [details] [diff] [review]
contextmenu v1.1

I think Shane sold me on this being the most expedient way forward, though there will probably be a bunch of gotchas that we'll need to keep an eye out for. Hoping Felipe can give a feedback pass on this one, since I won't be able to for a little while.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 10 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8)
> (In reply to Mark Hammond (:markh) from comment #7)
> > I wonder if we should just support html5 context menus and rely on the
> > provider to offer a "reload" facility if they think it worthwhile for their
> > implementation?
> 
> We should definitely support that, and it would be nice if we could call
> that sufficient from our side - but I fear that won't fly, and we probably
> need some kind of better "default" solution on our end. And in any case,
> IIRC, nsContextMenu is what supports the html5 context menu stuff.

html5 menus are part of nsContextMenu, and work for social with this patch.  

There are a number of other menu's that need to be in the social context menu as well, including but not limited to, spell check functionality, security info, reload, etc.  When it came down to it, there were only a couple menu items that I disabled (e.g. history, dev tools) either because they didn't make sense or required larger changes to make work correctly (e.g. inspect element, which does sort of work).  The rest seem applicable to web content in general and should be available in the sidebars.

This patch also should fix several issues with using the context menu for the normal web sidebar, though there are likely a few extra issues there as well.

	:Felipe Gomes (needinfo for replies!)
	Comment 11 • 11 years ago

Comment on attachment 696376 [details] [diff] [review]
contextmenu v1.1

I think reusing the main context menu is fine, but why not an approach of explicitly setting the items needed by social, instead of hiding the ones that are not needed?

This would be less error-prone for new additions to the menu, and add-on friendly. It could be a separate function for this.onSocial, or an attribute added on each item directly on the xul file, like showonsocial="true" for example. I don't know how that fits the HTML5 menus but I believe it's workable. But if there are good reasons for this approach too let me know.

----
comments on the patch:

>     this.showItem("context-back", shouldShow);
>     this.showItem("context-forward", shouldShow);
>     var shouldShowReload = XULBrowserWindow.stopCommand.getAttribute("disabled") == "true";
>-    this.showItem("context-reload", shouldShow && shouldShowReload);
>+    this.showItem("context-reload", (shouldShow || this.onSocial) && shouldShowReload);
>     this.showItem("context-stop", shouldShow && !shouldShowReload);

shouldShowReload reflects the main page reload state, not the social frame AFAICT.


>     this.target = aNode;
> 
>+    var targetBrowser = this.target.ownerDocument.defaultView
>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIWebNavigation)
>+                                .QueryInterface(Ci.nsIDocShell)
>+                                .chromeEventHandler;

what's the deal with this.browser here and elsewhere? Is it not the targetBrowser as expected? I think it would be better to fix it if it makes sense, than to change this.browser -> this.target... elsewhere

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 12 • 11 years ago

(In reply to :Felipe Gomes from comment #11)
> Comment on attachment 696376 [details] [diff] [review]
> contextmenu v1.1
> 
> I think reusing the main context menu is fine, but why not an approach of
> explicitly setting the items needed by social, instead of hiding the ones
> that are not needed?
> 
> This would be less error-prone for new additions to the menu, and add-on
> friendly. It could be a separate function for this.onSocial, or an attribute
> added on each item directly on the xul file, like showonsocial="true" for
> example. I don't know how that fits the HTML5 menus but I believe it's
> workable. But if there are good reasons for this approach too let me know.

I went through each item in the context menu (and just repeated the exercise) and considered whether each should be removed or not.  Since a social sidebar can have any web content that a page would have, I believe most of the menu commands are as pertinent for the sidebar.  The only commands I feel are definitely not are bookmark/navigation.

> ----
> comments on the patch:
> 
> >     this.showItem("context-back", shouldShow);
> >     this.showItem("context-forward", shouldShow);
> >     var shouldShowReload = XULBrowserWindow.stopCommand.getAttribute("disabled") == "true";
> >-    this.showItem("context-reload", shouldShow && shouldShowReload);
> >+    this.showItem("context-reload", (shouldShow || this.onSocial) && shouldShowReload);
> >     this.showItem("context-stop", shouldShow && !shouldShowReload);
> 
> shouldShowReload reflects the main page reload state, not the social frame
> AFAICT.

yeah that can change to shouldShow && (shouldShowReload || this.onSocial).

> 
> >     this.target = aNode;
> > 
> >+    var targetBrowser = this.target.ownerDocument.defaultView
> >+                                .QueryInterface(Ci.nsIInterfaceRequestor)
> >+                                .getInterface(Ci.nsIWebNavigation)
> >+                                .QueryInterface(Ci.nsIDocShell)
> >+                                .chromeEventHandler;
> 
> what's the deal with this.browser here and elsewhere? Is it not the
> targetBrowser as expected? I think it would be better to fix it if it makes
> sense, than to change this.browser -> this.target... elsewhere

this.browser is *usually* gBrowser, not the targetBrowser, although sometimes it is a browser (e.g. in the web panel).  Essentially, this.browser is slightly confused but everything works because gBrowser calls map to the browser element for the currently selected tab.  Using target is more correct, it will always have the right browser without making assumptions.

I could remove this.browser and rely only on the target, that would not be much more effort, but it touches a lot of files (primarily tests where an nsContextMenu instance is created).

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 13 • 11 years ago

This is an alternate patch that achieves the same results, but removes the possible confusion between this.browser and this.target that the last patch could cause.  All existing tests pass.

	:Felipe Gomes (needinfo for replies!)
	Comment 14 • 11 years ago

Comment on attachment 705147 [details] [diff] [review]
alternate patch

Review of attachment 705147 [details] [diff] [review]:
-----------------------------------------------------------------

r+ too with the below change, unless it was not meant to be the final patch (not sure as you only requested feedback).

Nice cleanup on the this.browser part.

::: browser/base/content/nsContextMenu.js
@@ +160,2 @@
>      this.showItem("context-stop", shouldShow && !shouldShowReload);
> +    this.showItem("context-sep-stop", shouldShow || this.onSocial);

This works but is very obscure. shouldShowReload is already a bad name (because it means "should show reload _instead of stop_") and it gets more confusing. Can you rewrite this part to be more straightforward, something like:

let stopped = XULBrowserWindow.stopCommand.getAttribute("disabled") == "true";

let stopReloadItem = "";
if (shouldShow || this.onSocial) {
  stopReloadItem = (stopped || this.onSocial) ? "reload" : "stop";
}

showItem("reload", stopReloadItem == "reload");
showItem("stop", stopReloadItem == "stop");
showItem("sep", !!stopReloadItem);

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 15 • 11 years ago

I decided to do something a little more simple with the navigation items.  That is the only change from the previous patch you r+'d, but since my fix is different I wanted to pass it by you again.

	:Felipe Gomes (needinfo for replies!)
	Comment 16 • 11 years ago

Comment on attachment 706644 [details] [diff] [review]
contextmenu

Review of attachment 706644 [details] [diff] [review]:
-----------------------------------------------------------------

removing review request as we talked on IRC that the handling of stop/reload is still not quite right

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 17 • 11 years ago

this patch implements changes from the feedback in comment #14 where it was given r+, carrying that forward again.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9795d97ccf6

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

Backed out for mochitest-1 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/177ddb6900f4

https://tbpl.mozilla.org/php/getParsedLog.php?id=19174038&tree=Mozilla-Inbound

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 20 • 11 years ago

fixed patch, somehow lost one rather important chunk before and learned I must run ALL mochitests :(

new try at

https://tbpl.mozilla.org/?tree=Try&rev=b7ab4aebd56a

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 21 • 11 years ago

Comment on attachment 706902 [details] [diff] [review]
contextmenu

per irc with felipe, carrying forward r+  try is green again.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 22 • 11 years ago

here we go again :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/274c7e3f6e3f

	Phil Ringnalda (:philor)
	Comment 23 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/274c7e3f6e3f

	u279076
	Comment 25 • 11 years ago

Verified fixed in the latest Firefox 21 build.