Closed Bug 814956 Opened 12 years ago Closed 11 years ago

AppMenu button on Mail Toolbar of standalone message window does not work

Categories

(Thunderbird :: Toolbars and Tabs, defect)

17 Branch
defect
Not set
major

Tracking

(thunderbird22 fixed, thunderbird-esr1722+ fixed)

RESOLVED FIXED
Thunderbird 23.0
Tracking Status
thunderbird22 --- fixed
thunderbird-esr17 22+ fixed

People

(Reporter: thomas8, Assigned: Paenglab)

References

Details

Attachments

(2 files, 5 obsolete files)

+++ 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
No longer depends on: 814742
Summary: AppMenu button from standalone message window customization palette does not work → AppMenu button added to Mail Toolbar of standalone message window from customization palette does not work
In fact, it looks as if the AppMenu button is present on the Mail Toolbar of standalone message window *by default*.
Summary: AppMenu button added to Mail Toolbar of standalone message window from customization palette does not work → AppMenu button on Mail Toolbar of standalone message window does not work
Richard, could you comment?
Flags: needinfo?(richard.marti)
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.
(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...
Whiteboard: [STR comment 8]
(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.
Fallen implemented the AppButton on all toolbars in Bug 785692. Maybe he can say why it doesn't work in standalone window.
Flags: needinfo?(richard.marti) → needinfo?(philipp)
(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.
(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.
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.
Severity: normal → major
Blocks: 785692
Attached patch patch (obsolete) — Splinter Review
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?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #710333 - Flags: feedback?(mconley)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #710333 - Attachment is obsolete: true
Attachment #710333 - Flags: feedback?(mconley)
Attachment #710581 - Flags: feedback?(mconley)
Flags: needinfo?(philipp)
Comment on attachment 710581 [details] [diff] [review]
patch v2

I'm fine with this solution.
Attachment #710581 - Flags: feedback?(mconley) → feedback+
But how could the problem with the staying open splitmenus in the standalone window be solved?
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.
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #710581 - Attachment is obsolete: true
Attachment #726304 - Flags: review?(mconley)
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?
Flags: needinfo?(richard.marti)
Attached patch patch v4 (obsolete) — Splinter Review
(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?
Attachment #726304 - Attachment is obsolete: true
Attachment #726304 - Flags: review?(mconley)
Attachment #728658 - Flags: review?(mconley)
Flags: needinfo?(richard.marti)
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 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
Attachment #728658 - Flags: review?(mconley) → review-
(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.
(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?
+++ 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");
   }
(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?
Attached patch patch v5 (obsolete) — Splinter Review
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?
Attachment #728658 - Attachment is obsolete: true
Attachment #734415 - Flags: review?(mkmelin+mozilla)
(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 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.)
Attachment #734415 - Flags: review?(mkmelin+mozilla) → review-
Attached patch patch v6Splinter Review
(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.
Attachment #734415 - Attachment is obsolete: true
Attachment #734820 - Flags: review?(mkmelin+mozilla)
Comment on attachment 734820 [details] [diff] [review]
patch v6

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

Looks good, thx! r=mkmelin
Attachment #734820 - Flags: review?(mkmelin+mozilla) → review+
Magnus, do know, does this patch also fix bug 854735?
Keywords: checkin-needed
I think so yes.
https://hg.mozilla.org/comm-central/rev/c4c8dc9e4c8f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
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.
Attachment #734820 - Flags: approval-comm-esr17?
Attachment #734820 - Flags: approval-comm-beta?
Attachment #734820 - Flags: approval-comm-aurora?
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.
Attachment #734820 - Flags: approval-comm-beta?
Attachment #734820 - Flags: approval-comm-aurora?
Attachment #734820 - Flags: approval-comm-aurora+
Attachment #734820 - Flags: approval-comm-esr17? → approval-comm-esr17+
You need to log in before you can comment on or make changes to this bug.