1.95 KB, patch
|Details | Diff | Splinter Review|
93.59 KB, image/png
75.25 KB, image/png
2.68 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0 Build Identifier: 3.0a2pre (2008061403) and older Thunderbird doesn't have a Go - Folder menu command. This hinders navigating to a folder without a mouse, and is therefore an accessibility issue. The underlying functionality (menu-based folder navigation) is already part of the code, as one can see by selecting "Move To" on the context menu of a message from the message list pane. Such a feature used to be there in SeaMonkey and older Mozilla versions. The Nostalgy plugin doesn't solve the problem, because it doesn't provide hierarchical navigation. Reproducible: Always Steps to Reproduce: Select the "Go" menu". Actual Results: A "Folder" command is missing. Expected Results: A "Folder" submenu should appear. Each element of the "Folder" submenu is the name of a folder, which can again be a submenu, if the folder contains subfolders. This functionlity exists in SeaMonkey. The submenu's entries are already available in the "Move To" and "Copy To" message context menu commands.
SeaMonkey implementation was bug 21344 - xref bug 349767. The SM menu looks like this: ----------------- Next Previous Forward Back ----------------- Mail Start Page ----------------- Work Mail > News & Blogs > news.mozilla.org > ------------------ Hm, we do have a lot of room under the Go menu. I think I like it.
Created attachment 330842 [details] [diff] [review] Patch against version 22.214.171.124 (20080421) that implements the proposed change The implementation could be further improved by removing the intermediate Folder menu thus saving an additional keystroke.
Created attachment 331406 [details] [diff] [review] Patch tested on version 126.96.36.199 (20080708) that implements the proposed change The previously submitted patch had incomplete patches for mailWindowOverlay.xul
Created attachment 490469 [details] [diff] [review] Patch for Thunderbird 3.0.4 This is a patch against the files distributed with Thunderbird 3.0.4. It has been tested and it works correctly.
Created attachment 490473 [details] [diff] [review] Patch for Thunderbird 3.0.9 source code This is an untested hand-crafted patch against the Thunderbird 3.0.9 source code.
Diomidis, thank you very much for the paches. To be accepted in to Thunderbird your patch will need a few things : 1) It needs to be against trunk/tip of the tree (see below) 2) as It touches the UI, you'll need to get ui review on it (providing a screen-shot helps the review in terms of speed) 3) You'll need to get code reviews as well. You'll find the complete set of rules at https://developer.mozilla.org/en/comm-central . Following the docs at https://developer.mozilla.org/En/Developer_Guide/Source_Code/Getting_comm-central should let you create a patch against trunk/tip.
Created attachment 490619 [details] [diff] [review] Patch against trunk 50d164d40d14
Created attachment 490620 [details] User interface: popup menu This shows the selection of a folder by pressing ALT-G-O-L-A-S (Go - Folder - Local Folders - AFolder - Subfolder) on a Windows-7 system.
Created attachment 490624 [details] User interface: after the selection After selecting the subfolder the menu closes and the folder is selected on the folder selection pane.
(In reply to comment #6) > Diomidis, thank you very much for the paches. To be accepted in to Thunderbird > your patch will need a few things : > > 1) It needs to be against trunk/tip of the tree (see below) Done - see comment 7. > 2) as It touches the UI, you'll need to get ui review on it (providing a > screen-shot helps the review in terms of speed) I added two screen shots - see comments 8 and 9. I don't know how to arrange a UI menu. > 3) You'll need to get code reviews as well. When I first added the patches (comments 4 and 5) I sent email to clarkbw asking for a code review.
Comment on attachment 490619 [details] [diff] [review] Patch against trunk 50d164d40d14 Looks pretty good to me
Marco, do you think this would help?
Comment on attachment 490619 [details] [diff] [review] Patch against trunk 50d164d40d14 No need to request two reviews one is enough.
Comment on attachment 490619 [details] [diff] [review] Patch against trunk 50d164d40d14 >+++ b/mail/base/content/mailWindowOverlay.xul Mon Nov 15 10:24:39 2010 +0200 >@@ -1332,16 +1332,30 @@ > <menuseparator id="goNextSeparator"/> >+ <menu id="goFolderMenu" >+ label="&folderMenu.label;" >+ accesskey="&folderMenu.accesskey;" >+ oncommand="gFolderTreeView.selectFolder(event.target._folder, true)"> Please don't use tabs, and line the attributes up with the "id" attribute. >+ <menupopup id="menu_GoFolderPopup" >+ type="folder" >+ showFileHereLabel="true" >+ fileHereLabel="&thisFolder.label;" >+ fileHereAccessKey="&thisFolder.accesskey;" >+ showRecent="true" >+ recentLabel="&contextMoveCopyMsgRecentMenu.label;" >+ recentAccessKey="&contextMoveCopyMsgRecentMenu.accesskey;"/> And these should be lines up with the "id" attribute, too. But other than that, I like it, so r=me with those nits fixed. (As a side note, we would usually need tests for this, but I think the folder-widget tests probably do a pretty good job of testing this functionality. Of course, if you'ld like to learn MozMill, and write some tests for this, I would be more than happy to review them! :) Thanks, Blake.
I think we need a correctly formatted patch before adding the checkin-needed flag, just to be on the safe side. But once there's a patch with the nits fixed, please add checkin-needed.
Created attachment 491849 [details] [diff] [review] Nits fixed following :bwinton code review This modifies the original patch (attachment 490619 [details] [diff] [review]) corrected per :bwinton review.
Comment on attachment 491849 [details] [diff] [review] Nits fixed following :bwinton code review You didn't actually need to re-ask for a review, cause you only fixed the things I asked you to fix, but r=me anyways. :) Thanks, Blake.