592900 - Update the bookmarks menu from the nav toolbar to match changes in the Firefox menu

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

We need to make a few changes to the bookmarks menu accessed from the navigation toolbar.  It needs to match the current organization of the bookmarks menu in the Firefox button, with one caveat:

If it is displayed on the navigation toolbar because the bookmarks toolbar is turned off, the first item should be "View bookmarks toolbar."  If the bookmarks toolbar is visible, that command should not appear. 

So:

View Bookmarks Toolbar (only if it is off)
----------------
Show All Bookmarks (new text for "organize bookmarks")
----------------
Bookmark This Page
Subscribe to This Page.. (only display icon if active)
----------------
bookmarks toolbar items (if the bar is off, inline, jut a sub-menu)
----------------
bookmarks menu items
----------------
Unsorted Bookmarks (single item that launches the library, not a sub-menu)

Comment 1

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

We need to get this done as part of addressing bug 578967 (which I believe should also block as part of the new theme work)

Comment 2

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

related bug 592908 for updating the text in the Firefox menu

Comment 3

[(mostly gone) XtC4UaLL [:xtc4uall]]

Is the Loss of the "Show in Sidebar" Menuitem on Purpose/intended?
How about not removing it - since there would be no Menuitem for it at all in the default UI - and placing it above/under the "Show All Bookmarks" Item?

Comment 4

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

Yes, although we are keeping the keyboard shortcut and optional toolbar button

Comment 5

[Axel Hecht [:Pike]]

This has none of a patch, an owner, or a blocking decision but strings impact.

What's the triage or ETA status here?

Comment 6

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

strings should be the same as the current Firefox menu, right?

Comment 7

[Axel Hecht [:Pike]]

(In reply to comment #6)
> strings should be the same as the current Firefox menu, right?

Wouldn't you answer that question?

String re-use at large between the Firefox menu and other menu items is a larger topic which we'll likely revisit when we get to porting over to l20n.

Comment 8

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

>Wouldn't you answer that question?

The added string of "View bookmarks toolbar" is the only difference, and it is already in.  Removing [strings], assuming we can just reuse the existing entities.

Comment 9

[Axel Hecht [:Pike]]

For the Firefox menu, re-using other strings that were menu strings for the same functionality is fine.

That said, I've seen bug reports on the width of the columns of the fx menu, though I don't recall which bug that was. How do we control the width of the fx menu?

Comment 10

[Frank Yan (:fryn)]

Faaborg, I highly recommend that we handle the addition of the 'Subscribe to this page...' menu item to the bookmarks menu button's menu in this bug and leave only the removal of the feed button to bug 578967 to prevent merge conflicts and maintain the ability to subscribe in a toolbar item. I will update bug 578967's patch to do just that.

Comment 11

[Frank Yan (:fryn)]

Attachment: patch

The only thing missing is the lack of an icon for 'Subscribe to this page', but it's still enabled/disabled accordingly. If having an icon is really wanted, please file a followup.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 475752 [details] [diff] [review]
patch

>     // Update View Bookmarks Toolbar checkbox menuitem.
>-    viewToolbar.setAttribute("checked", !this.personalToolbar.collapsed);
>+    let toolbarVisible = !this.personalToolbar.collapsed;
>+    viewToolbar.setAttribute("checked", toolbarVisible);
>+    viewToolbar.setAttribute("hidden", toolbarVisible);
>+    document.getElementById("BMB_viewBookmarksToolbar_separator").
>+      setAttribute("hidden", toolbarVisible);

I think we shouldn't do this (or at least discuss it in a followup, so as to unblock this one).

Consider a user exploring the default UI (no bookmarks toolbar). They select this menuitem, decide they don't like the change, and go to undo it by... crap. The menuitem has magically disappeared!

The topmost placement of this menuitem kind of sucks, anyway. I'd assume most people prefer the toolbar either on or off, and don't toggle the state. Putting it at the top is odd because it would be infrequently used, but at a prime spot to be accidentally clicked.

How about we just remove this menuitem? (In a followup ;). It's already accessible via the old menubar, and also via App Button -> Options -> Bookmarks Toolbar.


>+++ b/browser/base/content/browser.css
...
>+#BMB_unsortedBookmarksFolder_separator, #BMB_unsortedBookmarksFolder {
>+  -moz-box-ordinal-group: 999999;
>+}

I suppose this is sub-optimal, as an addon XUL overlay trying to add a menuitem after this would see it appear in the wrong spot?

>+++ b/browser/base/content/browser.xul

>+            <menuseparator id="BMB_bookmarksToolbarFolderPopup_separator"/>
>+            <menuseparator id="BMB_unsortedBookmarksFolder_separator"/>

Err, won't this cause adjacent seperators? Oh, right, -moz-box-ordinal-group. :/

>             <menu id="BMB_bookmarksToolbarFolderMenu"

This patch also isn't implementing Alex's request to make the UI like the FF button (toolbar bookmarks inline, unless toolbar is shown). Hmm, maybe we should just trim this down to a partial patch adding "Subscribe to page", so that bug 578967 can land?

Comment 13

[Frank Yan (:fryn)]

Comment on attachment 475752 [details] [diff] [review]
patch

Definitely misread some things. Will fix later.

I share Dolske's concern about the menuitem, but meanwhile, I'll file another bug for just the 'subscribe to this page' menu item, so we can land that first.

Comment 14

[Marco Bonardo [:mak]]

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

>     // Update View Bookmarks Toolbar checkbox menuitem.
>-    viewToolbar.setAttribute("checked", !this.personalToolbar.collapsed);
>+    let toolbarVisible = !this.personalToolbar.collapsed;
>+    viewToolbar.setAttribute("checked", toolbarVisible);
>+    viewToolbar.setAttribute("hidden", toolbarVisible);
>+    document.getElementById("BMB_viewBookmarksToolbar_separator").
>+      setAttribute("hidden", toolbarVisible);

as Dolske said, don't do this. Just let the code as it is.

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css
>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css
>@@ -238,16 +238,20 @@ toolbarbutton.bookmark-item {
> %ifdef MENUBAR_CAN_AUTOHIDE
> #toolbar-menubar:not([autohide="true"]) ~ #nav-bar > #bookmarks-menu-button-container,
> #toolbar-menubar:not([autohide="true"]) ~ toolbar > #personal-bookmarks > #bookmarks-menu-button,
> #toolbar-menubar:not([autohide="true"]) > #personal-bookmarks > #bookmarks-menu-button {
>   display: none;
> }
> %endif
> 
>+#BMB_unsortedBookmarksFolder_separator, #BMB_unsortedBookmarksFolder {
>+  -moz-box-ordinal-group: 999999;
>+}


You should not do stuff like this. Also the stuff in appmenu to append unsorted is ugly and unneeded.
This kind of things can completely break drag&drop targeting.
What you want to do is simply adding stuff in the correct order in xul, then where you want Places injected items to stop set attribute builder="end"
So you'll end up with:

<menuseparator id="BMB_unsortedBookmarksFolder_separator"
               class="hide-if-empty-places-result"
               builder="end"/>
<menuitem id="BMB_unsortedBookmarksFolder" ...

notice the added class too.

(In reply to comment #12)
> >             <menu id="BMB_bookmarksToolbarFolderMenu"
> 
> This patch also isn't implementing Alex's request to make the UI like the FF
> button (toolbar bookmarks inline, unless toolbar is shown).

And we should not do that. First because the menu won't be long enough to show everything, second because Places views have not been built to inline elements from different folders, I'd bet that we would break all the interaction. These are not just list of of elements, there is a lot of code behind for managing bookmarks.

Comment 15

[Marco Bonardo [:mak]]

ps: I hope we are not trying to inline toolbar elements in the appmenu button, did at least one Places peer review those changes?

Comment 16

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

Comment on attachment 475752 [details] [diff] [review]
patch

Keeping the view bookmarks toolbar command regardless of if it is shown, or removing the command entirely, is fine.  Main thing we need to land here is getting the subscribe functionality displayed.

Comment 17

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

>ps: I hope we are not trying to inline toolbar elements in the appmenu button,
>did at least one Places peer review those changes?

We haven't yet, but that is the intended behavior.  Some users might only have bookmarks available on their toolbar, and they will want to be able to access those if the toolbar isn't visible.

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> We haven't yet, but that is the intended behavior.  Some users might only have
> bookmarks available on their toolbar, and they will want to be able to access
> those if the toolbar isn't visible.

what we can do is what we do in bookmarks menu and button, have a folder pointing to toolbar's element.  What we can't do is putting toolbar elements with menu elements in the same menu.

Comment 19

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

>What we can't do is putting toolbar elements
>with menu elements in the same menu.

Why can we not do that and show the user a flat list?

Comment 20

[Frank Yan (:fryn)]

(In reply to comment #142)
> Also wonder if the RSS icon should be shown when the menuitem is active, ala
> the Windows AppButton ("#appmenu_subscribeToPage:not([disabled])" in
> winstripe's browser.css). Guess bug 592900 can figure that out too.

I see. I'll include that in the next iteration of the patch.

Comment 21

[Marco Bonardo [:mak]]

(In reply to comment #19)
> >What we can't do is putting toolbar elements
> >with menu elements in the same menu.
> 
> Why can we not do that and show the user a flat list?

because the code of the views that manages drag&drop and copy/paste handles a single root element per popup, to add two we should fix it in quite a bunch of places, and that's not something we want to do at this stage. Unless you want a read-only list, we can do that, but users won't be able to do any change to those bookmarks.

Comment 22

[Marco Bonardo [:mak]]

the simple relation is 1 popup <-> 1 bookmarks folder

Comment 23

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

Ok, we are going to want to follow up after Firefox 4 so that we can get both the contents of the bookmarks toolbar and the bookmarks menu displayed in a flat list in a single popup.  Until then we'll go with what we currently have (bookmarks toolbar in sub-menu).

Comment 24

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

This needs to land in beta7, as it's tied into the RSS removal issue.

Sad to hear that we can't get the Bookmarks Toolbar inlined here; I actually think read only would be fine, since it's only legacy items, but let's deal with that afterwards when I get all angsty about polish. ;)

Comment 25

[Frank Yan (:fryn)]

(In reply to comment #24)
> This needs to land in beta7, as it's tied into the RSS removal issue.

I'll post a patch by Thursday night. If you need it earlier, feel free to steal the bug :P

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #24)
> This needs to land in beta7, as it's tied into the RSS removal issue.
> 
> Sad to hear that we can't get the Bookmarks Toolbar inlined here; I actually
> think read only would be fine,

While we can absolutely do that, I don't think read-only is a nice solution, we have this awesome possibility to allow users to manage bookmarks as they see them, all around the browser ui. Dropping the feature makes us worse than others.

I think I'll steal the bug for now trying to have a patch by the end of today, I hope you don't mind.

Comment 27

[Marco Bonardo [:mak]]

Attachment: patch v2.0

this is based on previous comments discussion, and fixes bogus code shared with appmenu button that was pretty broken as well.

Comment 28

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

Dolske reminds/informs that this isn't as tied to the RSS removal as I thought. No longer blocking b7.

Comment 29

[Marco Bonardo [:mak]]

this patch has a couple string removals, are those problematic for the string freeze? IIRC removals should not be, but I don't recall exactly.

Comment 30

[Marco Bonardo [:mak]]

Attachment: patch v2.1

this is just an unbitrot on top of current addons bar patch (touching browser-places.js).

If that patch sticks this applies clean, otherwise v2.0 will apply.

Comment 31

[Axel Hecht [:Pike]]

betaN+ is fine for string removals.

Comment 33

[Justin Dolske [:Dolske]]

Comment on attachment 477911 [details] [diff] [review]
patch v2.1

>+++ b/browser/base/content/browser-places.js
...
>+_popups: {},

Nit: _popupState (since this doesn't hold the actual popups).

>   onPopupShowing: function BMB_onPopupShowing(event) {
>-    if (!this._popupNeedsUpdating)
>+    let popup = event.target;
>+
>+    if (!this._popups[popup.id])
>+      this._popups[popup.id] = { initialize: true, update: true };

Nit: { initialized: false, needsUpdate: true }

>+    if (!this._popups[popup.id].update)

Use a local |popupState| var, so you don't have to keep referring to "this._popups[popup.id]".

>   updatePosition: function BMB_updatePosition() {
>-    this._popupNeedsUpdating = true;
>+    for each (let popup in this._popups) {
>+      popup.update = true;
>+    }

Huh, kinda seems odd that this is the only place that was setting _popupNeedsUpdating to true.


>+++ b/browser/base/content/browser.xul
...
>+                <menu id="appmenu_bookmarksToolbar"
>+                      anonid="toolbar"

Using anonid this way feels like hidden magic. It would be more clear (and searchable!) if it was a unique, explicit attribute (maybe "placesType" or "builder"?).

Why no changes to the "Bookmarks toolbar" entry in browser-menubar.inc? Seems odd for that to be going through some other code path? (Seems to still work with this patch...)


>+++ b/browser/themes/gnomestripe/browser/browser.css
...
> #bookmarksToolbarFolderMenu,
>-#BMB_bookmarksToolbarFolderMenu {
>+#BMB_bookmarksToolbar {
>   list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png");  
> }
> 
> #unsortedBookmarksFolderMenu {
>   list-style-image: url("chrome://browser/skin/places/unsortedBookmarks.png");  
> }

Hmm, your winstripe/pinstripe changes are also changing the #BMB_unsortedBookmarksFolderMenu, but not here... Gnomestripe's CSS seems to be currently broken, as there is no #unsortedBookmarksFolderMenu ID in the tree. Go ahead and update that rule to reflect the new name, and fix it in the process?

Comment 34

[Marco Bonardo [:mak]]

(In reply to comment #33)

> >   updatePosition: function BMB_updatePosition() {
> >-    this._popupNeedsUpdating = true;
> >+    for each (let popup in this._popups) {
> >+      popup.update = true;
> >+    }
> 
> Huh, kinda seems odd that this is the only place that was setting
> _popupNeedsUpdating to true.

the popup needs updating only if the toolbar collapsed status changes or the bookmarks button is moved... both of those locations call updatePosition(). I'll add a comment.

> >+++ b/browser/base/content/browser.xul
> ...
> >+                <menu id="appmenu_bookmarksToolbar"
> >+                      anonid="toolbar"
> 
> Using anonid this way feels like hidden magic. It would be more clear (and
> searchable!) if it was a unique, explicit attribute (maybe "placesType" or
> "builder"?).

It is hidden magic! Btw, will rename it

> Why no changes to the "Bookmarks toolbar" entry in browser-menubar.inc? Seems
> odd for that to be going through some other code path? (Seems to still work
> with this patch...)

Bookmarks menu has its own dtd entry, I guess I can remove it and make it pass through this code. I did not do that because bookmarks toolbar entry would disappear from there when the toolbar is visibile if it passes through this code, and I seem to recall we put it there to allow keyboard access regardless from toolbar status... What do you think about this issue?

> >+++ b/browser/themes/gnomestripe/browser/browser.css
> ...
> > #bookmarksToolbarFolderMenu,
> >-#BMB_bookmarksToolbarFolderMenu {
> >+#BMB_bookmarksToolbar {
> >   list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png");  
> > }
> > 
> > #unsortedBookmarksFolderMenu {
> >   list-style-image: url("chrome://browser/skin/places/unsortedBookmarks.png");  
> > }
> 
> Hmm, your winstripe/pinstripe changes are also changing the
> #BMB_unsortedBookmarksFolderMenu, but not here... Gnomestripe's CSS seems to be
> currently broken, as there is no #unsortedBookmarksFolderMenu ID in the tree.
> Go ahead and update that rule to reflect the new name, and fix it in the
> process?

uh, will take a look, it's possible this has always been broken.

Comment 35

[Marco Bonardo [:mak]]

Also, I'm unsure about the removal of "View in sidebar" dtd entry because looks like we don't have the final design of the menus, the sidebar entries are not available from FX app button, and we added it to the bookmarks button for that reason. Now we are removing it, I fear we could want to reintroduce it in future (After string freeze) and there won't be any usable string for that.

Comment 36

[Marco Bonardo [:mak]]

Attachment: patch v2.2

Addresses comments from Dolske.

to sum up, 2 questions left, I guess mostly for Faaborg (but feel free to give opinions)

1. Should Bookmarks toolbar entry in old bookmarks menu disappear if personal toolbar is visible? If the original menu is intended for keyboard access that is probably a bad idea and I should not touch the original menu.

2. Should I remove the "Show in sidebar" localization entry? If we plan to reintroduce that string in any part of the UI after the freeze I should not remove it since there is no other string we can use for that.

Comment 37

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

1. No, leave it for keyboard access.
2. Keep the string for now.

Comment 38

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

Comment on attachment 479578 [details] [diff] [review]
patch v2.2

I agree with Mike's two answers

Comment 39

[Marco Bonardo [:mak]]

Attachment: patch v2.3

I did a small step back and used personalBar.cmd as title for Bookmarks Toolbar, as in the old menu, while it's not 100% correct due to the source of the string, this saves some work on the first popupshowing and it did not hurt so far. We could easily change that in future if the cmd label should change without touching strings.

for the name of the attribute I went for placesanonid, it's not related to roots or a type, nor I want it to be confused with builder. It's just an anonid, but this makes the code searchable (that I guess was the biggest concern)

also simplified the code and the popups status cache.

Comment 40

[Justin Dolske [:Dolske]]

(In reply to comment #34)

> > Why no changes to the "Bookmarks toolbar" entry in browser-menubar.inc?
> 
> Bookmarks menu has its own dtd entry, I guess I can remove it and make it pass
> through this code. I did not do that because bookmarks toolbar entry would
> disappear from there when the toolbar is visibile if it passes through this
> code, and I seem to recall we put it there to allow keyboard access regardless
> from toolbar status... What do you think about this issue?

Hmm, I'm not sure why I asked now. I think I was just confused, since this code is specifically for the BMB.

Comment 41

[Justin Dolske [:Dolske]]

Comment on attachment 479721 [details] [diff] [review]
patch v2.3

>+    if (!initialized) {
>+      let unsorted = getPlacesAnonymousElement("unsorted");
>+      if (unsorted)
>+        unsorted.label = PlacesUtils.getString("UnsortedBookmarksFolderTitle");
>+    }

Hmm, we could skip this too if the XUL just set the label by default. Doesn't seem to be any reason this needs to be set via JS... Just leave (and use) the existing appMenuUnsorted.label entity, and I guess add a new one for the BMB? Guess we need to do that before B7 after all.

r+ with that!

Comment 42

[Justin Dolske [:Dolske]]

Marking B7 again for string addition, and I suppose we should take for for B7 too to avoid post-B7 churn for extensions expecting menu order stability.

Comment 43

[Marco Bonardo [:mak]]

hm, you know what, I can also set toolbarId="PersonalToolbar" in xul and completely avoid initialization. I did not want to replicate the same string but at this point it is convenient.

Comment 44

[Marco Bonardo [:mak]]

Attachment: patch v2.4

- use .dtd strings, added bookmarksMenuButton.unsorted.label
- I've renamed the local vars "toolbar" and "viewToolbar" to "toolbarMenuitem" and "viewToolbarMenuitem", because seeing toolbar.collapsed was confusing.
- set toolbarId in xul thanks to latest changes in browser code, killed all popup initialization js code
- added a currentTarget check so that we handle only the popupshowing for the main popup, not submenus

ready to land.

Comment 45

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/9a00cdcdb869