Closed Bug 473829 Opened 11 years ago Closed 11 years ago

Add ids to Firefox menus to make them accessible for MozMill

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: verified1.9.1, Whiteboard: [fixed1.9.1b3])

Attachments

(3 files, 6 obsolete files)

Attached patch Ids for menu items (obsolete) — Splinter Review
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.
Attachment #357234 - Flags: review?(gavin.sharp)
Flags: wanted-firefox3.1?
Comment on attachment 357234 [details] [diff] [review]
Ids for menu items

Moving review request to Dao.
Attachment #357234 - Flags: review?(gavin.sharp) → review?(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
Attachment #357234 - Flags: review?(dao) → review-
Summary: Add id's to Firefox menus to make them accessible for MozMill → Add ids to Firefox menus to make them accessible for MozMill
Attached patch Ids for menu items v2 (obsolete) — Splinter Review
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
Attachment #357234 - Attachment is obsolete: true
Attachment #357490 - Flags: review?(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.
Attached patch Ids for menu items v3 (obsolete) — Splinter Review
Yes, happened due to the c&p action. Fixed now.
Attachment #357490 - Attachment is obsolete: true
Attachment #357492 - Flags: review?(dao)
Attachment #357490 - Flags: review?(dao)
Comment on attachment 357492 [details] [diff] [review]
Ids for menu items v3

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

This is indented too far.
Attachment #357492 - Flags: review?(dao) → review+
For checkin. Dao, can you check this into mozilla-central?
Attachment #357492 - Attachment is obsolete: true
Attachment #357494 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4ec2e438987a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
I've changed menu_viewPageInfo to menu_pageInfo.
Attached patch Patch for 1.9.1 (obsolete) — Splinter Review
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.
Attachment #357515 - Flags: review+
Attachment #357515 - Flags: approval1.9.1?
Attachment #357494 - Attachment filename: bookmarkmenu.patch → [checked-in] bookmarkmenu.patch
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"/>
Indeed. We have the id twice. :( I'll attach a bustage fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #357515 - Attachment is obsolete: true
Attachment #357518 - Flags: review?(dao)
Attachment #357515 - Flags: approval1.9.1?
Attachment #357515 - Flags: review+
Thanks Timo and Gavin. With the bustage fix checked-in it looks good now.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Please don't include the whitespace changes for a branch patch?
Attached patch Branch patch (obsolete) — Splinter Review
This is the branch patch without the wrapping changes.
Attachment #357543 - Flags: review?(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"
Attached patch Branch patch v2 (obsolete) — Splinter Review
Fixed nits.
Attachment #357543 - Attachment is obsolete: true
Attachment #357547 - Flags: review?(dao)
Attachment #357543 - Flags: review?(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.
I hope that fixes it finally. Will remember for the next patches.
Attachment #357547 - Attachment is obsolete: true
Attachment #357554 - Flags: review?(dao)
Attachment #357547 - Flags: review?(dao)
Attachment #357554 - Flags: review?(dao) → review+
(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 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.
Attachment #357554 - Flags: approval1.9.1?
Duplicate of this bug: 382473
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!
Beltzner, the qa team would be happy if you could find a free minute to check the approval request.
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();
Status: RESOLVED → VERIFIED
Comment on attachment 357554 [details] [diff] [review]
Branch patch v3
[Checkin: Comment 29]

a191=beltzner
Attachment #357554 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 landing]
Keywords: checkin-needed
Attachment #357494 - Attachment description: for checkin → for checkin [Checkin: Comment 8]
Attachment #357494 - Attachment filename: [checked-in] bookmarkmenu.patch → bookmarkmenu.patch
Attachment #357518 - Attachment description: bustage fix → bustage fix [Checkin: Comment 14]
Comment on attachment 357554 [details] [diff] [review]
Branch patch v3
[Checkin: Comment 29]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1e5de8f74405
Attachment #357554 - Attachment description: Branch patch v3 → Branch patch v3 [Checkin: Comment 29]
Whiteboard: [needs 191 landing] → [fixed1.9.1b3]
Version: 3.1 Branch → Trunk
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
Flags: wanted-firefox3.1?
Blocks: 484532
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?
Flags: wanted1.9.0.x?
(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.
You need to log in before you can comment on or make changes to this bug.