175175 - Add ID to Bookmark Menupopups

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Andy Edmonds]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016

Filing for  news://news.netscape.com:119/aona02$[e-mail]

Needed to enable overlays... not sure about the debug menu, but the Bookmark
menu is critical for add-on developers.  

Component reassignment welcome, bug 167391 addresses the GO menu and is assigned
to no component. 

Reproducible: Always

Steps to Reproduce:
1. View code
2. Settle for alternative overlay or hack *.xul directly

Comment 1

[Andy Edmonds]

Focusing bug on bookmark menu per request in bug 161282

Comment 2

[Torben]

Since this is about bookmarks menupopups I am sendining this to bookmarks. I
appologize if this is wrong.

Comment 3

[Boyd Timothy]

Attachment: This patch adds id attributes to the bookmark's menupopup and menuitems

This will patch the main menu in Mozilla and allow the BookmarksMenu to be
extended with a dynamic overlay.

Comment 4

[Boyd Timothy]

Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

Could you please review this patch as a fix to allow extensions to use dynamic
overlays to extend the BookmarksMenu from Mozilla's main menu?	Thanks, Boyd

Comment 5

[Michael Lefevre]

Ben doesn't work on Seamonkey (mozilla suite) stuff any more... might be best to
request review from someone else.

Comment 6

[Michael Lefevre]

Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

ben is highly unlikely to review this. requesting review from default assignee.

Comment 7

[Wayne Mery (:wsmwk)]

default assignee Pierre is no longer active with bookmarks
boris, can you review - pc_h is no longer doing bookmark reviews

Comment 8

[Wayne Mery (:wsmwk)]

Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

Pierre not reviewing

Comment 9

[[e-mail]]

Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

Your menuitem ids aren't consistent, either match the groupmark menuitem or
change the groupmark menuitem to match your ids.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Excuse me?  I have no idea why you decided to assign this bug to me.  Please do
NOT assign bugs to me unless they're regressions caused by a patch I checked in
or unless I explicitly requested that they be assigned to me.

If you want me to review, request review via the review flags.  That said, I
know nothing about bookmarks or the bookmarks UI, and I'm not a module owner or
peer for any of it, so I can't review that patch anyway.  See
http://www.mozilla.org/owners.html for a list of people you can ask for review.

Comment 11

[Wayne Mery (:wsmwk)]

Tim, are you available to update the patch per comment 9?

Comment 12

[Boyd Timothy]

(In reply to comment #11)
> Tim, are you available to update the patch per comment 9?

I will make some time next week to update the patch.

Comment 13

[Wayne Mery (:wsmwk)]

bump

Comment 14

[timeless]

Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

per Comment 9

Comment 15

[Edmund Wong (:ewong)]

Attachment: Add IDs to Bookmark menupopups. (v1)

Comment 16

[Karsten Düsterloh]

Comment on attachment 653689 [details] [diff] [review]
Add IDs to Bookmark menupopups. (v1)

>-            <menuitem id="menuitem_showhide_tabbar"
>+            <menuitem id="menuitem_showHide_tabBar"

This is wrong — there's no point here in changing an existing id.

>+            <menuitem id="menuitem_componentBarCmd"
>+                      label="&componentbarCmd.label;"

I'd prefer the new id's case to match the label's, i.e. menuitem_componentbarCmd here.

>-        <menu id="menu_UseStyleSheet"
>+        <menu id="menu_useStylesheet"

No. :(
Normal diff unfortunately doesn't mark the exact change, only lines — but e.g. kdiff3 does and may help to catch such changes.

Comment 17

[Edmund Wong (:ewong)]

Attachment: Add IDs to Bookmark Menupopups (v2)

Comment 18

[Philip Chee]

> -        <menuitem label="&stopCmd.label;" accesskey="&stopCmd.accesskey;" id="menuitem-stop" disabled="true" oncommand="BrowserStop();" key="key_stop"/>
> +        <menuitem id="menuitem_stopCmd"
> +                  label="&stopCmd.label;"
> +                  accesskey="&stopCmd.accesskey;"
> +                  id="menuitem-stop"
> +                  disabled="true"
> +                  oncommand="BrowserStop();"
> +                  key="key_stop"/>

This item already has an ID of "menuitem-stop"!

Comment 19

[Edmund Wong (:ewong)]

Attachment: Add IDs to Bookmark menu popups. (v3)

Comment 20

[Karsten Düsterloh]

Comment on attachment 657625 [details] [diff] [review]
Add IDs to Bookmark menu popups. (v3)

>-            <menuitem id="menuitem_showhide_tabbar" label="&tabbarCmd.label;" accesskey="&tabbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" oncommand="showHideTabbar();" checked="true"/>
>+            <menuitem id="menuitem_showhide_tabBar"

Sorry, but you are still altering an existing id. :(

>-        <menuitem label="&stopCmd.label;" accesskey="&stopCmd.accesskey;" id="menuitem-stop" disabled="true" oncommand="BrowserStop();" key="key_stop"/>
>+        <menuitem label="&stopCmd.label;"
>+                  accesskey="&stopCmd.accesskey;"
>+                  id="menuitem-stop"
>+                  disabled="true"
>+                  oncommand="BrowserStop();"
>+                  key="key_stop"/>

Please make id the first attribute like everywhere else. 
(Nice spotting, Phil!)

>+        <menuitem id="menuitem_pageSourceCmd"
>+                  accesskey="&pageSourceCmd.accesskey;"
>+                  label="&pageSourceCmd.label;"
>+                  key="key_viewSource"
>+                  command="View:PageSource"/>
>+        <menuitem id="menuitem_pageInfoCmd"
>+                  accesskey="&pageInfoCmd.accesskey;"
>+                  label="&pageInfoCmd.label;"
>+                  key="key_viewInfo"
>+                  command="View:PageInfo"/>

Please move label to second position.

>+        <menu id="menu_iconic_feedsMenu"
>+              class="menu-iconic feedsMenu"
>+              command="feedsMenu"
>+              label="&feedsMenu.label;"
>+              accesskey="&feedsMenu.accesskey;">

Please move label/accesskey to second/third position.

r- for the one unwanted id change.

Comment 21

[Edmund Wong (:ewong)]

Attachment: Add IDs to Bookmark Menupopups (v4)

Comment 22

[Karsten Düsterloh]

Comment on attachment 658336 [details] [diff] [review]
Add IDs to Bookmark Menupopups (v4)

>             <menuitem id="menuitem_showhide_tabbar" label="&tabbarCmd.label;" accesskey="&tabbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" oncommand="showHideTabbar();" checked="true"/>

Might as well rewrap this while you're here (if you feel like).

>+            <menuitem id="menuitem_taskbarCmd"
>+                      label="&taskbarCmd.label;"
>+                      accesskey="&taskbarCmd.accesskey;"
>+                      class="menuitem-iconic"
>+                      type="checkbox"
>+                      observes="cmd_viewtaskbar" />

Please remove the space before /> on checkin.

r=me.

Comment 23

[Edmund Wong (:ewong)]

Attachment: Add IDs to Bookmark Menupopups (v5)

Comment 24

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f6cce22f417f

Comment 25

[Frank Wein [:mcsmurf]]

Settings flags to make tracking of fixed bugs easier.