Last Comment Bug 519041 - Implement context menu for contentTabs
: Implement context menu for contentTabs
Status: RESOLVED FIXED
[has l10n impact]
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 3.0rc1
Assigned To: Mark Banner (:standard8)
:
:
Mentors:
Depends on: 533642
Blocks: 346220
  Show dependency treegraph
 
Reported: 2009-09-26 08:46 PDT by Mark Banner (:standard8)
Modified: 2009-12-15 07:47 PST (History)
3 users (show)
standard8: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1 - Rework and share (29.50 KB, patch)
2009-09-26 11:35 PDT, Mark Banner (:standard8)
clarkbw: ui‑review+
Details | Diff | Splinter Review
[checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste (13.07 KB, patch)
2009-09-28 04:22 PDT, Mark Banner (:standard8)
mkmelin+mozilla: review+
clarkbw: ui‑review+
standard8: approval‑thunderbird3+
Details | Diff | Splinter Review
[checked in] Part 1 - Rework and share v2 (29.21 KB, patch)
2009-09-28 05:44 PDT, Mark Banner (:standard8)
mkmelin+mozilla: review+
standard8: ui‑review+
standard8: approval‑thunderbird3+
Details | Diff | Splinter Review
[checked in] Part 3 - Open in Browser and Open Link in Browser (4.97 KB, patch)
2009-09-29 05:26 PDT, Mark Banner (:standard8)
dmose: review+
clarkbw: ui‑review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2009-09-26 08:46:15 PDT
Content tabs don't currently have a context menu. We've been discussing this and decided it would be good to have at least a minimal menu for Thunderbird 3 (which unfortunately means some more strings).

We can actually obtain a lot of a context menu free by re-using the one we have on the message pane.

There's going to be two parts to this bug 1) Make the existing context menu work across different types of tabs & windows. 2) Add new strings specific to content tabs.

I'm still not quite sure about extensibility of the context menu, but I think we need to get the strings in and possibly think about that a bit more later.
Comment 1 Mark Banner (:standard8) 2009-09-26 11:35:22 PDT
Created attachment 403044 [details] [diff] [review]
Part 1 - Rework and share

This patch doesn't affect strings, but the next one will.

Main changes in this patch:

- Moved the decision making on what to show/not to show wrt message items into nsContextMenu.js
- Moved nsContextMenu.js into messenger.jar from comm.jar; there's no reason for it to be in comm.jar now (and comm.jar contains little, so IMHO we should kill it sometime). Also dropped the preprocessing on the file.
- Removed the old checks for seeing if there's a background image as we don't really use it. I left comments should we want to put it back later.

UX affecting changes:

- "Copy" is now only shown when we actually have some text selected rather than all the time and sometimes being disabled (the old code semi-intended to do this, but overwrote one decision with another).
- "Move to Again" used to be shown disabled for newsgroups even though "Move To" wasn't, "Move to Again" is now not shown for newsgroups (but is in the other places).
- "Archive" is now only shown to the same time as the move option. Before this patch it was being shown virtually everywhere, including when right clicking on audio/video controls.
- Content Tabs will have the possibility for the following options to be displayed depending on what is being right-clicked (this list doesn't show the actual order):

* Save Link As...
* Save Image As...
* Copy
* Copy Link Location
* Copy Image Location
* Select All
* Add to Address Book... (for mailto links)
* Compose Message To (for mailto links)
* Copy Email Address (for mailto links)
* Play / Pause (for video/audio elements)
* Mute / Unmute (for video/audio elements)

I've tested this quite a bit and not found any missing items yet ;-)
Comment 2 Phil Ringnalda (:philor) 2009-09-27 19:59:27 PDT
Comment on attachment 403044 [details] [diff] [review]
Part 1 - Rework and share

Magnus, if you've got bandwidth, I could use help here, since we need to clear this one out to get to his patch with l10n impact.
Comment 3 Bryan Clark (DevTools PM) [@clarkbw] 2009-09-27 22:32:39 PDT
Comment on attachment 403044 [details] [diff] [review]
Part 1 - Rework and share

(In reply to comment #1)
> UX affecting changes:
> 
> - "Copy" is now only shown when we actually have some text selected rather than
> all the time and sometimes being disabled (the old code semi-intended to do
> this, but overwrote one decision with another).
> - "Move to Again" used to be shown disabled for newsgroups even though "Move
> To" wasn't, "Move to Again" is now not shown for newsgroups (but is in the
> other places).
> - "Archive" is now only shown to the same time as the move option. Before this
> patch it was being shown virtually everywhere, including when right clicking on
> audio/video controls.
> - Content Tabs will have the possibility for the following options to be
> displayed depending on what is being right-clicked (this list doesn't show the
> actual order):
> 
> * Save Link As...
> * Save Image As...
> * Copy
> * Copy Link Location
> * Copy Image Location
> * Select All
> * Add to Address Book... (for mailto links)
> * Compose Message To (for mailto links)
> * Copy Email Address (for mailto links)
> * Play / Pause (for video/audio elements)
> * Mute / Unmute (for video/audio elements)

I'm good with this, did a little testing and everything looked normal but better - i can't archive when right clicking on an email address!
Comment 4 Mark Banner (:standard8) 2009-09-28 04:22:12 PDT
Created attachment 403217 [details] [diff] [review]
[checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste 

I lied, there will probably be three patches - the third one will do "Open this page in browser" or something like that.

This patch adds the following items to the context menu:

- Stop, Reload: Shown when right-clicking on a contentTab browser only.
- Undo, Cut, Paste (Copy is already in there): Shown only when right-clicking on a textbox or input element.

Additional notes:

- nsMsgStatusFeedback was wired up to enable/disable the stop command. It now doesn't and we ask it if we're busy if necessary when we update the command handler. This change impacted on the subscribe dialog which now looks after its stop button itself.

- Added in the check for onCanvas in nsContextMenu - this allows us to exactly mirror what FF checks when working out if to display the stop and reload commands.

- The reload enable/disabling in contentTab probably isn't entirely necessary, but I'd like to keep it matching what FF do for now at least.
Comment 5 Mark Banner (:standard8) 2009-09-28 05:44:48 PDT
Created attachment 403227 [details] [diff] [review]
[checked in] Part 1 - Rework and share v2

Minor update to part 1. I realised that I'd broken the global search results list context menu (show as list or click on message from global search).

The logic was incorrectly assuming it was a non-message tab. This is now fixed by comparing tab modes with those in mailTabType.modes.

The context menu in the global search results list still isn't as extensive as it could be - bug 519192 will fix that, I just didn't want to make it worse here ;-)
Comment 6 Magnus Melin 2009-09-28 11:10:28 PDT
Comment on attachment 403227 [details] [diff] [review]
[checked in] Part 1 - Rework and share v2

Looks good, except for the "Move to xxx Again" hiding on news.

It should be like it was, since it shifts to "Copy to xxx Again" when copy was the last action. And copy of course works for news.
Comment 7 Magnus Melin 2009-09-28 11:26:37 PDT
Comment on attachment 403217 [details] [diff] [review]
[checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste 

Looks good. Tested using 
Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("mail:3pane").document.getElementById("tabmail").openTab("contentTab",{contentPage: "http://tinyvid.tv/show/2h9led44g152z"});
Comment 8 Bryan Clark (DevTools PM) [@clarkbw] 2009-09-28 11:36:32 PDT
Comment on attachment 403217 [details] [diff] [review]
[checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste 

nice, works great.  thanks to magnus for the testing snippet.
Comment 9 Mark Banner (:standard8) 2009-09-28 14:22:19 PDT
Comment on attachment 403227 [details] [diff] [review]
[checked in] Part 1 - Rework and share v2

a=me for both of these patches as I know we really want these. Additionally as most of it was copy/pasted then it should be reasonably low risk.
Comment 10 Mark Banner (:standard8) 2009-09-29 01:34:50 PDT
Comment on attachment 403227 [details] [diff] [review]
[checked in] Part 1 - Rework and share v2

Checked in: http://hg.mozilla.org/comm-central/rev/6f68182d5e7f
Comment 11 Mark Banner (:standard8) 2009-09-29 01:35:10 PDT
Comment on attachment 403217 [details] [diff] [review]
[checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste 

Checked in: http://hg.mozilla.org/comm-central/rev/21e93a577fa3
Comment 12 Mark Banner (:standard8) 2009-09-29 05:26:10 PDT
Created attachment 403466 [details] [diff] [review]
[checked in] Part 3 - Open in Browser and Open Link in Browser

Last patch for this bug (which affects strings).

As discussed with davida and dmose on irc, two more items - Open in Browser and Open Link in Browser.

"Open in Browser" is shown when right-clicking on the page and not a link/image/video etc. It is also hidden if the link is an about: or chrome: as those two don't make any sense to try and open in a browser.

"Open Link in Browser" is shown when right-clicking on a link that isn't a mailto link and isn't an about: or chrome: link.

Dan - I've put you as reviewer as you probably know most about the external protocol handler stuff - if you haven't got time, please pass it on.
Comment 13 Dan Mosedale (:dmose) 2009-09-29 11:28:14 PDT
Comment on attachment 403466 [details] [diff] [review]
[checked in] Part 3 - Open in Browser and Open Link in Browser

>diff --git a/mail/base/content/nsContextMenu.js b/mail/base/content/nsContextMenu.js
>--- a/mail/base/content/nsContextMenu.js
>+++ b/mail/base/content/nsContextMenu.js
>@@ -152,8 +152,20 @@ nsContextMenu.prototype = {
>       goUpdateCommand("cmd_reload");
>     }
> 
>+    let loadedProtocol = "";
>+    if (content.document && content.document.location)
>+      loadedProtocol = content.document.location.protocol;
>+
>+    this.showItem("mailContext-openInBrowser", shouldShow && loadedProtocol &&
>+                  loadedProtocol != "about:" && loadedProtocol != "chrome:");

It's not clear from reading this function exactly why shouldShow applies to what it does, both in general, but particularly as it relates to the above context item.  Comments would help, renaming "shouldShow" might also make sense, as might splitting up the second argument.

>+    const mailContextSeparators = [
>+      "mailContext-sep-open-browser", "mailContext-sep-link",
>+      "mailContext-sep-open", "mailContext-sep-open2",
>+      "mailContext-sep-reply", "paneContext-afterMove",
>+      "mailContext-sep-afterMarkMenu", "mailContext-sep-edit",
>+      "mailContext-sep-copy", "mailContext-sep-reportPhishing"
>+    ];
>+
>+    for (let i = 0; i < mailContextSeparators.length; ++i)
>+      this.hideIfAppropriate(mailContextSeparators[i]);
>+

Mmmm, table-driven FTW!  It's unlikely to be a problem here, so no need to worry about this now, but for future reference, Array.forEach is a good habit to get into; see <https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in> for details about why.

r=dmose
Comment 14 Mark Banner (:standard8) 2009-09-29 13:23:59 PDT
Comment on attachment 403466 [details] [diff] [review]
[checked in] Part 3 - Open in Browser and Open Link in Browser

I clarified the variable and also swapped to forEach.

Checked in: http://hg.mozilla.org/comm-central/rev/e722c91a1823
Comment 15 Mark Banner (:standard8) 2009-09-29 13:24:31 PDT
I believe we have all the options we need as a minimum now, so marking this bug as fixed.

Note You need to log in before you can comment on or make changes to this bug.