Bookmark all Tabs menuitem not properly enabled / disabled

VERIFIED FIXED in Firefox1.5

Status

()

--
major
VERIFIED FIXED
14 years ago
13 years ago

People

(Reporter: bangbang023, Assigned: vlad)

Tracking

({fixed1.8})

Trunk
Firefox1.5
x86
Windows XP
fixed1.8
Points:
---
Bug Flags:
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
The "Bookmark all tabs..." option is not disabled at startup when using
about:blank as homepage. It is properly disabled when one tab is open as soon as
a page is loaded.
An inverse (but likely related) problem is that if a user:

  - opens a new browser
  - opens links as tabs in background

the menu item is *disabled* when it should be enabled.
Summary: Bookmark all Tabs is not disabled at startup as should be → Bookmark all Tabs menuitem not properly enabled / disabled
The code which updates the disabled state of the command is living in
onLocationChange, pretty bogus for many cases...
Severity: normal → major
Flags: blocking1.8b4?
Target Milestone: --- → Firefox1.5

Updated

14 years ago
Flags: blocking1.8b4? → blocking1.8b4+

Comment 3

14 years ago
Ben - can you take a look?
Assignee: nobody → beng
Gonna grab this from ben (who probably doesn't even know he has it ;)
Assignee: beng → vladimir
Speaking of things in the background: open some tabs, change to one to enable
Bookmark All Tabs, then right-click that tab and choose "Close Other Tabs" and
Bookmark All Tabs remains enabled.
What you really want here is some notification (i.e. events) from tabbrowser on
removal and addition of tabs.

Another approach would be to use onpopupshowing but that wouldn't play nice w/
the keyboard shortcut (unless you're doing the same checks inside the oncommand
code as well.).
Created attachment 194120 [details] [diff] [review]
bookmark-all-tabs.patch

I think adding more events is too risky at this point; checking in
onpopupshowing and also fudging a bit if we get into the command via a keyboard
shortcut as Mano suggested seems to get the job done.
Attachment #194120 - Flags: review?(bugs.mano)
Comment on attachment 194120 [details] [diff] [review]
bookmark-all-tabs.patch

 - The tabbar context menu isn't fixed here, you need to apply the
onpopupshowing attribute on it as well (dynamically, in
|addBookmarkMenuitems()|).

>+  // we only disable the menu item on onpopupshowing; if we get
>+  // here via keyboard shortcut, we need to pretend like
>+  // nothing happened if we have no tabs
>+  if ((!browsers || browsers.length == 1) && aBookmarkAllTabs)
>+    return;

Isn't it enough to check for |aBookmarkAllTabs| here?

 - Mac only, the menuitem is enabled in non-browser windows (or, when no window
is open), you might also get a JS error when opening the bookmarks menu; please
disable the command if |getBrowser()| returns null.
Attachment #194120 - Flags: review?(bugs.mano) → review-
eek, ignore the part on |aBookmarkAllTabs|.
Created attachment 194132 [details] [diff] [review]
updated patch

Updated patch with comments (and irc comments) incorporated
Attachment #194120 - Attachment is obsolete: true
Attachment #194132 - Flags: review?(bugs.mano)
Created attachment 194135 [details] [diff] [review]
fixed for reals

Blah, fixed for real this time.. I misunderstood when we created those menu
items.
Attachment #194132 - Attachment is obsolete: true
Attachment #194135 - Flags: review?(bugs.mano)
Comment on attachment 194135 [details] [diff] [review]
fixed for reals

> function UpdateBookmarkAllTabsMenuitem()
> {
>   var tabbrowser = getBrowser();
>-  var numTabs = tabbrowser.tabContainer.childNodes.length;
>+  var numtabs = 0;
>+  if (tabbrowser)
>+    numTabs = tabbrowser.tabContainer.childNodes.length;

s/numtabs/numTabs in declaration.


>+  // set up the bookmarkAllTabs menu item correctly when the menu popup is shown
>+  tabMenu.addEventListener("popupshowing", function(e) { UpdateBookmarkAllTabsMenuitem(); }, false);

The addEventListner call can be simplied to:
>+  tabMenu.addEventListener("popupshowing", UpdateBookmarkAllTabsMenuitem, false);

r=mano with those fixed.
Attachment #194135 - Flags: review?(bugs.mano) → review+
Can we get this landed on the trunk, resolved as fixed and verified, before we
take on the branch?
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Verified with Windows trunk build: 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050830
Firefox/1.6a1
Status: RESOLVED → VERIFIED

Updated

14 years ago
Attachment #194135 - Flags: approval1.8b4? → approval1.8b4+
checked in on branch
Keywords: fixed1.8
The browser.js part of this patch that prevents the shortcut key from working
when it shouldn't didn't make the branch for some reason. Not a big deal I
guess, it seems to behave the same as "Add Bookmark".
Created attachment 198602 [details] [diff] [review]
just the keyboard accelerator fix

Huh; I wonder if I checked in the wrong version of the patch to the trunk. 
This is just the second half of the above patch; it's been baking on the trunk
for a while.
Attachment #198602 - Flags: approval1.8b5?

Updated

13 years ago
Flags: blocking1.8b5+ → blocking1.8rc1+
Keywords: fixed1.8
Comment on attachment 198602 [details] [diff] [review]
just the keyboard accelerator fix

moving request to 1.8rc1 since 1.8b5 has shipped.
Attachment #198602 - Flags: approval1.8b5? → approval1.8rc1?

Updated

13 years ago
Attachment #198602 - Flags: approval1.8rc1? → approval1.8rc1+
Checked in remaining fix bits, thanks
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.