304499 - Bookmark all Tabs menuitem not properly enabled / disabled

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Christopher Vendemio]

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.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 2

[Asaf Romano (gone)]

The code which updates the disabled state of the command is living in
onLocationChange, pretty bogus for many cases...

Comment 3

[Mike Schroepfer]

Ben - can you take a look?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Gonna grab this from ben (who probably doesn't even know he has it ;)

Comment 5

[Phil Ringnalda (:philor)]

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.

Comment 6

[Asaf Romano (gone)]

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.).

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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.

Comment 8

[Asaf Romano (gone)]

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.

Comment 9

[Asaf Romano (gone)]

eek, ignore the part on |aBookmarkAllTabs|.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

Updated patch with comments (and irc comments) incorporated

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixed for reals

Blah, fixed for real this time.. I misunderstood when we created those menu
items.

Comment 12

[Asaf Romano (gone)]

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.

Comment 13

[Asa Dotzler [:asa]]

Can we get this landed on the trunk, resolved as fixed and verified, before we
take on the branch?

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in on trunk

Comment 15

[Tracy Walker [:tracy]]

Verified with Windows trunk build: 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050830
Firefox/1.6a1

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in on branch

Comment 17

[:Gavin Sharp [email: [e-mail]]]

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".

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 198602 [details] [diff] [review]
just the keyboard accelerator fix

moving request to 1.8rc1 since 1.8b5 has shipped.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in remaining fix bits, thanks