Last Comment Bug 814956 - AppMenu button on Mail Toolbar of standalone message window does not work
: AppMenu button on Mail Toolbar of standalone message window does not work
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: 17 Branch
: All All
: -- major (vote)
: Thunderbird 23.0
Assigned To: Richard Marti (:Paenglab)
:
Mentors:
Depends on:
Blocks: TB-AppMenu 785692
  Show dependency treegraph
 
Reported: 2012-11-25 06:15 PST by Thomas D. (currently busy elsewhere; needinfo?me)
Modified: 2013-06-19 02:44 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
22+
fixed


Attachments
Testcase1.eml (plain vanilla message which opens in standalone mail window) (119 bytes, message/rfc822)
2012-12-09 06:02 PST, Thomas D. (currently busy elsewhere; needinfo?me)
no flags Details
patch (1.27 KB, patch)
2013-02-05 12:09 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
patch v2 (4.29 KB, patch)
2013-02-06 02:24 PST, Richard Marti (:Paenglab)
mconley: feedback+
Details | Diff | Splinter Review
patch v3 (7.49 KB, patch)
2013-03-18 13:11 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
patch v4 (7.43 KB, patch)
2013-03-23 13:44 PDT, Richard Marti (:Paenglab)
mkmelin+mozilla: review-
Details | Diff | Splinter Review
patch v5 (10.74 KB, patch)
2013-04-07 15:22 PDT, Richard Marti (:Paenglab)
mkmelin+mozilla: review-
Details | Diff | Splinter Review
patch v6 (10.51 KB, patch)
2013-04-08 14:03 PDT, Richard Marti (:Paenglab)
mkmelin+mozilla: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description Thomas D. (currently busy elsewhere; needinfo?me) 2012-11-25 06:15:56 PST
+++ This bug was initially created as a clone of Bug #814742 +++

STR

(Observed on TB17/WinXP)

1 select msg, context menu > Open Message in New Window
2 ensure Mail Toolbar is shown (not sure if that's default or not)
3 from Mail Toolbar context menu > Customize..., drag to add "AppMenu" button to Mail Toolbar
4 click "AppMenu" button

Actual result

- nothing

Expected result 

- open AppMenu
Comment 1 Thomas D. (currently busy elsewhere; needinfo?me) 2012-11-25 06:21:23 PST
In fact, it looks as if the AppMenu button is present on the Mail Toolbar of standalone message window *by default*.
Comment 2 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-09 05:22:22 PST
Richard, could you comment?
Comment 3 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-09 05:30:54 PST
This bug also occurs for all messages opened from .eml files, as they open in a standalone msg window by default. That window is entirely crippled without the menu button, and unless you know all the shortccuts, at least those to bring the main menu bar back, you'll be in trouble even for as simple actions as printing.
Comment 4 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-09 05:59:37 PST
(In reply to Thomas D. from comment #3)
> This bug also occurs for all messages opened from .eml files, as they open
> in a standalone msg window by default. That window is entirely crippled
> without the menu button, and unless you know all the shortccuts, at least
> those to bring the main menu bar back, you'll be in trouble even for as
> simple actions as printing.

Here's a test msg.eml file which TB will open in a standalone window showing the mail toolbar with broken appmenu button.

If you don't see (or hide) the mail toolbar, note how hard it is to get it back, unless you press [alt] to show the traditional main menu...
Comment 5 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-09 06:02:58 PST
Created attachment 690187 [details]
Testcase1.eml (plain vanilla message which opens in standalone mail window)

(In reply to Thomas D. from comment #4)
> (In reply to Thomas D. from comment #3)
> > This bug also occurs for all messages opened from .eml files...
> 
> Here's a test msg.eml file which TB will open in a standalone window showing
> the mail toolbar with broken appmenu button.
Comment 6 Richard Marti (:Paenglab) 2012-12-09 06:21:27 PST
Fallen implemented the AppButton on all toolbars in Bug 785692. Maybe he can say why it doesn't work in standalone window.
Comment 7 Richard Marti (:Paenglab) 2012-12-09 06:27:50 PST
(In reply to Thomas D. from comment #3)
> This bug also occurs for all messages opened from .eml files, as they open
> in a standalone msg window by default. That window is entirely crippled
> without the menu button, and unless you know all the shortccuts, at least
> those to bring the main menu bar back, you'll be in trouble even for as
> simple actions as printing.

That's not true. The menu bar is by default visible on standalone windows, and then everything is accessible. If somebody hides the menu bar he also knows how to show it again.
Comment 8 Richard Marti (:Paenglab) 2012-12-09 06:32:34 PST
(In reply to Thomas D. from comment #4)
> Here's a test msg.eml file which TB will open in a standalone window showing
> the mail toolbar with broken appmenu button.
> 
> If you don't see (or hide) the mail toolbar, note how hard it is to get it
> back, unless you press [alt] to show the traditional main menu...

Please, don't mix bugs. This bug is for the AppButton not working on standalone windows.
Comment 9 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-26 01:45:02 PST
If we are introducing the AppMenu button as a menu replacement (esp. for new users), and then that AppMenu button doesn't work for opening .eml files or opening emails in a new window, I suspect that's a major loss of functionality which should be fixed asap. Feel free to disagree.
Comment 10 Richard Marti (:Paenglab) 2013-02-05 12:09:36 PST
Created attachment 710333 [details] [diff] [review]
patch

The menu works with simply adding  id="mainPopupSet" to the popupset. I'm asking only for feedback because I see two issues:
- Options/Quick Filter Bar is also showing in standalone window. How can I 
  disable it?
- In standalone window bug 784676 is here again. What's going wrong here?
Comment 11 Richard Marti (:Paenglab) 2013-02-06 02:24:16 PST
Created attachment 710581 [details] [diff] [review]
patch v2

Solved the issue with Quick Filter Bar and other separators showing.

Bug 784676 is still here.

Clearing the needinfo request as I found the solution to show the menu.
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2013-03-17 14:45:42 PDT
Comment on attachment 710581 [details] [diff] [review]
patch v2

I'm fine with this solution.
Comment 13 Richard Marti (:Paenglab) 2013-03-17 14:54:36 PDT
But how could the problem with the staying open splitmenus in the standalone window be solved?
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2013-03-17 15:32:40 PDT
Ah, right - I see what's happening. InitAppEditMessagesMenu is failing because gFolderTreeView is null. That's causing initAppMenuPopup to not complete, which causes the menus to fail.
Comment 15 Richard Marti (:Paenglab) 2013-03-18 13:11:59 PDT
Created attachment 726304 [details] [diff] [review]
patch v3

I moved the favorite folder check to the appmenu_FolderViewsPopup onpopupshowing. With this the gFolderTreeView can't be null and the splitmenus are working as in main window.
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2013-03-23 12:00:05 PDT
Comment on attachment 726304 [details] [diff] [review]
patch v3

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

::: mail/base/content/mailWindowOverlay.js
@@ +149,5 @@
>        favoriteAppFolderMenu.hidden = true;
>      }
>    }
> +
> +  for (let i = 0; i < event.target.childNodes.length; i++) {

I think there might be a simpler way to do this - maybe:

let selected = event.target.querySelector("[value=" + gFolderTreeView.mode + "]");
if (selected) {
  selected.setAttribute("checked", "true");
}

Does that work?
Comment 17 Richard Marti (:Paenglab) 2013-03-23 13:44:23 PDT
Created attachment 728658 [details] [diff] [review]
patch v4

(In reply to Mike Conley (:mconley) from comment #16)
> 
> I think there might be a simpler way to do this - maybe:
> 
> let selected = event.target.querySelector("[value=" + gFolderTreeView.mode +
> "]");
> if (selected) {
>   selected.setAttribute("checked", "true");
> }
> 
> Does that work?

This works as always when you propose something. New patch with this change.

Should I also change the old code on line 260 where I copied the code?
Comment 18 Magnus Melin 2013-04-07 11:41:42 PDT
The problem here (In reply to Mike Conley (:mconley) from comment #14)
> Ah, right - I see what's happening. InitAppEditMessagesMenu is failing
> because gFolderTreeView is null. That's causing initAppMenuPopup to not
> complete, which causes the menus to fail.

This is because !favoriteAppFolderMenu.disabled in InitAppEditMessagesMenu doesn't work. We need to set and use the attribute, not the property...

(In reply to Richard Marti [:Paenglab] from comment #15)
> Created attachment 726304 [details] [diff] [review]
> patch v3
> 
> I moved the favorite folder check to the appmenu_FolderViewsPopup
> onpopupshowing. With this the gFolderTreeView can't be null and the
> splitmenus are working as in main window.

That doesn't make gFolderTreeView any less null. It's just that the disabled property is working for that case.
Comment 19 Magnus Melin 2013-04-07 11:42:42 PDT
Comment on attachment 728658 [details] [diff] [review]
patch v4

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

I think we should rather fix the bogus "disabled" code, from my prevous comment
Comment 20 Magnus Melin 2013-04-07 11:50:10 PDT
(In reply to Richard Marti [:Paenglab] from comment #17)
> Should I also change the old code on line 260 where I copied the code?

Sure. Also, looks like you're fixing bug 854735.
Comment 21 Richard Marti (:Paenglab) 2013-04-07 12:24:43 PDT
(In reply to Magnus Melin from comment #18)
> The problem here (In reply to Mike Conley (:mconley) from comment #14)
> > Ah, right - I see what's happening. InitAppEditMessagesMenu is failing
> > because gFolderTreeView is null. That's causing initAppMenuPopup to not
> > complete, which causes the menus to fail.
> 
> This is because !favoriteAppFolderMenu.disabled in InitAppEditMessagesMenu
> doesn't work. We need to set and use the attribute, not the property...

Magnus, I'm a noob in JS. Please can you show me how to do this?
Comment 22 Magnus Melin 2013-04-07 12:27:14 PDT
+++ b/mail/base/content/mailWindowOverlay.js
-  if (!favoriteAppFolderMenu.disabled) {
+  if (!favoriteAppFolderMenu.getAttribute("disabled") == "true") {

and 

+++ b/mail/base/content/messageWindow.js

   if (favoriteFolder) {
-    favoriteFolder.disabled = true;
+    favoriteFolder.setAttribute("disabled", "true");
     favoriteFolder.setAttribute("hidden", "true");
   }
Comment 23 Richard Marti (:Paenglab) 2013-04-07 13:59:31 PDT
(In reply to Magnus Melin from comment #22)
> +++ b/mail/base/content/mailWindowOverlay.js
> -  if (!favoriteAppFolderMenu.disabled) {
> +  if (!favoriteAppFolderMenu.getAttribute("disabled") == "true") {
> 
> and 
> 
> +++ b/mail/base/content/messageWindow.js
> 
>    if (favoriteFolder) {
> -    favoriteFolder.disabled = true;
> +    favoriteFolder.setAttribute("disabled", "true");
>      favoriteFolder.setAttribute("hidden", "true");
>    }

Thank you, but when I'm using this, the menuitem isn't ticked when it's a favorite folder or hidden on a server folder. It looks like the commands in the bracket are never executed. What could be wrong?
Comment 24 Richard Marti (:Paenglab) 2013-04-07 15:22:31 PDT
Created attachment 734415 [details] [diff] [review]
patch v5

Instead of

if (!favoriteAppFolderMenu.getAttribute("disabled") == "true") {

I used 

if (!favoriteAppFolderMenu.getAttribute("disabled")) {

and it works. I haven't seen negative effects. Is this okay like this?
Comment 25 Magnus Melin 2013-04-08 12:07:55 PDT
(In reply to Richard Marti [:Paenglab] from comment #24)
> if (!favoriteAppFolderMenu.getAttribute("disabled")) {
> 
> and it works. I haven't seen negative effects. Is this okay like this?

yeah my version would have needed parenthesis. Anywway, !favoriteAppFolderMenu.hasAttribute("disabled") is somewhat clearer.
Comment 26 Magnus Melin 2013-04-08 12:31:14 PDT
Comment on attachment 734415 [details] [diff] [review]
patch v5

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

::: mail/base/content/mailWindowOverlay.js
@@ +128,5 @@
>    document.commandDispatcher.updateCommands('create-menu-edit');
>  
>    // initialize the favorite Folder checkbox in the appmenu menu
>    let favoriteAppFolderMenu = document.getElementById('appmenu_favoriteFolder');
> +  if (!favoriteAppFolderMenu.getAttribute("disabled")) {

!favoriteAppFolderMenu.hasAttribute("disabled")

@@ +140,5 @@
>        favoriteAppFolderMenu.hidden = true;
>      }
>    }
> +
> +  let selected = event.target.querySelector("[value=" + gFolderTreeView.mode + "]");

this is outside the folder menu check so there's some type errors in the error console. we don't have a gFolderTreeView here, so we need a similar check like for the fav folder (but folder view shouldn't be enabled only when one folder is selected.)
Comment 27 Richard Marti (:Paenglab) 2013-04-08 14:03:00 PDT
Created attachment 734820 [details] [diff] [review]
patch v6

(In reply to Magnus Melin from comment #26)
> Comment on attachment 734415 [details] [diff] [review]
> patch v5
> 
> Review of attachment 734415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/mailWindowOverlay.js
> @@ +128,5 @@
> >    document.commandDispatcher.updateCommands('create-menu-edit');
> >  
> >    // initialize the favorite Folder checkbox in the appmenu menu
> >    let favoriteAppFolderMenu = document.getElementById('appmenu_favoriteFolder');
> > +  if (!favoriteAppFolderMenu.getAttribute("disabled")) {
> 
> !favoriteAppFolderMenu.hasAttribute("disabled")

Done

> @@ +140,5 @@
> >        favoriteAppFolderMenu.hidden = true;
> >      }
> >    }
> > +
> > +  let selected = event.target.querySelector("[value=" + gFolderTreeView.mode + "]");
> 
> this is outside the folder menu check so there's some type errors in the
> error console. we don't have a gFolderTreeView here, so we need a similar
> check like for the fav folder (but folder view shouldn't be enabled only
> when one folder is selected.)

I removed this part and call InitViewFolderViewsMenu(event) at onpopupshowing of appmenu_FolderViewsPopup.
Comment 28 Magnus Melin 2013-04-09 11:34:58 PDT
Comment on attachment 734820 [details] [diff] [review]
patch v6

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

Looks good, thx! r=mkmelin
Comment 29 Richard Marti (:Paenglab) 2013-04-09 11:37:53 PDT
Magnus, do know, does this patch also fix bug 854735?
Comment 30 Magnus Melin 2013-04-09 11:45:50 PDT
I think so yes.
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-04-13 05:12:49 PDT
https://hg.mozilla.org/comm-central/rev/c4c8dc9e4c8f
Comment 32 Richard Marti (:Paenglab) 2013-04-15 13:29:08 PDT
Comment on attachment 734820 [details] [diff] [review]
patch v6

[Approval Request Comment]
It looks as this patch would solve two other bugs: bug 854735 and bug 861662
Also would this patch fix the issue of the non-functional AppButton on standalone windows.

The risk is, it has new JS code but this is more a cleanup/correction of the old code.

I've tried the patch on comm-beta and it applies.
Comment 33 Mark Banner (:standard8) 2013-05-01 02:27:05 PDT
Comment on attachment 734820 [details] [diff] [review]
patch v6

I'd like to get some testing on this before we push it out to esr. So I think we can take this onto aurora this cycle, and then onto ESR during the next cycle.
Comment 34 Mark Banner (:standard8) 2013-05-01 02:44:52 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/1ff503208de0
Comment 35 Mark Banner (:standard8) 2013-06-19 02:44:28 PDT
https://hg.mozilla.org/releases/comm-esr17/rev/9926a6beae39

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