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

RESOLVED FIXED in Thunderbird 23.0

Status

Thunderbird
Toolbars and Tabs
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Thomas D. (currently busy elsewhere; needinfo?me), Assigned: Paenglab)

Tracking

17 Branch
Thunderbird 23.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird22 fixed, thunderbird-esr1722+ fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

+++ 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]
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.
(Assignee)

Comment 6

5 years ago
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)
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
(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
(Assignee)

Comment 10

5 years ago
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?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #710333 - Flags: feedback?(mconley)
(Assignee)

Comment 11

5 years ago
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.
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+
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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.
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)
(Assignee)

Comment 17

5 years ago
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?
Attachment #726304 - Attachment is obsolete: true
Attachment #726304 - Flags: review?(mconley)
Attachment #728658 - Flags: review?(mconley)
Flags: needinfo?(richard.marti)

Comment 18

4 years ago
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

4 years ago
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-

Comment 20

4 years ago
(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.
(Assignee)

Comment 21

4 years ago
(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

4 years ago
+++ 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");
   }
(Assignee)

Comment 23

4 years ago
(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?
(Assignee)

Comment 24

4 years ago
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?
Attachment #728658 - Attachment is obsolete: true
Attachment #734415 - Flags: review?(mkmelin+mozilla)

Comment 25

4 years ago
(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

4 years ago
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-
(Assignee)

Comment 27

4 years ago
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.
Attachment #734415 - Attachment is obsolete: true
Attachment #734820 - Flags: review?(mkmelin+mozilla)

Comment 28

4 years ago
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+
(Assignee)

Comment 29

4 years ago
Magnus, do know, does this patch also fix bug 854735?
Keywords: checkin-needed

Comment 30

4 years ago
I think so yes.
https://hg.mozilla.org/comm-central/rev/c4c8dc9e4c8f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
(Assignee)

Comment 32

4 years ago
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+
tracking-thunderbird-esr17: --- → 22+
https://hg.mozilla.org/releases/comm-aurora/rev/1ff503208de0
status-thunderbird22: --- → fixed
Attachment #734820 - Flags: approval-comm-esr17? → approval-comm-esr17+
https://hg.mozilla.org/releases/comm-esr17/rev/9926a6beae39
status-thunderbird-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.