Closed Bug 175175 Opened 18 years ago Closed 8 years ago

Add ID to Bookmark Menupopups

Categories

(SeaMonkey :: Bookmarks & History, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(seamonkey2.15 fixed)

RESOLVED FIXED
seamonkey2.15
Tracking Status
seamonkey2.15 --- fixed

People

(Reporter: andyed, Assigned: ewong)

References

Details

Attachments

(1 file, 5 obsolete files)

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$35c1@ripley.netscape.com

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
Focusing bug on bookmark menu per request in bug 161282
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Add ID's to Bookmark and Debug Menupopup 's → Add ID to Bookmark Menupopup 's
Since this is about bookmarks menupopups I am sendining this to bookmarks. I
appologize if this is wrong.
Assignee: asa → ben
Component: Browser-General → Bookmarks
QA Contact: asa → claudius
This will patch the main menu in Mozilla and allow the BookmarksMenu to be
extended with a dynamic overlay.
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
Attachment #148322 - Flags: review?(bugs)
Ben doesn't work on Seamonkey (mozilla suite) stuff any more... might be best to
request review from someone else.
Assignee: bugs → p_ch
QA Contact: claudius → seamonkey.bookmarks
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.
Attachment #148322 - Flags: review?(bugs) → review?(p_ch)
Product: Browser → Seamonkey
default assignee Pierre is no longer active with bookmarks
boris, can you review - pc_h is no longer doing bookmark reviews
Assignee: p_ch → bzbarsky
Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

Pierre not reviewing
Attachment #148322 - Flags: review?(p_ch) → review?
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.
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.
Assignee: bzbarsky → nobody
Summary: Add ID to Bookmark Menupopup 's → Add ID to Bookmark Menupopups
Tim, are you available to update the patch per comment 9?
Attachment #148322 - Flags: review?
(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.
bump
Attachment #148322 - Flags: review?(timeless)
Comment on attachment 148322 [details] [diff] [review]
This patch adds id attributes to the bookmark's menupopup and menuitems

per Comment 9
Attachment #148322 - Flags: review?(timeless) → review-
Assignee: nobody → ewong
Attachment #148322 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #653689 - Flags: review?(mnyromyr)
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.
Attachment #653689 - Flags: review?(mnyromyr) → review-
Attachment #653689 - Attachment is obsolete: true
Attachment #657523 - Flags: review?(mnyromyr)
> -        <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"!
Attachment #657523 - Attachment is obsolete: true
Attachment #657523 - Flags: review?(mnyromyr)
Attachment #657625 - Flags: review?(mnyromyr)
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.
Attachment #657625 - Flags: review?(mnyromyr) → review-
Attachment #657625 - Attachment is obsolete: true
Attachment #658336 - Flags: review?(mnyromyr)
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.
Attachment #658336 - Flags: review?(mnyromyr) → review+
Attachment #658336 - Attachment is obsolete: true
Attachment #659081 - Flags: review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f6cce22f417f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 793582
Settings flags to make tracking of fixed bugs easier.
Target Milestone: --- → seamonkey2.15
You need to log in before you can comment on or make changes to this bug.