Bug 1546309 - appmenu: follow-up on aceman code review. r=aceman
authorPaul Morris <paul@paulwmorris.com>
Wed, 12 Jun 2019 15:31:57 -0400
changeset 35852 4ad0ea172c057fe9dc0f5489cda7e96eca46e8d9
parent 35851 c8cac88bbe303a3e81fd15311a18a0eeb459f3d0
child 35853 b9426afd27bf876545398412679bc0798a567e83
push id392
push userclokep@gmail.com
push dateMon, 02 Sep 2019 20:17:19 +0000
reviewersaceman
bugs1546309
Bug 1546309 - appmenu: follow-up on aceman code review. r=aceman
mail/base/content/mailWindowOverlay.js
mail/components/customizableui/content/panelUI.inc.xul
mail/test/mozmill/composition/test-reply-multipart-charset.js
mail/test/mozmill/content-tabs/test-addons-mgr.js
mail/test/mozmill/folder-display/test-watch-ignore-thread.js
mail/test/mozmill/folder-pane/test-folder-pane-consumers.js
mail/test/mozmill/folder-tree-modes/test-mode-switching.js
mail/test/mozmill/folder-widget/test-message-filters.js
mail/test/mozmill/shared-modules/test-window-helpers.js
--- a/mail/base/content/mailWindowOverlay.js
+++ b/mail/base/content/mailWindowOverlay.js
@@ -1078,18 +1078,19 @@ function InitRecentlyClosedTabsPopup(
     parent.appendChild(item);
   });
 
   // Only show "Restore All Tabs" if there is more than one tab to restore.
   if (tabs.length > 1) {
     parent.appendChild(document.createXULElement(separatorName));
 
     const item = document.createXULElement(elementName);
-    item.setAttribute("label", document.getElementById("bundle_messenger")
-                                           .getString("restoreAllTabs"));
+    item.setAttribute("label",
+      document.getElementById("bundle_messenger").getString("restoreAllTabs"));
+
     item.setAttribute("oncommand", "goRestoreAllTabs();");
 
     if (classes) {
       item.setAttribute("class", classes);
     }
     parent.appendChild(item);
   }
 
--- a/mail/components/customizableui/content/panelUI.inc.xul
+++ b/mail/components/customizableui/content/panelUI.inc.xul
@@ -693,21 +693,20 @@
                        class="subviewbutton subviewbutton-iconic"
                        label="&getNewMsgCurrentAccountCmdPopupMenu.label;"
                        key="key_getNewMessages"
                        command="cmd_getNewMessages"/>
         <toolbarseparator/> -->
         <menu id="appmenu_getNewMsgFor"
               label="&getNewMsgForCmd.label;"
               oncommand="MsgGetMessagesForAccount();">
-          <menupopup is="folder-menupopup"
-                      mode="getMail"
-                      id="appmenu_getAllNewMsgPopup"
-                      expandFolders="false"
-                      oncommand="MsgGetMessagesForAccount(event.target._folder); event.stopPropagation();">
+          <menupopup is="folder-menupopup" id="appmenu_getAllNewMsgPopup"
+                     mode="getMail"
+                     expandFolders="false"
+                     oncommand="MsgGetMessagesForAccount(event.target._folder); event.stopPropagation();">
             <menuitem id="appmenu_getnewmsgs_all_accounts"
                       label="&getAllNewMsgCmdPopupMenu.label;"
                       key="key_getAllNewMessages"
                       command="cmd_getMsgsForAuthAccounts"/>
             <menuitem id="appmenu_getnewmsgs_current_account"
                       label="&getNewMsgCurrentAccountCmdPopupMenu.label;"
                       key="key_getNewMessages"
                       command="cmd_getNewMessages"/>
--- a/mail/test/mozmill/composition/test-reply-multipart-charset.js
+++ b/mail/test/mozmill/composition/test-reply-multipart-charset.js
@@ -69,20 +69,17 @@ function subtest_replyEditAsNewForward_c
   if (aViewed) {
     // Only if the preview pane is on, we can check the following.
     assert_selected_and_displayed(mc, msg);
   }
 
   if (aOverride) {
     // Display the message using the override charset.
     // Use the app menu which is also available on Mac.
-    mc.click(mc.eid("button-appmenu"));
-    mc.click_appmenu_in_sequence(mc.e("appMenu-mainView"),
-      [ {label: "View"},
-        {label: "Text Encoding"} ],
+    mc.click_through_appmenu([{label: "View"}, {label: "Text Encoding"}],
       {label: aOverride});
   }
 
   let fwdWin;
   switch (aAction) {
   case 1: // Reply.
     fwdWin = open_compose_with_reply();
     break;
--- a/mail/test/mozmill/content-tabs/test-addons-mgr.js
+++ b/mail/test/mozmill/content-tabs/test-addons-mgr.js
@@ -45,19 +45,17 @@ function test_open_addons_with_url() {
 /**
  * Bug 1462923
  * Check if the "Tools->Add-on Options" menu item works and shows our add-on.
  * This relies on the MozMill extension having optionsURL defined in install.rdf,
  * however simplistic the preferences XUL document may be.
  */
 function test_addon_prefs() {
   // Open Add-on Options.
-  mc.click(mc.eid("button-appmenu"));
-  const subview = mc.click_appmenu_in_sequence(mc.e("appMenu-mainView"),
-    [{id: "appmenu_addons"}]);
+  const subview = mc.click_through_appmenu([{id: "appmenu_addons"}]);
 
   plan_for_modal_dialog("mozmill-prefs", function(controller) {
     // Add |mc.sleep(1000);| here to see the popup dialog.
     controller.window.close();
   });
 
   // MozMill add-on should be somewhere in the list. When found, click it.
   let foundAddon = false;
--- a/mail/test/mozmill/folder-display/test-watch-ignore-thread.js
+++ b/mail/test/mozmill/folder-display/test-watch-ignore-thread.js
@@ -31,20 +31,18 @@ function setupModule(module) {
   expand_all_threads();
 }
 
 /**
  * Click one of the menu items in the appmenu View | Messages menu.
  * @param {string} menuId  The id of the menu item to click.
  */
 function clickViewMessagesItem(menuId) {
-  mc.click(mc.eid("button-appmenu"));
-  mc.click_appmenu_in_sequence(mc.e("appMenu-mainView"),
-    [ {id: "appmenu_View"},
-      {id: "appmenu_viewMessagesMenu"} ],
+  mc.click_through_appmenu(
+    [{id: "appmenu_View"}, {id: "appmenu_viewMessagesMenu"}],
     {id: menuId});
 }
 
 /**
  * Test that Ignore Thread works as expected.
  */
 function test_ignore_thread() {
   let t1root = thread1.getMsgHdr(0);
--- a/mail/test/mozmill/folder-pane/test-folder-pane-consumers.js
+++ b/mail/test/mozmill/folder-pane/test-folder-pane-consumers.js
@@ -27,19 +27,17 @@ function setupModule(module) {
 
   let server = setupLocalServer(NNTP_PORT);
   nntpAccount = MailServices.accounts.FindAccountForServer(server);
 }
 
 function test_virtual_folder_selection_tree() {
   plan_for_modal_dialog("mailnews:virtualFolderProperties",
                         subtest_create_virtual_folder);
-  mc.click(mc.eid("button-appmenu"));
-  mc.click_appmenu_in_sequence(mc.e("appMenu-mainView"),
-    [{id: "appmenu_new"}],
+  mc.click_through_appmenu([{id: "appmenu_new"}],
     {id: "appmenu_newVirtualFolder"});
 
   wait_for_modal_dialog("mailnews:virtualFolderProperties");
 }
 
 function subtest_create_virtual_folder(vfc) {
   // Open the folder chooser.
   plan_for_modal_dialog("mailnews:virtualFolderList",
@@ -59,20 +57,17 @@ function subtest_check_virtual_folder_li
   // We should see the folders from the 2 base local accounts here.
   assert_true(tree.view.rowCount > 0, "Folder tree was empty in virtual folder selection!");
   listc.window.document.documentElement.cancelDialog();
 }
 
 function test_offline_sync_folder_selection_tree() {
   plan_for_modal_dialog("mailnews:synchronizeOffline", subtest_offline_sync);
 
-  mc.click(mc.eid("button-appmenu"));
-  mc.click_appmenu_in_sequence(mc.e("appMenu-mainView"),
-    [ {id: "appmenu_File"},
-      {id: "appmenu_offline"} ],
+  mc.click_through_appmenu([{id: "appmenu_File"}, {id: "appmenu_offline"}],
     {id: "appmenu_synchronizeOffline"});
 
   wait_for_modal_dialog("mailnews:synchronizeOffline");
 }
 
 function subtest_offline_sync(osc) {
   // Open the folder chooser.
   plan_for_modal_dialog("mailnews:selectOffline",
--- a/mail/test/mozmill/folder-tree-modes/test-mode-switching.js
+++ b/mail/test/mozmill/folder-tree-modes/test-mode-switching.js
@@ -94,35 +94,30 @@ function assert_mode_selected(aMode) {
   // We need to open the menu because only then the right mode is set in them.
   if (!mc.mozmillModule.isMac) {
     // On OS X the main menu seems not accessible for clicking from mozmill.
     mc.click(view_menu);
     let popuplist = mc.click_menus_in_sequence(view_menupopup, [ { id: modeList_menu.parentNode.id } ], true);
     assert_true(modeList_menu.querySelector('[value="' + baseMode + '"]').hasAttribute("checked"));
     mc.close_popup_sequence(popuplist);
   }
-  mc.click(appmenu_button);
-  mc.click_appmenu_in_sequence(appmenu_mainView,
-    [ {id: "appmenu_View"},
-      {id: "appmenu_FolderViews"} ]);
+  mc.click_through_appmenu([{id: "appmenu_View"}, {id: "appmenu_FolderViews"}]);
+
   assert_true(modeList_appmenu.querySelector('[value="' + baseMode + '"]').hasAttribute("checked"));
   // Close the appmenu by clicking the appmenu button again.
   mc.click(appmenu_button);
 }
 
 /**
  * Toggle the folder mode by clicking in the menu.
  *
  * @param mode  The base name of the mode to select.
  */
 function select_mode_in_menu(mode) {
-  mc.click(appmenu_button);
-  mc.click_appmenu_in_sequence(appmenu_mainView,
-    [ {id: "appmenu_View"},
-      {id: "appmenu_FolderViews"} ],
+  mc.click_through_appmenu([{id: "appmenu_View"}, {id: "appmenu_FolderViews"}],
     {value: mode});
 }
 
 /**
  * Toggle the Compact view option by clicking in the menu.
  */
 function toggle_compact_in_menu() {
   // For some reason, clicking the menuitem does not work by any means,
--- a/mail/test/mozmill/folder-widget/test-message-filters.js
+++ b/mail/test/mozmill/folder-widget/test-message-filters.js
@@ -87,20 +87,18 @@ function test_customize_toolbar_doesnt_d
   be_in_folder(folderA);
 
   /**
    * Get the getAllNewMessages menu and check the number of items.
    */
   function check_getAllNewMsgMenu() {
     wait_for_window_focused(mc.window);
 
-    mc.click(mc.eid("button-appmenu"));
-    const subview = mc.click_appmenu_in_sequence(mc.e("appMenu-mainView"),
-      [ {id: "appmenu_File"},
-        {id: "appmenu_getNewMsgFor"} ]);
+    const subview = mc.click_through_appmenu(
+      [{id: "appmenu_File"}, {id: "appmenu_getNewMsgFor"}]);
 
     assert_equals(subview.children.length, 5,
                   "Incorrect number of items for GetNewMessages before customization");
 
     // TODO appmenu - Now click somewhere that causes the appmenu to close.
     // (Once this test is no longer skipped, see below.)
   }
 
@@ -117,17 +115,17 @@ function test_customize_toolbar_doesnt_d
   plan_for_window_close(customc);
   customc.click(customc.eid("donebutton"));
   wait_for_window_close();
 
   check_getAllNewMsgMenu();
 }
 test_customize_toolbar_doesnt_double_get_mail_menu.EXCLUDED_PLATFORMS = ["darwin"];
 // TODO appmenu - Skipped because it depends on the folder-menupopup code being
-// adapted for use in the appmenu.  Namely the call to click_appmenu_in_sequence
+// adapted for use in the appmenu.  Namely the call to click_through_appmenu
 // won't work because the UI it expects will not be there yet.
 test_customize_toolbar_doesnt_double_get_mail_menu.__force_skip__ = true;
 
 /* A helper function that opens up the new filter dialog (assuming that the
  * main filters dialog is already open), creates a simple filter, and then
  * closes the dialog.
  */
 function create_simple_filter() {
--- a/mail/test/mozmill/shared-modules/test-window-helpers.js
+++ b/mail/test/mozmill/shared-modules/test-window-helpers.js
@@ -1063,38 +1063,37 @@ var AugmentEverybodyWith = {
           this.keypress(new elib.Elem(curPopup), "VK_ESCAPE", {});
         utils.waitFor(() => curPopup.state == "closed",
                       () => ("Popup did not close! id=" + curPopup.id +
                              ", state=" + curPopup.state), 5000, 50);
       }
     },
 
     /**
-     * Click through the appmenu. Uses a recursive style approach with a
-     * sequence of event listeners handling "ViewShown" events. The `navTargets`
-     * parameter specifies items to click to navigate through the menu. The
-     * optional `nonNavTarget` parameter specifies a final item to click to
-     * perform a command after navigating through the menu. If this argument is
-     * omitted, callers can interact with the last view panel that is returned.
-     * Callers will then need to close the appmenu when they are done with it.
+     * Click through the appmenu. Callers are expected to open the initial
+     * appmenu panelview (e.g. by clicking the appmenu buttton). We wait for it
+     * to open if it is not open yet. Then we use a recursive style approach
+     * with a sequence of event listeners handling "ViewShown" events. The
+     * `navTargets` parameter specifies items to click to navigate through the
+     * menu. The optional `nonNavTarget` parameter specifies a final item to
+     * click to perform a command after navigating through the menu. If this
+     * argument is omitted, callers can interact with the last view panel that
+     * is returned. Callers will then need to close the appmenu when they are
+     * done with it.
      *
-     * @param {Element} mainView  The initial appmenu panelview, namely
-     *     <panelview id="appMenu-mainView">. The caller is expected to open it
-     *     (e.g. by clicking the appmenu button). We wait for it to open if it
-     *     isn't open yet.
      * @param {Object[]} navTargets  Array of objects that contain
      *     attribute->value pairs. We pick the menu item whose DOM node matches
      *     all the attribute->value pairs. We click whatever we find. We throw
      *     if the element being asked for is not found.
      * @param {Object} [nonNavTarget]  Contains attribute->value pairs used
      *                                 to identify a final menu item to click.
      * @return {Element}  The <vbox class="panel-subview-body"> element inside
      *                    the last shown <panelview>.
      */
-    click_appmenu_in_sequence(mainView, navTargets, nonNavTarget) {
+    click_appmenu_in_sequence(navTargets, nonNavTarget) {
       const rootPopup = this.e("appMenu-popup");
       const controller = this;
 
       function viewShownListener(navTargets, nonNavTarget, allDone, event) {
         // Set up the next listener if there are more navigation targets.
         if (navTargets.length > 0) {
           rootPopup.addEventListener("ViewShown",
             viewShownListener.bind(null, navTargets.slice(1), nonNavTarget, allDone),
@@ -1136,24 +1135,42 @@ var AugmentEverybodyWith = {
 
       utils.waitFor(() => rootPopup.getAttribute("panelopen") == "true",
         "Waited for the appmenu to open, but it never opened.");
 
       // Because the appmenu button has already been clicked in the calling
       // code (to match click_menus_in_sequence), we have to call the first
       // viewShownListener manually, using a fake event argument, to start the
       // series of event listener calls.
-      const fakeEvent = {target: mainView};
+      const fakeEvent = {target: this.e("appMenu-mainView")};
       viewShownListener(navTargets, nonNavTarget, allDone, fakeEvent);
 
       utils.waitFor(() => done, "Timed out in click_appmenu_in_sequence.");
       return subviewToReturn;
     },
 
     /**
+     * Utility wrapper function that clicks the main appmenu button to open the
+     * appmenu before calling `click_appmenu_in_sequence`. Makes things simple
+     * and concise for the most common case while still allowing for tests that
+     * open the appmenu via keyboard before calling `click_appmenu_in_sequence`.
+     *
+     * @param {Object[]} navTargets  Array of objects that contain
+     *     attribute->value pairs to be used to identify menu items to click.
+     * @param {Object} [nonNavTarget]  Contains attribute->value pairs used
+     *                                 to identify a final menu item to click.
+     * @return {Element}  The <vbox class="panel-subview-body"> element inside
+     *                    the last shown <panelview>.
+     */
+    click_through_appmenu(navTargets, nonNavTarget) {
+      this.click(this.eid("button-appmenu"));
+      return this.click_appmenu_in_sequence(navTargets, nonNavTarget);
+    },
+
+    /**
      * mark_action helper method that produces something that can be concat()ed
      *  onto a list being passed to mark_action in order to describe the focus
      *  state of the window.  For now this will be a variable-length list but
      *  could be changed to a single object in the future.
      */
     describeFocus() {
       let arr = [
         "in window:",