Closed Bug 588011 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b11
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: pwd.mozilla, Assigned: mak)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: polish, Whiteboard: [softblocker])

Attachments

(1 file, 8 obsolete files)

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
Version: unspecified → Trunk
Severity: normal → enhancement
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?
Severity: enhancement → normal
Keywords: uiwanted
Paul: I think the discussion re: this is happening in Bug 587295.
Blocks: 587295
Thank you Marcia, I added that bug as a blocker.
there is a use case for bookmarking app tabs:
i use bookmarks all tabs to a folder to save the session sometimes
(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.
Duplicate of this bug: 612241
needs UX, but sounds correct, bookmarking all tabs relates to vurrent visited pages, not to persisted apps.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 560198
Keywords: polish
(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.
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #504493 - Flags: ui-review?(faaborg)
Attachment #504493 - Flags: review?(bmcbride)
Comment on attachment 504493 [details] [diff] [review]
patch v1.0

Yep, we shouldn't bookmark pinned tabs
Attachment #504493 - Flags: ui-review?(faaborg) → ui-review+
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.
Attachment #504493 - Flags: review?(bmcbride) → review?(dietrich)
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.
Attachment #504493 - Flags: feedback?(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.
Attachment #504493 - Flags: feedback?(dao) → feedback-
(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.
(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 on attachment 504493 [details] [diff] [review]
patch v1.0

I see your point to retain the key functionality, will do.
Attachment #504493 - Flags: review?(dietrich)
blocking2.0: --- → betaN+
Whiteboard: [softblocker]
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #504493 - Attachment is obsolete: true
Attachment #505074 - Flags: review?(dao)
(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.
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.
(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.
(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.
(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 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.
(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?
(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.
(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.
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.
Attached patch patch v1.2 (obsolete) — Splinter Review
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.
Attachment #505074 - Attachment is obsolete: true
Attachment #505242 - Flags: review?(dao)
Attachment #505074 - Flags: review?(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?
(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
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?
(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).
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);"
Sounds okay, except that I'd write it this way:

onpopupshowing="if (event.parentNode.parentNode == this &amp;&amp;
                    !('@mozilla.org/widget/nativemenuservice;1' in Cc))
                  this.setAttribute('openedwithkey', event.target.parentNode.openedWithKey);"
Attached patch patch v1.3 (obsolete) — Splinter Review
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.
Attachment #505242 - Attachment is obsolete: true
Attachment #506461 - Flags: review?(dao)
Attachment #505242 - Flags: review?(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.
(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.
(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.
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.
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 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).
Attachment #506461 - Flags: review?(dao) → review-
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?
We should also make use of it.
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.
Attached patch patch v1.4 (obsolete) — Splinter Review
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.
Attachment #506461 - Attachment is obsolete: true
Attached patch patch v1.4 (obsolete) — Splinter Review
sorry was the wrong one
Attachment #506739 - Attachment is obsolete: true
Attachment #506740 - Flags: review?(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?
Attachment #506740 - Flags: review?(dao) → review-
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.
unless I use oncommand on the key element and command on the menuitems. You fine with this?
even if still for add-ons the command could be disabled at the wrong time.
(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.
Attached patch patch v1.5 (obsolete) — Splinter Review
- 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
Attachment #506740 - Attachment is obsolete: true
Attachment #507148 - Flags: review?(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);
?
Attached patch patch v1.6 (obsolete) — Splinter Review
Ah, nice helper! The old code was not using it and unfortunately I was not aware of it. Replaced the code with the helper.
Attachment #507148 - Attachment is obsolete: true
Attachment #507153 - Flags: review?(dao)
Attachment #507148 - Flags: review?(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?
Attachment #507153 - Flags: review?(dao) → review+
Attached patch patch v1.7Splinter Review
> 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.
Attachment #507153 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/e2f9d9c8b18c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
Blocks: 607227
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110201 Firefox/4.0b11pre
Status: RESOLVED → VERIFIED
Blocks: 626825
Summary: "Bookmark All Tabs" should ignore App Tabs → "Bookmark All Tabs" should ignore App Tabs; hide the "Bookmark All Tabs" menu item unless the user invokes the menu using the keyboard
Duplicate of this bug: 607227
No longer blocks: FF2SM
Blocks: 672711
Duplicate of this bug: 894139
Depends on: 1323130
You need to log in before you can comment on or make changes to this bug.