473829 - Add ids to Firefox menus to make them accessible for MozMill

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Ids for menu items

While starting with the development of MozMill tests I noticed, that a couple of menu items don't have an id. But to run tests against such items it is necessary have have them, especially for locale-wide Mozmill tests. Currently there is only the possibility to access menu items via their name and this don't give us the chance to run tests on other locales than English.

Example:
controller.menus.Bookmarks["Bookmark This Page"].click();

The attached patch adds an id to each of the id-less menu items. Further I've re-factored existing menu items to better apply to our code styles. I haven't changed any of the existing ids. Hopefully all new ids will not break any extension.

If the patch is fine, would it be possible to get it into 1.9.1 too? I know it's late in the cycle but it looks like to be the last chance for Fx3.1. Otherwise MozMill tests which uses such menu items can only be run against en-US.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 357234 [details] [diff] [review]
Ids for menu items

Moving review request to Dao.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 357234 [details] [diff] [review]
Ids for menu items

+                <menuitem id="menu_Undo"
+                <menuitem id="menu_Redo"
+                <menuitem id="menu_Cut"
+                <menuitem id="menu_Copy"
+                <menuitem id="menu_Paste"
+                <menuitem id="menu_Delete"

menu_foo instead of menu_Foo

-                <menuseparator hidden="true" id="textfieldDirection-separator"/>
+                <menuseparator  id="textfieldDirection-separator"
+                                hidden="true"/>

undo this change

+                    <menuitem id="menu_viewBookmarksSidebar"

menu_boomarksSidebar

+                    <menuitem id="menu_viewHistorySidebar"

menu_historySidebar

+                <menuitem id="menu_browserStop"

menu_stop

+                <menuitem id="menu_browserReload"

menu_reload

+                    <menuitem id="menu_fullZoomEnlarge"

menu_zoomEnlage

+                    <menuitem id="menu_fullZoomReduce"

menu_zoomReduce

+                    <menuitem id="menu_fullZoomReset"

menu_zoomReset

-                            <menu label="&charsetMenuMore1.label;" accesskey="&charsetMenuMore1.accesskey;" datasources="rdf:charset-menu" ref="NC:BrowserMore1CharsetMenuRoot">
+                            <menu label="&charsetMenuMore1.label;" accesskey="&charsetMenuMore1.accesskey;"
+                                  datasources="rdf:charset-menu" ref="NC:BrowserMore1CharsetMenuRoot">
+                                <template>
+                                    <rule>
+                                        <menupopup>
+                                        <menuitem uri="..."
+                                                  label="rdf:http://home.netscape.com/NC-rdf#Name"/>
+                                        </menupopup>
+                                    </rule>
[etc.]

Something messes up the patch here, probably too many whitespace changes.

+                    <menuitem id="menu_charsetCustomize"

menu_customizeCharset

-                <menuseparator hidden="true" id="documentDirection-separator"/>
+                <menuseparator  id="documentDirection-separator"
+                                hidden="true" />

undo this change

+                <menuitem id="historyMenuShowAllHistory"

menu_showAllHistory

+      <menuitem id="bookmarkThisPageMenuItem"

menu_bookmarkThisPage

+      <menuitem id="bookmarkAllTabsMenu"

menu_bookmarkAllTabs

+              <menuitem id="menu_webSearch"

menu_search

+              <menuitem id="menu_viewPageInfo"
+                        accesskey="&pageInfoCmd.accesskey;"
+                        label="&pageInfoCmd.label;"
+                        command="View:PageInfo"/>
+              <menuitem id="menu_viewPageInfo"

duplicated menu item

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Ids for menu items v2

Ok, this version fixes all mentioned review comments.

> +                    <menuitem id="menu_viewBookmarksSidebar"
> menu_boomarksSidebar
> 
> +                    <menuitem id="menu_viewHistorySidebar"
> menu_historySidebar

I tried to mimic the current naming convention in that menu. But you are right, we should use more meaningful ids here.

> -                            <menu label="&charsetMenuMore1.label;"
> accesskey="&charsetMenuMore1.accesskey;" datasources="rdf:charset-menu"
> ref="NC:BrowserMore1CharsetMenuRoot">
> +                            <menu label="&charsetMenuMore1.label;"
> accesskey="&charsetMenuMore1.accesskey;"
> +                                  datasources="rdf:charset-menu"
> ref="NC:BrowserMore1CharsetMenuRoot">
> +                                <template>
> +                                    <rule>
> +                                        <menupopup>
> +                                        <menuitem uri="..."
> +                                                 
> label="rdf:http://home.netscape.com/NC-rdf#Name"/>
> +                                        </menupopup>
> +                                    </rule>
> [etc.]
> 
> Something messes up the patch here, probably too many whitespace changes.

Yeah. I've removed that completely to make this patch clutter-less.

> +              <menuitem id="menu_viewPageInfo"
> +                        accesskey="&pageInfoCmd.accesskey;"
> +                        label="&pageInfoCmd.label;"
> +                        command="View:PageInfo"/>
> +              <menuitem id="menu_viewPageInfo"
> 
> duplicated menu item

I completely messed-up with this part. Key is only be used on Linux and OS X but not on Windows:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#461

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 357490 [details] [diff] [review]
Ids for menu items v2

>                 </menu>
>-                <menu id="charsetMenu"
>+               <menu id="charsetMenu"

>                 </menu>
>+
>                 <menuseparator/>

These changes look accidental.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Ids for menu items v3

Yes, happened due to the c&p action. Fixed now.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 357492 [details] [diff] [review]
Ids for menu items v3

>+                  <menupopup id="historyUndoPopup"
>+                              onpopupshowing="HistoryMenu.populateUndoSubmenu();"/>

This is indented too far.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: for checkin [Checkin: Comment 8]

For checkin. Dao, can you check this into mozilla-central?

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4ec2e438987a

Comment 9

[Dão Gottwald [:dao]]

I've changed menu_viewPageInfo to menu_pageInfo.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Patch for 1.9.1

This patch will give us the chance to run MozMill tests against menus, even for localized builds. Only new ids were added for all needed id-less menu items.

Comment 11

[TimoT]

This bit causes chrome errors..

<menuitem id="menu_fullScreen"
          accesskey="&fullScreenCmd.accesskey;"
          label="&fullScreenCmd.label;" key="key_fullScreen"
          id="fullScreenItem"
          type="checkbox"
          command="View:FullScreen"/>

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Indeed. We have the id twice. :( I'll attach a bustage fix.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: bustage fix [Checkin: Comment 14]

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 357518 [details] [diff] [review]
bustage fix
[Checkin: Comment 14]

http://hg.mozilla.org/mozilla-central/rev/0f73f9eec8e5

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Thanks Timo and Gavin. With the bustage fix checked-in it looks good now.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Please don't include the whitespace changes for a branch patch?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Branch patch

This is the branch patch without the wrapping changes.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 357543 [details] [diff] [review]
Branch patch

>                 <menuitem id="goOfflineMenuitem"
>-                          label="&goOfflineCmd.label;" accesskey="&goOfflineCmd.accesskey;" 
>+                          label="&goOfflineCmd.label;" accesskey="&goOfflineCmd.accesskey;"
>                           type="checkbox" oncommand="BrowserOffline.toggleOfflineStatus();"/>
>-                <menuitem id="menu_FileQuitItem" 
>+                <menuitem id="menu_FileQuitItem"
> #ifdef XP_WIN
>         label="&quitApplicationCmdWin.label;"
>         accesskey="&quitApplicationCmdWin.accesskey;"
>@@ -99,34 +99,40 @@
>         command="cmd_quitApplication"/>
>               </menupopup>
>             </menu>
>-  
>+

more unneeded whitespace changes

>-                <menuitem label="&undoCmd.label;"
>+                <menuitem id="menu_undo"
>+                          label="&undoCmd.label;"

here and elsewhere, make this:

>                 <menuitem label="&undoCmd.label;"
>+                          id="menu_undo"

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Branch patch v2

Fixed nits.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 357547 [details] [diff] [review]
Branch patch v2

>-                    <menuitem key="key_fullZoomReduce"  label="&fullZoomReduceCmd.label;"  accesskey="&fullZoomReduceCmd.accesskey;"
>+                    <menuitem id="menu_zoomReduce" key="key_fullZoomReduce"  label="&fullZoomReduceCmd.label;"  accesskey="&fullZoomReduceCmd.accesskey;"
>                               command="cmd_fullZoomReduce"/>

Again, as a means to reduce potential typos:

>                     <menuitem key="key_fullZoomReduce"  label="&fullZoomReduceCmd.label;"  accesskey="&fullZoomReduceCmd.accesskey;"
>+                              id="menu_zoomReduce"
>                               command="cmd_fullZoomReduce"/>

Note that this is just an example. There are many such cases, which makes it slightly harder to review this patch.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Branch patch v3 [Checkin: Comment 29]

I hope that fixes it finally. Will remember for the next patches.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Will remember for the next patches.

Not a big problem in general. I actually welcome whitespace changes where they make it easier to work with the code afterwards. But on branch, we need to aim for the minimal risk.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 357554 [details] [diff] [review]
Branch patch v3
[Checkin: Comment 29]

Now asking again for approval1.9.1. This patch will allow us to run locale-wide MozMill tests against the menu. It would be great to have it for Firefox 3.1.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Mikeal, could you please give me a real example of how I can get it to work for the following example?

Select: "Edit | Undo"

I can do:
controller.click(new elementslib.ID(mailController.window.document, "edit-menu"));
controller.click(new elementslib.ID(mailController.window.document, "menu_undo"));

But how does it work via the menuAPI? Something like the following snippet doesn't work:

controller.menus.edit-menu.menu_undo.click();

I really would like to verify this fix on trunk. Thanks!

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Beltzner, the qa team would be happy if you could find a free minute to check the approval request.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090117 Shiretoko/3.1b3pre Ubiquity/0.1.4 ID:20090117020415 and latest trunk build of MozMill.

You can now mix names and ids when accessing a menu item via the controller interface. The second example is locale independent and will work for each locale: 

controller.menus["Bookmarks"].menu_bookmarkThisPage.click();
controller.menus.bookmarksMenu.menu_bookmarkThisPage.click();

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 357554 [details] [diff] [review]
Branch patch v3
[Checkin: Comment 29]

a191=beltzner

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 357554 [details] [diff] [review]
Branch patch v3
[Checkin: Comment 29]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1e5de8f74405

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090204020327

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Asking for wanted1.9.0.x because it would give us the opportunity to run Mozmill scripts also in 3.0.x builds. Without having the id's we can only run a small number of tests because our tests make heavy usage of menu clicks.

The most important test we cannot run in Firefox 3.0.x is the software update. Without the access to the software update dialog we cannot perform this test. It will be a big improvement for QA if this could land on 1.9.0 even it's not a security or stability related patch.

But one question remains for me. When we only add new id's how big is the chance to break existing extensions? Would this be a stopper for 1.9.0?

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to comment #31)
> The most important test we cannot run in Firefox 3.0.x is the software update.

My fault. On 3.0.x the software update menu item has an id. So those updates are not affected.