680192 - Port bug 617528 from Firefox and make html5 context menus work in Thunderbird (important for Conversations)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Protzenko [:protz]]

If we are to move towards richer UIs (I'm thinking Conversations here) designed using HTML, a very easy way to handle context menus is the HTML5 contextmenu attribute. Firefox just implemented this, and I'd love to see this work in Thunderbird too: this would allow me to have a better context menu for conversations, possibly one with actions that operate on the whole conversation, one specific to each message, one specific for attachments.

This would also allow me to implement all the annoying options that users scream for, and bury them deep in a context menu, so that regular users, me included, don't have to see them. And that would make the users happy.

Comment 1

[Jim Porter (:squib)]

I know that in content, HTML5 context menus add themselves as a submenu of the main context menu; is this any different in chrome? (I really really hope the answer is "yes".)

Comment 2

[Jim Porter (:squib)]

Let me amend that to say "I think that in content...". Bug 617528 is far too long for me to read through all of it. :)

Comment 3

[Jonathan Protzenko [:protz]]

In a chrome:// html page, the menu items also added themselves into the main context menu, as far as my testing could tell.

Comment 4

[Jan Varga [:janv]]

The implementation of HTML5 context menu is flexible in this case. A menu builder creates XUL menu items and keeps them in a document fragment, this fragment can be attached to any place in native XUL context menu.

Comment 5

[Jim Porter (:squib)]

Taking this. I've got a patch for it, and I want it for Mail Summaries.

Comment 6

[Mounir Lamouri (:mounir)]

I think Jan will be more appropriate than me to comment on this bug: he wrote the HTML5 context menu patches.
For what I can judge, your patch seems ok. I think you can test it with a mochitest-chrome test. You should find some doc about that in MDN if you never wrote one.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #6)
> I think Jan will be more appropriate than me to comment on this bug: he
> wrote the HTML5 context menu patches.
> For what I can judge, your patch seems ok. I think you can test it with a
> mochitest-chrome test. You should find some doc about that in MDN if you
> never wrote one.

Ignore this comment, it was for bug 704428 :(

Comment 8

[Jim Porter (:squib)]

Attachment: Add the appropriate hooks

Here's a patch for this; note that we immediately close the context menu if no items were added to it. Otherwise, right clicking on something with no HTML5 context menu would cause (under Linux, anyway) an invisible context menu that gobbles up the next click event. No tests for this here, since I'll end up testing this in bug 489999 and bug 492158.

If you want to try it out, install the tip version of Mail Summaries[1]. This link should work if you rename it to something.xpi: https://bitbucket.org/squib/mail-summaries/get/dd85e57a399f.zip

[1] https://bitbucket.org/squib/mail-summaries

Comment 9

[Jonathan Protzenko [:protz]]

Ok, so this works. However, I don't think your patch does the right thing.

In Firefox, whenever an html5 element has a contextmenu attribute on it, the elements from the user-provided contextmenu are added to the already existing contextmenu, they're not replacing it as a whole. Your approach basically says: if there's a contextmenu on the element that's being right-clicked, discard the original context menu, just show the entries from the user-provided one.

Think about a page that has a contextmenu attribute on some <div>, and just adds a couple specific actions. I'm pretty sure the author doesn't want to see copy, select all, and the like (copy image url, copy image, save image as, copy email address, add to address book, compose message to, open link in browser, etc. etc.) disabled...

I think the "right" thing to do here is: if the user provides a specific html context menu, disable all selected-msgHdrs-related actions, and append the user-provided actions to the main context menu, so as not to lose all other potentially useful actions.

I'm speaking very much with Conversations in mind here, so my point of view may be biased. David, Mark, do you have any specific opinion on this?

Comment 10

[Jim Porter (:squib)]

Without this patch, there is no context menu on the multi-message or account central frames, period. I'm assuming this is something we want to continue, though maybe it's not so good for Conversations. I don't think the global context menu is particularly useful for the account summary or folder summary, and it's only slightly useful for the multi-message summary. For the account/folder summary in particular, I would prefer not to have "Select All", "Copy", etc, since I don't think they're very useful. I suppose I could be persuaded otherwise, though.

If just Conversations needs the extra items, we could probably do something to allow the SummaryFrameManager to change the XUL context menu for the entire frame.

One other thing I'd be worried about is what the global context menu would do on the old account central page. This won't be a problem when the account summary is finished, but I still haven't made an account summary page for NNTP or RSS.

Comment 11

[Jonathan Protzenko [:protz]]

I'm asking because we'll have to consider as well what we're doing for content tabs that also provide html5 context menus.

Comment 12

[Jim Porter (:squib)]

(In reply to Jonathan Protzenko [:protz] from comment #11)
> I'm asking because we'll have to consider as well what we're doing for
> content tabs that also provide html5 context menus.

That's a good point. This patch should probably support content tabs too. Luckily, this is easy, and we can keep it separate from the context menu for the accountCentralPane and the multimessage.

Comment 13

[Jim Porter (:squib)]

Attachment: Add pagemenu to the global context menu too.

Ok, this patch also supports the HTML5 context menu in content tabs. The easiest way to test this is to set mailnews.start_page.override_url to "https://bug617528.bugzilla.mozilla.org/attachment.cgi?id=554309".

If you think this needs tests, I can probably write some.

Comment 14

[Jonathan Protzenko [:protz]]

Comment on attachment 578783 [details] [diff] [review]
Add pagemenu to the global context menu too.

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

The patch is correct and works fine (as far as I could tell). Is the shift key something you added, or was it designed like this in Firefox?

I'm ok with the patch but I do think it needs a test. There's everything ready in test-content-tab.js:100, including the right-click, and the assertions, so it should be real easy to test. Sorry if I'm insisting a little bit, but I think it can be easily regressed, if the Firefox guys decide to update their code without us noticing.

r=me assuming you add a couple tests

Thanks,

jonathan

Comment 15

[Jim Porter (:squib)]

Try build for tests in progress (I'm not taking any chances this time, since I always seem to cause Mac failures when testing menus): http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=b3d4b90a5847

Comment 16

[Jim Porter (:squib)]

Well, it's a good thing I pushed to try, since I missed an important part of the build config for this. The latest try build passes though, so checked in: http://hg.mozilla.org/comm-central/rev/6e7c802ef298