588011 - "Bookmark All Tabs" should ignore App Tabs; hide the "Bookmark All Tabs" menu item unless the user invokes the menu using the keyboard

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Paul [pwd]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre

If you attempt to 'Bookmark All Tabs' while you have some App Tabs open, it bookmarks the App Tabs too. App Tabs should be ignored when attempting such an action.

Reproducible: Always

Comment 1

[Paul [pwd]]

Given all the discussion regarding what App Tabs are supposed to be and how they're supposed to act, I'd say this is a bug rather than an enhancement? Perhaps we could get someone from the UX team to confirm?

Comment 2

[Marcia Knous [:marcia]]

Paul: I think the discussion re: this is happening in Bug 587295.

Comment 3

[Paul [pwd]]

Thank you Marcia, I added that bug as a blocker.

Comment 4

[eyal gruss]

there is a use case for bookmarking app tabs:
i use bookmarks all tabs to a folder to save the session sometimes

Comment 5

[Paul [pwd]]

(In reply to comment #4)
> there is a use case for bookmarking app tabs:
> i use bookmarks all tabs to a folder to save the session sometimes

If you had some App Tabs open, chances are they'll be open when you restore the session.

That said, on the dialog where it asks you to name the folder, a tick box could be provided in order to bookmark the app tabs or not.

Comment 7

[Marco Bonardo [:mak]]

needs UX, but sounds correct, bookmarking all tabs relates to vurrent visited pages, not to persisted apps.

Comment 8

[mkirsch358]

(In reply to comment #4)
> there is a use case for bookmarking app tabs:
> i use bookmarks all tabs to a folder to save the session sometimes

But now with Panorama, bookmarking all tabs on ly saves the current group, not the whole session.

I often want to bookmark a group, but the app tabs (which are there no matter what group you are viewing) are always there, and get bookmarked.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This implements the requested behavior.

Notice that since doing it correctly was going to add more tab listeners to gBookmarkAllTabsHandler (TabPinned/TabUnpinned) and bug 607227 requires to remove Bookmark All Tabs from bookmarks menu, it was going to be a useless perf hog on tab opening/closing.
Thus I decided to kill that crazy handler and properly make the menuitem updated on tab context popupshowing, like the other entries in that context menu.

Thus, for performance reasons, this implements both this fix and the fix for bug 607227.

Asking UI-R to be sure that not bookmarking pinned tabs is finally what we want.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 504493 [details] [diff] [review]
patch v1.0

Yep, we shouldn't bookmark pinned tabs

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 504493 [details] [diff] [review]
patch v1.0

Sorry, but I don't think I shouldn't be the one reviewing this - I've never even looked at half this code (plus, you know, I'm not a browser or places peer). Throwing it in Dietrich's general direction.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 504493 [details] [diff] [review]
patch v1.0

Dao, if you have any spare time to look through this  and/or review, please feel free to.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 504493 [details] [diff] [review]
patch v1.0

>+  get uniqueCurrentPages() {
>+    let uniquePages = {};
>+    let URIs = [];
>+    gBrowser.visibleTabs.forEach(function (tab) {
>+      if (!tab.pinned && gBrowser._removingTabs.indexOf(tab) == -1 &&
>+          !(tab.linkedBrowser.currentURI.spec in uniquePages)) {
>+        URIs.push(tab.linkedBrowser.currentURI);
>+      }
>+    });

visibleTabs won't contain tabs in the _removingTabs array.

>-    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:BookmarkAllTabs" modifiers="accel,shift"/>

Why are you removing this?

>@@ -8222,16 +8184,20 @@ var TabContextMenu = {
>     document.getElementById("context_unpinTab").hidden = !this.contextTab.pinned;
> 
>     // Disable "Close other Tabs" if there is only one unpinned tab and
>     // hide it when the user rightclicked on a pinned tab.
>     let unpinnedTabs = gBrowser.visibleTabs.length - gBrowser._numPinnedTabs;
>     document.getElementById("context_closeOtherTabs").disabled = unpinnedTabs <= 1;
>     document.getElementById("context_closeOtherTabs").hidden = this.contextTab.pinned;
> 
>+    // Disable "Bookmark All Tabs" if there are less than 2 bookmarkable tabs.
>+    document.getElementById("context_bookmarkAllTabs").disabled =
>+      PlacesCommandHook.uniqueCurrentPages.length < 2;

We should hide the context menu item when right clicking a pinned tab.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> >-    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:BookmarkAllTabs" modifiers="accel,shift"/>
> 
> Why are you removing this?

It's not used anymore, afaict. it was used in the bookmarks menu item, that I'm removing.

> We should hide the context menu item when right clicking a pinned tab.

Ok, I thought disabling it was enough, I'll do.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> (In reply to comment #13)
> > >-    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:BookmarkAllTabs" modifiers="accel,shift"/>
> > 
> > Why are you removing this?
> 
> It's not used anymore, afaict. it was used in the bookmarks menu item, that I'm
> removing.

The key combo works regardless of the menu item.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 504493 [details] [diff] [review]
patch v1.0

I see your point to retain the key functionality, will do.

Comment 17

[Nochum Sossonko [:Natch]]

Where is the visibility of this feature? Why is it assumed all users will understand that Bookmark __All__ Tabs doesn't include App Tabs. This seems like it should require a string change to the tune of "Bookmark Normal Tabs" or some such. An App Tab is still a tab, as the name suggests, so All Tabs includes them too.

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 19

[Marco Bonardo [:mak]]

(In reply to comment #17)
> Why is it assumed all users will
> understand that Bookmark __All__ Tabs doesn't include App Tabs.

app tabs are intended as something you always keep open, and that persists its status across sessions. It's rare that a user could need to bookmark current transient opened tabs AND something that is intended to persist.

Comment 20

[Marco Bonardo [:mak]]

And also consider that with tab groups we bookmark current visible tabs, and app tabs are visible in all groups... so it's even more unlikely you want to bookmark them as part of the visible group.

Comment 21

[Nochum Sossonko [:Natch]]

(In reply to comment #19)
> app tabs are intended as something you always keep open, and that persists its
> status across sessions. It's rare that a user could need to bookmark current
> transient opened tabs AND something that is intended to persist.

I'm not arguing against fixing this, I'm arguing against the discover-ability and UI misdirection. I wholeheartedly agree that the mass-bookmarking should affect normal tabs as opposed to app tabs, I just think it will cause confusion because of the wording. Ultimately, the check box approach in comment 5 is the best option. There are multiple legitimate reasons for wanting to bookmark app tabs as well (e.g. when you plan on cleaning them off your tab toolbar and want to keep the website as a bookmark).

Let me put this even better: if user A right-clicks on App Tab and has a few normal tabs open, then clicks "Bookmark All Tabs" it won't bookmark the App Tab. That's just confusing and wrong.

Comment 22

[Paul [pwd]]

(In reply to comment #21)
> Let me put this even better: if user A right-clicks on App Tab and has a few
> normal tabs open, then clicks "Bookmark All Tabs" it won't bookmark the App
> Tab. That's just confusing and wrong.

The option isn't shown if you right click on an App Tab.

Comment 23

[Nochum Sossonko [:Natch]]

(In reply to comment #22)
> The option isn't shown if you right click on an App Tab.

Right, missed that part of the patch. In any event, the rest stands.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 505074 [details] [diff] [review]
patch v1.1

>-      <menuitem id="menu_bookmarkAllTabs"
>-                label="&addCurPagesCmd.label;"
>-                command="Browser:BookmarkAllTabs"
>-                key="bookmarkAllTabsKb"/>

Unfortunately this makes the keyboard shortcut entirely undiscoverable...

>   /**
>    * This function returns a list of nsIURI objects characterizing the

You're changing this so that it's not a function anymore.

>    * tabs currently open in the browser.  The URIs will appear in the
>    * list in the order in which their corresponding tabs appeared.  However,
>    * only the first instance of each URI will be returned.
>    *
>-   * @returns a list of nsIURI objects representing unique locations open
>+   * @return A list of nsIURI objects representing unique locations open.
>+   * @note Pinned tabs are excluded.

I don't know what @note is supposed to be used for. Seems like "Pinned tabs are excluded" would fit just fine into the paragraph above this. E.g. "a list of nsIURI objects representing the currently open tabs, modulo pinned tabs."

>+    // Hide "Bookmark All Tabs" for a pinned tab.
>+    let bmAllTabsElt = document.getElementById("context_bookmarkAllTabs");
>+    bmAllTabsElt.hidden = this.contextTab.pinned;
>+    // Disable "Bookmark All Tabs" if it's visible but there are less than 2
>+    // bookmarkable tabs.

Please merge the comments, e.g.:

// Hide "Bookmark All Tabs" on pinned tabs,
// disable it if there are less than two "unique current pages".

>+    if (!bmAllTabsElt.hidden) {
>+      bmAllTabsElt.disabled = PlacesCommandHook.uniqueCurrentPages.length < 2;
>+    }

Remove braces.

s/bmAllTabsElt/bookmarkAllTabs/ please.

Comment 25

[Paul [pwd]]

(In reply to comment #23)
Granted a tick box could be provided, but it's far from a must-have. And if
it's a trade-off between discoverability and the tick-box (adding a new string
thus deferring the patch to a later firefox release), I'd rather we shipped
without the discoverability. Which I personally don't feel is a major concern
here. Perhaps file a followup bug to deal with that?

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #24)
> Comment on attachment 505074 [details] [diff] [review]
> patch v1.1
> 
> >-      <menuitem id="menu_bookmarkAllTabs"
> >-                label="&addCurPagesCmd.label;"
> >-                command="Browser:BookmarkAllTabs"
> >-                key="bookmarkAllTabsKb"/>
> 
> Unfortunately this makes the keyboard shortcut entirely undiscoverable...

true, but I guess keep supporting it is mostly for backwards compatibility with user habits and keyboard access.  Those users should know about it already.
The remaining part could be fixed by a future about:shortcuts page (I think we plan that?). This is not the only undiscoverable shortcut we have I think.

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #26)
> The remaining part could be fixed by a future about:shortcuts page (I think we
> plan that?).

I don't know. Anyway, I don't like patches landing with the assumption that some future project will save us.

> This is not the only undiscoverable shortcut we have I think.

I'd be surprised if there are many more. We should certainly try to avoid them.

Comment 28

[Marco Bonardo [:mak]]

Well, I'm not making assumptions, I based my patch on the UX request in bug 607227.
The best suggestion I can found to deal with menu removals is https://bugzilla.mozilla.org/show_bug.cgi?id=513168#c3, unfortunately that patch never landed and we can't add strings at this point.
Will ping Alex on the issue, the best thing I can do otherwise, is not removing the entry (thus not fixing bug 607227) and handling its update in popupshowing of the bookmarks menu. Then we could add this shortcuts page to 4.x and do menus cleanup later than FX4.

Comment 29

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Ok, asked about "the secret plot" to Alex.
The long term solution for removing menuitems with associated discoverable shortcuts will be a shortcuts panel/page opened through shift-? and a menuitem in the Help menu.
The short term solution decided for Firefox 4 release is leveraging bug 607224. The menuitem must be hidden for mouse interactions, but visible if the menu has been opened through keyboard access.

Thus, I added a new helper to the legacy bookmarks menu to handle this behavior.
Notice that bug 607224 does not support Mac, thus there the menuitem will always be visible.
Fixed other comments, too.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 505242 [details] [diff] [review]
patch v1.2

>+  onLegacyBookmarksPopupShowing:

New name please, that popup isn't "legacy" or going away anytime soon, afaik.

>+  function BEH_onLegacyBookmarksPopupShowing(event) {
>+    // Hide the "Bookmark All Tabs" menuitem if the menu is not opened by
>+    // keyboard.  Disable it if there are less than two "unique current pages".
>+    let bookmarkAllTabs = document.getElementById("menu_bookmarkAllTabs");
>+    bookmarkAllTabs.hidden = !event.target.parentNode.openedWithKey;

Does this hide it always on OS X?

Comment 31

[Marco Bonardo [:mak]]

(In reply to comment #30)
> Does this hide it always on OS X?

based on the comments in bug 607224 it should always show, BUT looking at the patch that's unclear, also because the value is initialized to PR_FALSE.
So I'm going to directly check on Mac.

Would you be fine using the method in bug 607224 Comment 15 and bug 607224 Comment 16 rather than the single onpopupshowing handler I have here? That would make trivial to implement bug 626825

Comment 32

[Marco Bonardo [:mak]]

So, checking on Mac, openedWithKey is always false, that is somehow a problem for our scope (show the entry till Mac properly supports this).

I'd suggest going in browser-menubar.inc for

<menubar id="main-menubar"
         style="border:0px;padding:0px;margin:0px;-moz-appearance:none"
#ifdef XP_MACOSX
         openedWithKey="true"
#else
         onpopupshowing="this.setAttribute('openedWithKey',
                           event.target.parentNode.openedWithKey);"
#endif
>

and adding in browser/base/content/browser.css a

#main-menubar[openedWithKey=false] menuitem[show-only-for-keyboard] {
  display:none;
}

Dao, what do you think of this approach?

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> So, checking on Mac, openedWithKey is always false, that is somehow a problem
> for our scope (show the entry till Mac properly supports this).
> 
> I'd suggest going in browser-menubar.inc for
> 
> <menubar id="main-menubar"
>          style="border:0px;padding:0px;margin:0px;-moz-appearance:none"
> #ifdef XP_MACOSX
>          openedWithKey="true"
> #else
>          onpopupshowing="this.setAttribute('openedWithKey',
>                            event.target.parentNode.openedWithKey);"
> #endif
> >
> 
> and adding in browser/base/content/browser.css a
> 
> #main-menubar[openedWithKey=false] menuitem[show-only-for-keyboard] {
>   display:none;
> }
> 
> Dao, what do you think of this approach?

The only problem here is the hardcoded #ifdef XP_MACOSX, as Firefox on Ubuntu may get native menu bar support too (for Firefox 4 even, via an extension).

Comment 34

[Marco Bonardo [:mak]]

it's a bit verbose, but we could

onpopupshowing="this.setAttribute('openedWithKey',
                 '@mozilla.org/widget/nativemenuservice;1' in Components.classes ?
                  "true" : event.target.parentNode.openedWithKey);"

Comment 35

[Dão Gottwald [:dao]]

Sounds okay, except that I'd write it this way:

onpopupshowing="if (event.parentNode.parentNode == this &&
                    !('@mozilla.org/widget/nativemenuservice;1' in Cc))
                  this.setAttribute('openedwithkey', event.target.parentNode.openedWithKey);"

Comment 36

[Marco Bonardo [:mak]]

Attachment: patch v1.3

A couple notes where I differ from previous comments, to easy the diff review:
- I used full Components.classes, not Cc shortcut, I'm not sure which Cc would browser.xul use, but if it's the one defined in placesOverlay.xul that's a bug (bug 406371) an we should not rely on those. If there is another proper Cc defined somewhere else included by browser.xul, let me know.
- I used a class named show-only-for-keyboard, it looks more readable and clear than a attribute plus we can apply it to menus or separators if needed.
- I still have a onpopupshowing handler method for the bookmarks popup, it is actually just doing a single op and could be moved to a inlined onpopupshowing, but I didn't want to pollute more the popup code plus the handler can be reused easier, but I'm fine both ways, just let me know.

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #36)
> Created attachment 506461 [details] [diff] [review]
> patch v1.3
> 
> A couple notes where I differ from previous comments, to easy the diff review:
> - I used full Components.classes, not Cc shortcut, I'm not sure which Cc would
> browser.xul use, but if it's the one defined in placesOverlay.xul that's a bug
> (bug 406371) an we should not rely on those. If there is another proper Cc
> defined somewhere else included by browser.xul, let me know.

The bug isn't that Cc is defined in the browser.xul context, the bug is that it's not defined by browser.js. Bug 406371 can't possibly change the former, as it would break the world, but only the latter.

Comment 38

[Marco Bonardo [:mak]]

(In reply to comment #37)
> The bug isn't that Cc is defined in the browser.xul context, the bug is that
> it's not defined by browser.js. Bug 406371 can't possibly change the former, as
> it would break the world, but only the latter.

Sure, the bug is about it being defined in the wrong place, if we want them defined globally. The original browser impl. did not want them, and other projects (seamonkey ad an example) still have to port all browser changes converting shortcuts to the extended form. While I also think we should keep and move them to a proper place, for now I'd prefer using the safer extended version till that smoke is cleared and a decision is taken.

Comment 39

[Dão Gottwald [:dao]]

(In reply to comment #38)
> The original browser impl. did not want them,

In, what, 2004? 2002? Today the constants are used all over the place in browser.js and related code, plus extensions, so I can't imagine that any browser peer would suggest or support removing them. It's just not an option for us, as far as I can tell. No smoke, no pending decision.

Comment 40

[Marco Bonardo [:mak]]

So far I don't like that browser code depends on constants defined by a overlay, but if that's fine for you I will do it. It's clearly a temporary bad condition that will be fixed.
Is this the only remaining comment? I'd prefer touching the patch just one time to fix them. So if this is a "r=me provided you use Cc" I'll update the patch, otherwise I prefer waiting for the proper review.

Comment 41

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

Relying on Cc/Ci being defined in the main browser window scope is not a problem (regardless of how that's currently achieved). We may change how/where they are defined, but we're quite unlikely to change the fact that they are, as Dao mentions.

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 506461 [details] [diff] [review]
patch v1.3

You removed Browser:BookmarkAllTabs but kept using it in one place.
I think we should keep the command for add-on compatibility, but we may get away with not updating its disabled state.

>        <menubar id="main-menubar"
>-                style="border:0px;padding:0px;margin:0px;-moz-appearance:none">
>+                style="border:0px;padding:0px;margin:0px;-moz-appearance:none"
>+                onpopupshowing="if (event.target.parentNode.parentNode == this &amp;&amp;
>+                                    !('@mozilla.org/widget/nativemenuservice;1' in Components.classes))
>+                                  this.setAttribute('openedwithkey',
>+                                                    event.target.parentNode.openedWithKey);">

nit: if you put onpopupshowing before style, you can leave the style line untouched. Blame might be interesting here (haven't checked).

Comment 43

[Marco Bonardo [:mak]]

do you want me to just reinsert Browser:BookmarkAllTabs command but leave it unused in our code, or to actually use it everywhere as command="Browser:BookmarkAllTabs" rather than using oncommand?

Comment 44

[Dão Gottwald [:dao]]

We should also make use of it.

Comment 45

[Marco Bonardo [:mak]]

the problem is that, if I set a command, the disabled state of the menuitem is inherited from the command. If i make our code update the command disabled state, it could persist in the wrong state when invoked from keyboard.

Comment 46

[Marco Bonardo [:mak]]

Attachment: patch v1.4

Afaict, this is the maximum I can do with the command, I'm using it in the key element, while menuitems use oncommand. The command inherited disabled value seems to have precedence, even if it's not set.

Comment 47

[Marco Bonardo [:mak]]

Attachment: patch v1.4

sorry was the wrong one

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 506740 [details] [diff] [review]
patch v1.4

Rather than enabling/disabling the individual items, can you update the command state on demand?

Comment 49

[Marco Bonardo [:mak]]

Yes but, suppose that I update it to disabled when you open the context menu of a tab, because you have only one tab. Then you open another tab and you invoke the command through the key combo CTRL+SHIFT+D, at this point the command is wrongly disabled.

Comment 50

[Marco Bonardo [:mak]]

unless I use oncommand on the key element and command on the menuitems. You fine with this?

Comment 51

[Marco Bonardo [:mak]]

even if still for add-ons the command could be disabled at the wrong time.

Comment 52

[Dão Gottwald [:dao]]

(In reply to comment #51)
> even if still for add-ons the command could be disabled at the wrong time.

Add-ons would need to trigger updates of the command state. Providing a helper function to do that seems to me like the correct course of action.

Comment 53

[Marco Bonardo [:mak]]

Attachment: patch v1.5

- uses the command everywhere, but in the key element that must be always enabled
- added a helper to update the command state, used in both places

Comment 54

[Dão Gottwald [:dao]]

Comment on attachment 507148 [details] [diff] [review]
patch v1.5

>+  updateBookmarkAllTabsCommand:
>+  function PCH_updateBookmarkAllTabsCommand() {
>+    // Disable "Bookmark All Tabs" if there are less than two
>+    // "unique current pages".
>+    let command = document.getElementById("Browser:BookmarkAllTabs");
>+    if (this.uniqueCurrentPages.length < 2) {
>+      command.setAttribute("disabled", "true");
>+    }
>+    else {
>+      command.removeAttribute("disabled");
>+    }

goSetCommandEnabled("Browser:BookmarkAllTabs",
                    this.uniqueCurrentPages.length >= 2);
?

Comment 55

[Marco Bonardo [:mak]]

Attachment: patch v1.6

Ah, nice helper! The old code was not using it and unfortunately I was not aware of it. Replaced the code with the helper.

Comment 56

[Dão Gottwald [:dao]]

Comment on attachment 507153 [details] [diff] [review]
patch v1.6

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css
>@@ -511,3 +511,8 @@ window[chromehidden~="toolbar"] toolbar:
> browser[tabmodalPromptShowing] {
>   -moz-user-focus: none !important;
> }
>+
>+/* Hide menu elements intended for keyboard access support */
>+#main-menubar[openedwithkey=false] .show-only-for-keyboard {
>+  display: none;
>+}

Can you move this up to some remotely related stuff, e.g. to the menu rules right before the /* ::::: location bar ::::: */ line?

Comment 57

[Marco Bonardo [:mak]]

Attachment: patch v1.7

> Can you move this up to some remotely related stuff, e.g. to the menu rules
> right before the /* ::::: location bar ::::: */ line?

Done! I was looking for a place with menubar rules, but I didn't find any (apart the app-menu ones), so I ended up just appending.
Thanks.

Comment 58

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/e2f9d9c8b18c

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110201 Firefox/4.0b11pre

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also updated the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=10033