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)
Tracking
(thunderbird22 fixed, thunderbird-esr1722+ fixed)
RESOLVED
FIXED
Thunderbird 23.0
People
(Reporter: thomas8, Assigned: Paenglab)
References
Details
Attachments
(2 files, 5 obsolete files)
119 bytes,
message/rfc822
|
Details | |
10.51 KB,
patch
|
mkmelin
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-esr17+
|
Details | Diff | Splinter Review |
+++ 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
Reporter | ||
Updated•12 years ago
|
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
Reporter | ||
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 3•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 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. 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]
Reporter | ||
Comment 5•11 years ago
|
||
(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•11 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•11 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•11 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.
Reporter | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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 12•11 years ago
|
||
Comment on attachment 710581 [details] [diff] [review] patch v2 I'm fine with this solution.
Attachment #710581 -
Flags: feedback?(mconley) → feedback+
Assignee | ||
Comment 13•11 years ago
|
||
But how could the problem with the staying open splitmenus in the standalone window be solved?
Comment 14•11 years ago
|
||
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•11 years ago
|
||
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 16•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(richard.marti)
Assignee | ||
Comment 17•11 years ago
|
||
(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•11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
||
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•11 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•11 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•11 years ago
|
||
(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•11 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•11 years ago
|
||
Magnus, do know, does this patch also fix bug 854735?
Keywords: checkin-needed
Comment 30•11 years ago
|
||
I think so yes.
Comment 31•11 years ago
|
||
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
Assignee | ||
Comment 32•11 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 33•11 years ago
|
||
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+
Updated•11 years ago
|
tracking-thunderbird-esr17:
--- → 22+
Comment 34•11 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/1ff503208de0
status-thunderbird22:
--- → fixed
Updated•11 years ago
|
Attachment #734820 -
Flags: approval-comm-esr17? → approval-comm-esr17+
Comment 35•11 years ago
|
||
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.
Description
•