512375 - make message header "other actions' contain an appropriate default set of actions

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect, P3)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v1

Right now, the "other actions" menu only contains two items.  It was designed, however, to handle more, as per bug 511491 comment 17.  There are a lot of things that could be useful there, but I've started with the easiest three:

* open in new tab (except for in the standalone message window)
* open in new window
* print

I initially tried for mark as read/unread, but that turned out to be hairier than expected do to lack of a notification mechanism (that I could find) for messages being marked in combination with the auto-mark-read timer stuff.  So that'll happen another day.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: screenshot

In the standalone message window, the "open in new tab" option isn't there.  I added the separator because it feels to me like the top three items are about handling the message inside Thunderbird, whereas the bottom two are really about copying the data out of Thunderbird.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Whoops, posted a bogus patch; let's try again.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

After a bit of discussion with philor and andrew about db change listeners being big firehoses to drink from, it occurs to me that I may be able to check the read/unread state lazily, at the time the menu is being popped up.  This patch is a fine start, and there are sufficient other fish to fry, though, that I think I'll leave that for a future patch.

Comment 4

[Richard Marti (:Paenglab)]

Can you also add the 'Add Star' and maybe the 'Tag' menu entry's?

One other point: I have in my theme masked out the #otherActionsTopSpacer with display:none. With this the other actions are directly under the main buttons. This would also leaves space under the 'other actions' for placing the Star icon or/and for the signed icons. The Star icon can be very helpful in a message only view.

Should I file a new Bug for this? Or should I add this comment to Bug 511491?

Comment 5

[Stefan Plewako [:stef]]

How about:
* open in a (only one, combined item) new window|new tab
* separator
* add star
* add tag
* separator
* display attachments
* show as (combined) html|plain text
* separator
* create a filter for this message
* print preview
* save as

View source should be removed.

Comment 6

[Magnus Melin [:mkmelin]]

I think comment 4 and 5 are symptomatic of why i dislike the "other actions" button. Why should we duplicate the context menu (or portions of it) behind this button? It's equally many clicks to get to actions.

Comment 7

[David Ascher (:davida)]

Magnus: context menus aren't that discoverable is all -- lots of people don't know how to find them.  On the mac it's positively hidden, but even on other machines it's surprising how many users don't understand/know them.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 396346 [details] [diff] [review]
patch, v2

Discoverable or not, I guess that depends if you really want to discover. People tend to learn only as much as they need to.

If we're going to keep it, what is the strategy for what is allowed to go to this menu? Many of the suggestions in the earlier comments could easily go in if we keep "view source", which is only useful for techies/debugging. And I don't think we've had a single report about "print" being hard to find, for instance. 

>diff --git a/mail/base/content/mailWindowOverlay.xul b/mail/base/content/mailWindowOverlay.xul
>--- a/mail/base/content/mailWindowOverlay.xul
>+++ b/mail/base/content/mailWindowOverlay.xul
>@@ -532,17 +532,17 @@
>     <menuseparator id="mailContext-sep-open"/>
>     <menuitem id="mailContext-openNewWindow"
>               label="&contextOpenNewWindow.label;"
>               accesskey="&contextOpenNewWindow.accesskey;"
>               oncommand="MsgOpenNewWindowForMessage();"/>
>     <menuitem id="threadPaneContext-openNewTab"
>               label="&contextOpenNewTab.label;"
>               accesskey="&contextOpenNewTab.accesskey;"
>-              oncommand="ThreadTreeContextMenuNewTab(event);"/>
>+              oncommand="OpenMessageInNewTab(event);"/>
>     <menuseparator id="mailContext-sep-open2"/>
>     <menuitem id="mailContext-replySender"
>               label="&contextReplySender.label;"
>               accesskey="&contextReplySender.accesskey;"
>               oncommand="MsgReplySender(event);"/>
>     <menuitem id="mailContext-replyNewsgroup"
>               label="&contextReplyNewsgroup.label;"
>               accesskey="&contextReplyNewsgroup.accesskey;"
>diff --git a/mail/base/content/msgHdrViewOverlay.js b/mail/base/content/msgHdrViewOverlay.js
>--- a/mail/base/content/msgHdrViewOverlay.js
>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -252,16 +252,20 @@ function OnLoadMsgHeaderPane()
>   // two panels), and then the user upgraded to Tb3, which only has one.
>   // Presumably this can also catch cases of extension uninstalls as well.
>   let deckElement = document.getElementById('msgHeaderViewDeck')
>   if (deckElement.selectedIndex < 0 ||
>       deckElement.selectedIndex >= deckElement.childElementCount) {
>     deckElement.selectedIndex = 0;
>   }
> 
>+  // only offer openInTab if this window supports tabs...
>+  let openInTab = document.getElementById("otherActionsOpenInNewTab");
>+  openInTab.hidden = document.getElementById("tabmail") ? false : true;
>+ 

Trailing space ^^

I doubt anyone would want to open in new window either *from* the standalone message window, so it should probably not be shown there either.

>   // dispatch an event letting any listeners know that we have loaded the message pane
>   var event = document.createEvent('Events');
>   event.initEvent('messagepane-loaded', false, true);
>   var headerViewElement = document.getElementById("msgHeaderView");
>   headerViewElement.dispatchEvent(event);
> }
> 
> function OnUnloadMsgHeaderPane()
>diff --git a/mail/base/content/msgHdrViewOverlay.xul b/mail/base/content/msgHdrViewOverlay.xul
>--- a/mail/base/content/msgHdrViewOverlay.xul
>+++ b/mail/base/content/msgHdrViewOverlay.xul
>@@ -183,23 +183,36 @@
> 
>           <!-- perhaps this should be a customizable toolbar too -->
>           <vbox id="otherActionsBox" align="end">
>             <spacer id="otherActionsTopSpacer" flex="1"/>
>             <button type="menu" id="otherActionsButton"
>                     label="&otherActionsButton.label;"
>                     class="msgHeaderView-button msgHeaderView-flat-button">
>               <menupopup id="otherActionsPopup">
>-                <menuitem id="viewSourceMenuItem"
>+                <menuitem id="otherActionsOpenInNewWindow"
>+                          label="&otherActionsOpenInNewWindow.label;"
>+                          accesskey="&otherActionsOpenInNewWindow.accesskey;"
>+                          oncommand="MsgOpenNewWindowForMessage();" />
>+                <menuitem id="otherActionsOpenInNewTab"
>+                          label="&otherActionsOpenInNewTab.label;"
>+                          accesskey="&otherActionsOpenInNewTab.accesskey;"
>+                          oncommand="OpenMessageInNewTab(event);" />
>+                <menuitem id="viewSourceMenuItem;"

No spaces before /> please. 

> <!ENTITY otherActionsButton.label "other actions">
>+<!ENTITY otherActionsOpenInNewWindow.label "open in new window">
>+<!ENTITY otherActionsOpenInNewWindow.accesskey "W">
>+<!ENTITY otherActionsOpenInNewTab.label "open in new tab">
>+<!ENTITY otherActionsOpenInNewTab.accesskey "T">
>+<!ENTITY otherActionsToggleRead.accesskey "m">
> <!ENTITY saveAsMenuItem.label "save asâ¦">
> <!ENTITY saveAsMenuItem.accesskey "S">
> <!ENTITY viewSourceMenuItem.label "view sourceâ¦">
> <!ENTITY viewSourceMenuItem.accesskey "V">
>+<!ENTITY otherActionsPrint.label "print">
>+<!ENTITY otherActionsPrint.accesskey "P">

Accesskeys are case sensitive (performance wise). 

But then again, I also dislike the lower case labels :P

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 396344 [details]
screenshot

looks like a good start.  I'd like to change the view source and save as items (though not sure how yet) as I think they are going to be less used.  ideas?

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #8)
> (From update of attachment 396346 [details] [diff] [review])
> Discoverable or not, I guess that depends if you really want to discover.
> People tend to learn only as much as they need to.
> 
> If we're going to keep it, what is the strategy for what is allowed to go to
> this menu? Many of the suggestions in the earlier comments could easily go in
> if we keep "view source", which is only useful for techies/debugging. And I
> don't think we've had a single report about "print" being hard to find, for
> instance. 

Good point.  My strategy so far has been for the most common and "lightweight" actions.  I think the fact that we have no bug reports on print being hard to find could be symptomatic of people who can't find print can't find bugzilla either; although there are lots of other possible manifestations.

I'm especially uncertain about the view source because I feel like the people who want to view the source of the email can find the context menu.  I wish I could boil down that strategy into something more concise.  "Common context menu items that can be hard to find".  Mark as (un)read being one of those where you have to carefully click the icon in the list or hunt through the menu; Messages -> Mark -> As Read.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

> I doubt anyone would want to open in new window either *from* the standalone
> message window, so it should probably not be shown there either.

One use case I can think of for that is for people who page through messages one after another in the standalone window.  They might want to keep one window for paging, and open a specific message in another window for processing later.  That said, I don't imagine this is a terribly common use case, so removing it seems reasonable.  Duly tweaked.

Remaining issues fixed in new patch iteration.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Asking clarkbw for ui-review again because of removal of "Open in New Window" from standalone message window.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 397206 [details] [diff] [review]
patch, v2

i'll step back from the superreview and only take a look at ui-review later tomorrow ;)

Comment 14

[Günter]

Sorry to go back to the statement for the new revised message header (AFAIR D.Ascher argued it): to bring the message handling function closer to the message and not having them to use via the main menu ... and with a further step to removed those from the main menu toolbar.

Form that we need a button [Other Actions] somehow .. and the pro's for the [Other Actions] are within Davids initial statement.

And I would prefer to have it beside the other buttons. This way it was maintained by Ovidui see  Bug 466025 - explore message header tweaks and variants for tb3 [and polish] see attachment.cgi?id=351876. That button doesn't need a text description, maybe a good icon would be enough here.
Also this would consume vertical space for the first line, the other lines will have the full space!! And icons instead of text and icons will do it even better.

Another point is the scrolling thru the header. The situation with running the buttons out of the header scope can be seen with switching on the Headers/All. Also here Ovidui's solution solves the problem with using a toolbar concept .. that got lost, any other solution/plan for it?

Finally I'm asking to make the TAG service for the message more prominent. Our tweaks discussed on mozillazine.org can be seen here:
http://forums.mozillazine.org/viewtopic.php?f=29&t=1405155&sid=f4958795066e89c8008b5924e93d333d&start=75.
And because we use only buttons for the [Other Actions] and [Tag] vertical space is not a problem.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Günter, I appreciate that you have ideas and thoughts about other ways to improve the message header.  However, we're at the stage of Tb3 where we have to focus on a small set of well-defined bugs in order to ship.  I realize now that I didn't make it clear when I filed this bug that it was intended to be very narrowly scoped to just getting the right set of actions into the existing menu, and I apologize for that.  I'm editing the bug summary now to make that more clear.
 
It's important that this specific set of work have it's own bug so that it doesn't get slowed down by other (possibly valid, but different) concerns.  Please take other concerns to other bugs.  Thanks!

Comment 16

[Günter]

@ --- Comment #15 from Dan Mosedale (:dmose) <[e-mail]>  2009-08-28 08:39:32 PDT ---
> Günter, I appreciate that you have ideas and thoughts about other ways to
> improve the message header.  However, we're at the stage of Tb3 where we have
> to focus on a small set of well-defined bugs in order to ship.  

Sorry Dan, I don't want to stop the train nor delay it .. and maybe I have mixing up things for the new, improved header (yes, I think TB makes the right decision with the redesign).

The bugzilla has the disadvantage not giving a whole picture for specific subject. AFAIK there are some bugs beside this, and they are a bit overlapping!??
Is there a 'common' place to trace the new header concept easily?

Here is what I know:
Bug 512375 - make message header "other actions' contain an appropriate default set of actions .. (this one here)
Bug 511491 - Remove other actions button from message header view and save some vertical space
Bug 499410 -  message header view: get rid of extra vertical whitespace N 
Bug 511625 -  move other actions button in line with header buttons (chevron like)[polish]
Bug 466025 -  explore message header tweaks and variants for tb3 [and polish] 
Bug 471312 -  Cosmetic and font issues with the Shredder message header block 
... and I feel there are more ..

I'm not about to criticize all of that, but -- beside getting an 'optimal' new header, we also need a design we can easily write extensions to achive a more powerful header design (and pave the ground for further enhancements to standard later on). Just to follow your statement with:
Bug 480623 -  move compact message header view to an extension 

Dan, if you think the discussion has to follow on at another place, please let us know.

Thanks for helping 
Günter

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 397206 [details] [diff] [review]
patch, v2

+  // only offer openInTab and openInNewWindow if this window supports tabs...
+  // (i.e. is not a standalone message window), since those actions are likely
+  // to be significantly less common in that case.
+  let opensAreHidden = document.getElementById("tabmail") ? false : true;
+  let openInTab = document.getElementById("otherActionsOpenInNewTab");
+  let openInNewWindow = document.getElementById("otherActionsOpenInNewWindowab");
+  openInTab.hidden = openInNewWindow.hidden = opensAreHidden;
+  

Nit: spaces on empty line, and please capitalize.

Howver, having some problems testing the patch. 

Error: openInNewWindow is null
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 266

and in the native console

Error -> TypeError: GetSelectedMsgFolder() is undefined

... which may be causing the above. Anyway, I see this only with the patch applied - messages open blank in the standalone window with it applied.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3

Doh; sorry about that!  Fixed patch with requested changes...

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #16)

> The bugzilla has the disadvantage not giving a whole picture for specific
> subject. AFAIK there are some bugs beside this, and they are a bit
> overlapping!??
> Is there a 'common' place to trace the new header concept easily?

Have a look at the dependency tree of bug 456814; it links to many of the message header bugs.  That list is maintained by people adding bugs to the "depends on" field by hand, which means that sometimes bugs get missed (feel free to add them, in that case).

Alternately, looking at bugs in the "Message Reader UI" component using Bugzilla (or, alternately, Bugxhibit) is another way to get an overview.

> I'm not about to criticize all of that, but -- beside getting an 'optimal' new
> header, we also need a design we can easily write extensions to achive a more
> powerful header design (and pave the ground for further enhancements to
> standard later on). 

Agreed.  In general, the right thing to do for extensibility problems in the message headers is just to file individual bugs on each one.  The ones that I know about are already covered by existing bugs.

Comment 20

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 397568 [details] [diff] [review]
patch, v3

thanks dan!

only nit is you need an elipsis on the print item.

ui-r+ with that

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 397568 [details] [diff] [review]
patch, v3

Yup, and view source should /not/ have an ellipsis.

Ideally I think there should be a separator after the open actions, but the the menu would also have to be smart and hide unneeded separators in the standalone.

r=mkmelin with the ellipsis swapped

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v4

Turns out I had a longstanding misimpression about exactly when elipses should be used.  Added to print; removed from view source, as requested.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4; second try (checked in)

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Pushed: <http://build.mozillamessaging.com/mercurial/comm-central/rev/b83f71869a25>.

I've cloned this bug into the wanted+ bug 514094 for further iteration, which we'd like, but wouldn't actually block shipping Tb3 on.

Comment 25

[Richard Marti (:Paenglab)]

I've found a typo in your patch: <menuitem id="viewSourceMenuItem;"
The id has a trailing semicolon. For me it is important as I define in my theme icons for the menu entrys.
Would you leave this as it is or should I open for this a new Bug?

Comment 26

[Günter]

Attachment: some misalignments ..

Pl. see attached JPG showing subject and date items are not in line and with "All" headers the whole gets screwed up

Comment 27

[Richard Marti (:Paenglab)]

Are you using TB 3.1a1pre? If yes this is known (Bug 494478)

Comment 28

[Günter]

Yes, it's with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090902 Shredder/3.1a1pre
(Didn't noticed the mentioned bug, sorry)

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #25)
> I've found a typo in your patch: <menuitem id="viewSourceMenuItem;"
> The id has a trailing semicolon. For me it is important as I define in my theme
> icons for the menu entrys.
> Would you leave this as it is or should I open for this a new Bug?

Good eye; thanks for noticing!  Pushed a fix:

http://build.mozillamessaging.com/mercurial/comm-central/rev/81f27df7d581