social frames (sidebar, chat window, etc.) have no context menu

VERIFIED FIXED in Firefox 21

Status

()

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: markh, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [1.5])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 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...
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.
Blocks: 805227
Summary: social sidebar has no context menu → social frames (sidebar, chat window, etc.) have no context menu
Duplicate of this bug: 813719
(Assignee)

Updated

6 years ago
Whiteboard: [1.5]
(Assignee)

Comment 3

6 years ago
Created attachment 695022 [details] [diff] [review]
contextmenu

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.
Attachment #695022 - Flags: feedback?(gavin.sharp)
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.
Comment on attachment 695022 [details] [diff] [review]
contextmenu

feedback- given my last comment, but I may be convinced otherwise!
Attachment #695022 - Flags: feedback?(gavin.sharp) → feedback-
(Assignee)

Comment 6

6 years ago
Created attachment 696376 [details] [diff] [review]
contextmenu v1.1

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.
Attachment #695022 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Assignee: nobody → mixedpuppy
(Assignee)

Updated

6 years ago
Blocks: 806008
(Assignee)

Updated

6 years ago
Attachment #696376 - Flags: feedback?(gavin.sharp)
(Reporter)

Comment 7

6 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?
(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.
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.
Attachment #696376 - Flags: feedback?(gavin.sharp) → feedback?(felipc)
(Assignee)

Comment 10

6 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.
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
Attachment #696376 - Flags: feedback?(felipc) → feedback+
(Assignee)

Comment 12

6 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).
(Assignee)

Comment 13

6 years ago
Created attachment 705147 [details] [diff] [review]
alternate patch

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.
Attachment #705147 - Flags: feedback?(felipc)
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);
Attachment #705147 - Flags: review+
Attachment #705147 - Flags: feedback?(felipc)
Attachment #705147 - Flags: feedback+
(Assignee)

Comment 15

6 years ago
Created attachment 706644 [details] [diff] [review]
contextmenu

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.
Attachment #696376 - Attachment is obsolete: true
Attachment #705147 - Attachment is obsolete: true
Attachment #706644 - Flags: review?(felipc)
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
Attachment #706644 - Flags: review?(felipc)
(Assignee)

Comment 17

6 years ago
Created attachment 706821 [details] [diff] [review]
contextmenu

this patch implements changes from the feedback in comment #14 where it was given r+, carrying that forward again.
Attachment #706644 - Attachment is obsolete: true
Attachment #706821 - Flags: review+
(Assignee)

Comment 20

6 years ago
Created attachment 706902 [details] [diff] [review]
contextmenu

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
Attachment #706821 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Comment on attachment 706902 [details] [diff] [review]
contextmenu

per irc with felipe, carrying forward r+  try is green again.
Attachment #706902 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/274c7e3f6e3f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
(Assignee)

Updated

6 years ago
Duplicate of this bug: 806008
Verified fixed in the latest Firefox 21 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.