The default bug view has changed. See this FAQ.

A menu option for Bookmark All Tabs and bookmark options in the right-click menu for tabs

RESOLVED FIXED in Firefox1.5

Status

()

Firefox
Bookmarks & History
--
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: gsshih, Assigned: gsshih)

Tracking

(Depends on: 1 bug)

unspecified
Firefox1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

12 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050701 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050701 Firefox/1.0+

Under Bookmarks in the menu bar, a separate option for "Bookmark All Tabs..."
should be included.  This gets rid of the need for the checkbox "Bookmark all
tabs in folder".  Also, it would be convenient to have "Bookmark this tab" and
"Bookmark all tabs" options when right-clicking on a tab.

Reproducible: Always
(Assignee)

Comment 1

12 years ago
Created attachment 188991 [details] [diff] [review]
patch for bookmark all tabs
(Assignee)

Comment 2

12 years ago
Comment on attachment 188991 [details] [diff] [review]
patch for bookmark all tabs

>Index: base/content/browser-menubar.inc
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-menubar.inc,v
>retrieving revision 1.53
>diff -u -u -p -r1.53 browser-menubar.inc
>--- base/content/browser-menubar.inc	23 Jun 2005 02:25:04 -0000	1.53
>+++ base/content/browser-menubar.inc	11 Jul 2005 23:33:16 -0000
>@@ -371,6 +371,11 @@
>                           label="&manBookmarksCmd.label;"
>                           accesskey="&manBookmarksCmd.accesskey;"    
>                           oncommand="toOpenWindowByType('bookmarks:manager', 'chrome://browser/content/bookmarks/bookmarksManager.xul');"/>
>+                <menuitem id="bookmarkAllCmd"
>+                          key="addBookmarkGroupKb"
>+                          label="&bookmarkAllCmd.label;"
>+                          accesskey="&bookmarkAllCmd.accesskey;"
>+                          command="Browser:AddBookmarkGroup"/>
>                 <menuseparator/>
>               </menupopup>
>             </menu>
>Index: base/content/browser-sets.inc
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-sets.inc,v
>retrieving revision 1.49
>diff -u -u -p -r1.49 browser-sets.inc
>--- base/content/browser-sets.inc	20 Jun 2005 06:39:22 -0000	1.49
>+++ base/content/browser-sets.inc	11 Jul 2005 23:33:16 -0000
>@@ -149,6 +149,7 @@
>     <command id="cmd_findAgain" oncommand="onFindAgainCmd();"/>
>     <command id="cmd_findPrevious" oncommand="onFindPreviousCmd();"/>    
>     <command id="Browser:AddBookmarkAs" oncommand="addBookmarkAs(document.getElementById('content'));"/>
>+    <command id="Browser:AddBookmarkGroup" oncommand="addBookmarkGroup(document.getElementById('content'));"/>
>     <command id="Browser:Home"    oncommand="BrowserHome();"/>
>     <command id="Browser:Back"    oncommand="BrowserBack();" disabled="true"/>
>     <command id="Browser:Forward" oncommand="BrowserForward();" disabled="true"/>
>Index: base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.453
>diff -u -u -p -r1.453 browser.js
>--- base/content/browser.js	7 Jul 2005 19:30:09 -0000	1.453
>+++ base/content/browser.js	11 Jul 2005 23:33:16 -0000
>@@ -183,6 +183,17 @@ function UpdateBackForwardButtons()
>   }
> }
> 
>+function UpdateBookmarkAllTabsMenuitem()
>+{
>+  var browser = getBrowser();
>+  var numtabs = browser.tabContainer.childNodes.length;
>+  var bookmarkAllCommand = document.getElementById("Browser:AddBookmarkGroup");
>+  if (numtabs > 1)
>+    bookmarkAllCommand.setAttribute("disabled", "false");
>+  else
>+    bookmarkAllCommand.setAttribute("disabled", "true");
>+}
>+
> const gSessionHistoryObserver = {
>   observe: function(subject, topic, data)
>   {
>@@ -1524,6 +1535,32 @@ function addBookmarkForTabBrowser(aTabBr
>   var currentTabInfo = { name: "", url: "", charset: null };
> 
>   const activeBrowser = aTabBrowser.selectedBrowser;
>+
>+  var webNav = activeBrowser.webNavigation;
>+  var url = webNav.currentURI.spec;
>+  var name = "";
>+  var charSet, description;
>+  try {
>+    var doc = webNav.document;
>+    name = doc.title || url;
>+    charSet = doc.characterSet;
>+    description = BookmarksUtils.getDescriptionFromDocument(doc);
>+  } catch (e) {
>+    name = url;
>+  }
>+  
>+  currentTabInfo = {name: name, url: url, charset: charSet, description: description, bAddGroup: false};
>+  var dialogArgs = currentTabInfo;
>+  dialogArgs.objGroup = tabsInfo;
>+  openDialog("chrome://browser/content/bookmarks/addBookmark2.xul", "",
>+             BROWSER_ADD_BM_FEATURES, dialogArgs);
>+}
>+
>+function addBookmarkGroup(aTabBrowser, aSelect)
>+{
>+  var tabsInfo = [];
>+  var currentTabInfo = { name: "", url: "", charset: null };
>+  const activeBrowser = aTabBrowser.selectedBrowser;
>   const browsers = aTabBrowser.browsers;
>   for (var i = 0; i < browsers.length; ++i) {
>     var webNav = browsers[i].webNavigation;
>@@ -1538,12 +1575,14 @@ function addBookmarkForTabBrowser(aTabBr
>     } catch (e) {
>       name = url;
>     }
>-    tabsInfo[i] = { name: name, url: url, charset: charSet, description: description, bAddGroup: true };
>+    tabsInfo[i] = { name: name, url: url, charset: charSet, description: description};
>     if (browsers[i] == activeBrowser)
>       currentTabInfo = tabsInfo[i];
>   }
>+  currentTabInfo = {name: "[Folder name]"};
>   var dialogArgs = currentTabInfo;
>   dialogArgs.objGroup = tabsInfo;
>+  dialogArgs.bAddBookmarkGroup = true;
>   openDialog("chrome://browser/content/bookmarks/addBookmark2.xul", "",
>              BROWSER_ADD_BM_FEATURES, dialogArgs);
> }
>@@ -3112,7 +3151,7 @@ nsBrowserStatusHandler.prototype =
>     this.statusTextField        = document.getElementById("statusbar-display");
>     this.securityButton         = document.getElementById("security-button");
>     this.urlBar                 = document.getElementById("urlbar");
>-
>+  
>     // Initialize the security button's state and tooltip text
>     var securityUI = getBrowser().securityUI;
>     this.onSecurityChange(null, null, securityUI.state);
>@@ -3130,7 +3169,7 @@ nsBrowserStatusHandler.prototype =
>     this.securityButton         = null;
>     this.urlBar                 = null;
>     this.statusText             = null;
>-    this.lastURI                = null;
>+    this.lastURI                = null; 
>   },
> 
>   setJSStatus : function(status)
>@@ -3228,7 +3267,7 @@ nsBrowserStatusHandler.prototype =
>         if (aWebProgress.DOMWindow == content) {
>           if (aRequest)
>             this.endDocumentLoad(aRequest, aStatus);
>-          var browser = gBrowser.mCurrentBrowser;
>+          var browser = gBrowser.mCurrentBrowser;         
>           if (!gBrowser.mTabbedMode && !browser.mIconURL)
>             gBrowser.useDefaultIcon(gBrowser.mCurrentTab);
>           if (browser.mIconURL)
>@@ -3388,7 +3427,7 @@ nsBrowserStatusHandler.prototype =
>       }
>     }
>     UpdateBackForwardButtons();
>-
>+    UpdateBookmarkAllTabsMenuitem();
>     if (findField && gFindMode != FIND_NORMAL) {
>       // Close the Find toolbar if we're in old-style TAF mode
>       closeFindBar();
>Index: components/bookmarks/content/addBookmark2.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/bookmarks/content/addBookmark2.js,v
>retrieving revision 1.34
>diff -u -u -p -r1.34 addBookmark2.js
>--- components/bookmarks/content/addBookmark2.js	19 Apr 2005 15:17:04 -0000	1.34
>+++ components/bookmarks/content/addBookmark2.js	11 Jul 2005 23:33:16 -0000
>@@ -52,36 +52,38 @@
>  *   Apart from the standard openDialog parameters, this dialog can 
>  *   be passed additional information, which is contained in the 
>  *   wArg object:
>- * 
>- *   wArg.name          : Bookmark Name. The value to be prefilled
>- *                        into the "Name: " field (if visible).
>- *   wArg.description   : Bookmark description. The value to be added
>- *                      : to the boomarks description field.
>- *   wArg.url           : Bookmark URL: The location of the bookmark.
>- *                        The value to be filled in the "Location: "
>- *                        field (if visible).
>- *   wArg.folderURI     : Bookmark Folder. The RDF Resource URI of the
>- *                        folder that this bookmark should be created in.
>- *   wArg.charset       : Bookmark Charset. The charset that should be
>- *                        used when adding a bookmark to the specified
>- *                        URL. (Usually the charset of the current 
>- *                        document when launching this window).
>- *   wArg.bAddGroup     : True if adding a group of tabs, false
>- *                        otherwise.
>- *   wArg.objGroup[]    : If adding a group of tabs, this is an array
>- *                        of wArg objects with name, URL and charset
>- *                        properties, one for each group member.
>- *   wArg.bWebPanel     : If the bookmark should become a web panel.
>- *   wArg.keyword       : A suggested keyword for the bookmark. If this
>- *                        argument is supplied, the keyword row is made
>- *                        visible.
>- *   wArg.bNeedKeyword  : Whether or not a keyword is required to add
>- *                        the bookmark.
>- *   wArg.postData      : PostData to be saved with this bookmark, 
>- *                        in the format a string of name=value pairs
>- *                        separated by CRLFs.
>- *   wArg.feedURL       : feed URL for Livemarks (turns bookmark
>- *                        into Livemark)
>+ *  
>+ *   wArg.name              : Bookmark Name. The value to be prefilled
>+ *                            into the "Name: " field (if visible).
>+ *   wArg.description       : Bookmark description. The value to be added
>+ *                          : to the boomarks description field.
>+ *   wArg.url               : Bookmark URL: The location of the bookmark.
>+ *                            The value to be filled in the "Location: "
>+ *                            field (if visible).
>+ *   wArg.folderURI         : Bookmark Folder. The RDF Resource URI of the
>+ *                            folder that this bookmark should be created in.
>+ *   wArg.charset           : Bookmark Charset. The charset that should be
>+ *                            used when adding a bookmark to the specified
>+ *                            URL. (Usually the charset of the current 
>+ *                            document when launching this window).
>+ *   wArg.bAddGroup         : True if adding a group of tabs, false
>+ *                            otherwise.
>+ *   wArg.bAddBookmarkGroup : True if "Bookmark All Tabs" option is chosen,
>+ *                            false otherwise.
>+ *   wArg.objGroup[]        : If adding a group of tabs, this is an array
>+ *                            of wArg objects with name, URL and charset
>+ *                            properties, one for each group member.
>+ *   wArg.bWebPanel         : If the bookmark should become a web panel.
>+ *   wArg.keyword           : A suggested keyword for the bookmark. If this
>+ *                            argument is supplied, the keyword row is made
>+ *                            visible.
>+ *   wArg.bNeedKeyword      : Whether or not a keyword is required to add
>+ *                            the bookmark.
>+ *   wArg.postData          : PostData to be saved with this bookmark, 
>+ *                            in the format a string of name=value pairs
>+ *                            separated by CRLFs.
>+ *   wArg.feedURL           : feed URL for Livemarks (turns bookmark
>+ *                            into Livemark)
>  */
> 
> var gSelectedFolder;
>@@ -119,6 +121,7 @@ function Startup()
>   gName.focus();
>   gSuggestedKeyword = gArg.keyword;
>   gKeywordRequired = gArg.bNeedKeyword;
>+
>   if (!gSuggestedKeyword && !gKeywordRequired) {
>     gKeywordRow.hidden = true;
>   } else {
>@@ -127,6 +130,7 @@ function Startup()
>     if (gKeywordRequired)
>       gRequiredFields.push(gKeyword);
>   }
>+
>   if (!gArg.bAddGroup)
>     gGroup.setAttribute("hidden", "true");
>   sizeToContent();
>@@ -173,6 +177,9 @@ function onOK()
> 
>   var url, rSource;
>   var livemarkFeed = gArg.feedURL;
>+  if (gArg.bAddBookmarkGroup){
>+    gGroup.checked = true;
>+  }
>   if (gGroup && gGroup.checked) {
>     rSource = BMDS.createFolder(gName.value);
>     const groups = gArg.objGroup;
>Index: components/bookmarks/content/addBookmark2.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/bookmarks/content/addBookmark2.xul,v
>retrieving revision 1.19
>diff -u -u -p -r1.19 addBookmark2.xul
>--- components/bookmarks/content/addBookmark2.xul	2 Mar 2005 10:45:51 -0000	1.19
>+++ components/bookmarks/content/addBookmark2.xul	11 Jul 2005 23:33:16 -0000
>@@ -55,7 +55,7 @@
>         xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>         ondialogextra2="newFolder()"
>         ondialogaccept="return onOK(event)"
>-        buttons="accept,cancel" 
>+        buttons="accept,cancel"
>         buttonlabelextra2="&button.newfolder.label;" buttonaccesskeyextra2="&button.newfolder.accesskey;"
> #ifdef XP_UNIX
>         buttonlabelaccept="&acceptButton.label;"
>Index: locales/en-US/chrome/browser/browser.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v
>retrieving revision 1.20
>diff -u -u -p -r1.20 browser.dtd
>--- locales/en-US/chrome/browser/browser.dtd	23 Jun 2005 02:25:06 -0000	1.20
>+++ locales/en-US/chrome/browser/browser.dtd	11 Jul 2005 23:33:19 -0000
>@@ -63,6 +63,8 @@
> <!ENTITY addCurPageAsCmd.commandkey "d">
> <!ENTITY manBookmarksCmd.label "Manage Bookmarks...">
> <!ENTITY manBookmarksCmd.accesskey "M">
>+<!ENTITY bookmarkAllCmd.label "Bookmark All Tabs...">
>+<!ENTITY bookmarkAllCmd.accesskey "T">
> 
> <!ENTITY backCmd.label                "Back">
> <!ENTITY backCmd.accesskey            "B">
>Index: content/widgets/tabbrowser.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v
>retrieving revision 1.91
>diff -u -u -p -r1.91 tabbrowser.xml
>--- content/widgets/tabbrowser.xml	7 Jul 2005 15:52:49 -0000	1.91
>+++ content/widgets/tabbrowser.xml	11 Jul 2005 23:35:04 -0000
>@@ -82,6 +82,12 @@
>                           oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
>                                      tabbrowser.removeAllTabsBut(tabbrowser.mContextTab);"/>
>             <xul:menuseparator/>
>+            <xul:menuitem label="&addCurTabAsCmd.label;" accesskey="&addCurTabAsCmd.accesskey;" 
>+                          oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
>+                                     addBookmarkAs(getBrowserForTab(tabbrowser.mContextTab));"/>
>+            <xul:menuitem label="&addAllTabsAsCmd.label;" accesskey="&addAllTabsAsCmd.accesskey;"
>+                          command="Browser:AddBookmarkGroup"/>
>+            <xul:menuseparator/>
>             <xul:menuitem label="&closeTab.label;" accesskey="&closeTab.accesskey;"
>                           tbattr="tabbrowser-multiple"
>                           oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
>Index: locales/en-US/chrome/global/tabbrowser.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/global/tabbrowser.dtd,v
>retrieving revision 1.2
>diff -u -u -p -r1.2 tabbrowser.dtd
>--- locales/en-US/chrome/global/tabbrowser.dtd	24 Nov 2004 15:55:24 -0000	1.2
>+++ locales/en-US/chrome/global/tabbrowser.dtd	11 Jul 2005 23:35:04 -0000
>@@ -10,3 +10,7 @@
> <!ENTITY  reloadTab.label        "Reload Tab">
> <!ENTITY  reloadTab.accesskey         "r">
> <!ENTITY  newTabButton.tooltip   "Open a new tab">
>+<!ENTITY  addCurTabAsCmd.label "Bookmark This Tab...">
>+<!ENTITY  addCurTabAsCmd.accesskey "B">
>+<!ENTITY  addAllTabsAsCmd.label "Bookmark All Tabs...">
>+<!ENTITY  addAllTabsAsCmd.accesskey "T">
Attachment #188991 - Flags: review?(bugs)
Comment on attachment 188991 [details] [diff] [review]
patch for bookmark all tabs

hellooo bogus dependencies.  Toolkit bits cannot call functions in /browser,
its app-independent, and I don't want to add more items to the tab context menu
that aren't tab management-related, regardless.

As for the added menuitem in the bookmarks menu, that's probably necessary, but
that's a dupe to something much older, but we can ignore that for now.	I'm not
sure addbookmarkgroup is necessary in addition to addgroup if we're removing
the checkbox from the normal dialog, but I haven't done more than skim the
patch.

As a note, before you write a major UI patch, its usually a good idea to talk
to the app/module owners about how/if it should be implemented.
Attachment #188991 - Flags: review?(bugs) → review-
WONTFIX (as filed)

If you're still interested in the added menuitem for the bookmarks menu, please
find the original bug and attach a patch to just add that.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 5

12 years ago
Created attachment 191013 [details] [diff] [review]
patch for bookmark all tabs and tabs context menu
(Assignee)

Comment 6

12 years ago
Created attachment 191357 [details] [diff] [review]
updated patch for bookmark all tabs
Attachment #188991 - Attachment is obsolete: true
Attachment #191013 - Attachment is obsolete: true
(Assignee)

Comment 7

12 years ago
Created attachment 191361 [details] [diff] [review]
updated patch for bookmark all tabs
Attachment #191357 - Attachment is obsolete: true
Hi Mike. I am the module owner, and I talked to Grace about this :-)
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---

*** This bug has been marked as a duplicate of 267785 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
*** Bug 267785 has been marked as a duplicate of this bug. ***
Assignee: nobody → gsshih
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ben, that's all well and good, but the patch is unacceptable since it adds a bad
dependency.  It'd be nice if you actually talked to others about this type of
thing as well.
Comment on attachment 191361 [details] [diff] [review]
updated patch for bookmark all tabs

>+  if (numtabs > 1)
>+    bookmarkAllCommand.setAttribute("disabled", "false");
>+  else
>+    bookmarkAllCommand.setAttribute("disabled", "true");

Style nit: use interCaps for things like numTabs
Also, convention is to remove "disabled" attributes rather than set them to
false. 


>+
>+  var webNav = activeBrowser.webNavigation;
>+  var url = webNav.currentURI.spec;
>+  var name = "";
>+  var charSet, description;
> ... 

Do you really need to duplicate this entire function? Can't you make
"bookmark-all-tabs"ness a parameter instead and just use that as a trigger to
do certain tasks/etc?

>+  currentTabInfo = {name: "[Folder name]"};

This is not localizable. Put this string into browser.properties and use
gNavigatorBundle.getString() instead. 

>+  //  bookmarkDialog = document.getElementById("addBookmarkDialog");
>+  //  bookmarkDialog.setAttribute("title", "Bookmark All Tabs");

Get rid of this. 

>     this.urlBar                 = document.getElementById("urlbar");
>-
>+  

Kill the extra whitespace. 

>-    this.lastURI                = null;
>+    this.lastURI                = null; 

Ditto. 

>-          var browser = gBrowser.mCurrentBrowser;
>+          var browser = gBrowser.mCurrentBrowser;         

Ditto.

>+  getTitle();

Call this function "initTitle" or something... getTitle implies a function that
returns a value, which it does not. 

>   gSelectedFolder = RDF.GetResource(gMenulist.selectedItem.id);
>   gExpander.setAttribute("tooltiptext", gExpander.getAttribute("tooltiptextdown"));
>   gPostData = gArg.postData;
>@@ -155,6 +161,14 @@
>   }
> } 
> 
>+function getTitle()
>+{
>+  if(gArg.bAddBookmarkGroup)
>+    document.title = document.getElementById("addBookmarkDialog").getAttribute("titlenewgroup");
>+  else
>+    document.title = document.getElementById("addBookmarkDialog").getAttribute("titlenew");
>+}

Include a stringbundle in the addBookmark2.xul file like this:

<stringbundle id="bookmarksStrings"
src="chrome://browser/locale/bookmarks/bookmarks.properties"/>

and get these strings from there instead. 

>-        title="&newBookmark.title;" title-selectFolder="&selectFolder.label;"
>+        titlenew="&newBookmark.title;" titlenewgroup="&newBookmarkGroup.title;" 

See comment above about using stringbundle for these instead. 

>+            <xul:menuitem anonid="bookmarktab" label="&addCurTabAsCmd.label;" accesskey="&addCurTabAsCmd.accesskey;"/>

Like Mike said, put an anonid on the <xul:menupopup> and discover this from the
browser code, then insert these items programmatically, rather than adding them
here in the toolkit code. 

>+<!ENTITY  addCurTabAsCmd.label "Bookmark This Tab...">
>+<!ENTITY  addCurTabAsCmd.accesskey "B">
>+<!ENTITY  addAllTabsAsCmd.label "Bookmark All Tabs...">
>+<!ENTITY  addAllTabsAsCmd.accesskey "T">

Once you've done the above, these strings should go in browser.properties

Let's get another patch that fixes these things and I'll review again.
Attachment #191361 - Flags: review-
Mike, we did actually discuss this with beltzner.
(I'm talking about UI, not implementation - grace and I did discuss how this
should be done - and the latest patch is not there yet, see my review for details). 
(Assignee)

Comment 15

12 years ago
Created attachment 192145 [details] [diff] [review]
new bookmarks patch

followed review comments to fix patch
Attachment #191361 - Attachment is obsolete: true
Comment on attachment 192145 [details] [diff] [review]
new bookmarks patch

>+  if (aBookmarkAllTabs) {
>+    dialogArgs = {name: gNavigatorBundle.getString('bookmarkAllTabsDefault')};

Style nit: spaces around the { and }
and double quotes are the convention for string literals in JavaScript files. 

That + moving the initialization to delayedStartup gets r=ben@mozilla.org. 

I'll let you attach a new patch so I can apply it and land it for you.
(Assignee)

Comment 17

12 years ago
Created attachment 192147 [details] [diff] [review]
newest bookmarks patch

Done! Thanks Ben.
Attachment #192145 - Attachment is obsolete: true
Comment on attachment 192147 [details] [diff] [review]
newest bookmarks patch

>   var url, rSource;
>   var livemarkFeed = gArg.feedURL;
>+  if (gArg.bBookmarkAllTabs){
>+    gGroup.checked = true;
>+  }

Oops I didn't notice this the first time... we should just eliminate the
checkbox completely if we're never going to show it, right? 

Get rid of the checkbox from addBookmark2.xul and this code as well, then in
onOK use gArg.bBookmarkAllTabs instead of gGroup.checked to determine whether
or not to add a bookmark group.
Attachment #192147 - Flags: review-
(Assignee)

Comment 19

12 years ago
Created attachment 192155 [details] [diff] [review]
removed the checkbox
Attachment #192147 - Attachment is obsolete: true
(Assignee)

Comment 20

12 years ago
Created attachment 192162 [details] [diff] [review]
it works now! =)
Attachment #192155 - Attachment is obsolete: true
(Assignee)

Comment 21

12 years ago
Created attachment 192166 [details] [diff] [review]
sorry actually this works a lot better
Attachment #192162 - Attachment is obsolete: true

Comment 22

12 years ago
If I understand this bug correctly you're adding "Bookmarks-> Bookmark All
Tabs", correct?  If so can you remove any accesskey you've created for that as
we've just recently removed the accesskeys for "Bookmark This Page" & "Manage
Bookmarks" due to bug 261985.  Any accesskey key you create will cause the same
issue that is discussed in that bug.  The root issue will be fixed after 1.5 and
the accesskeys added back at that time.

~B
(Assignee)

Comment 23

12 years ago
Created attachment 192265 [details] [diff] [review]
removed accesskey for bookmark all tabs menuitem
Attachment #192166 - Attachment is obsolete: true
Comment on attachment 192265 [details] [diff] [review]
removed accesskey for bookmark all tabs menuitem

r=ben@mozilla.org
Attachment #192265 - Flags: review+
b4+ since contains l10n. landed. 
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Flags: blocking1.8b4+
Resolution: --- → FIXED
At least from looking at bonsai, this wasn't checked in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Blocks: 304405

Comment 27

12 years ago
We now have "Bookmark This Page..." on the main menu and "Bookmark This Tab..."
on the context menu. Not sure if that inconsistency is an oversight or by design.
"Bookmark This Tab" is visible and enabled when right clicking on blank parts of
the tabbar (And, triages a JS error).

Comment 29

12 years ago
Yay. I was beginning to worry that there were still some context menus in
Firefox which hadn't been ruined by adding in tons of pointless **** to
accidentally hit yet. Here's hoping the text selection context menu gets "add
this select text as a bookmarklet" and "send this text by email to" sooner
rather than later.

 - Chris
Target Milestone: --- → Firefox1.1
Blocks: 304748

Updated

12 years ago
Depends on: 304861

Updated

12 years ago
Depends on: 304558
Depends on: 305216

Comment 30

12 years ago
But Now I can't Ctrl+D click a check box and bookmark all tabs, how will I do
that now? Will there be a new shortcut key? Can't you just leave the checkbox
were it was?

Comment 31

12 years ago
Keyboard shortcut for this feature -> bug 305955
You need to log in before you can comment on or make changes to this bug.