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

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: faaborg, Assigned: mak)

Tracking

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings])

Attachments

(1 attachment, 5 obsolete attachments)

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)
(Reporter)

Comment 1

7 years ago
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)
blocking2.0: --- → ?
(Reporter)

Comment 2

7 years ago
related bug 592908 for updating the text in the Firefox menu
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?
(Reporter)

Comment 4

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

Comment 5

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

What's the triage or ETA status here?
(Reporter)

Updated

7 years ago
Blocks: 578967
(Reporter)

Comment 6

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

Comment 7

7 years ago
(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.
(Reporter)

Comment 8

7 years ago
>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.
Whiteboard: [strings]

Comment 9

7 years ago
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

7 years ago
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

7 years ago
Created attachment 475752 [details] [diff] [review]
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.
Assignee: nobody → fryn
Status: NEW → ASSIGNED
Attachment #475752 - Flags: ui-review?(faaborg)
Attachment #475752 - Flags: review?(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?
Attachment #475752 - Flags: review?(dolske) → review-

Comment 13

7 years ago
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.
Attachment #475752 - Flags: ui-review?(faaborg)
(Assignee)

Comment 14

7 years ago
>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.
(Assignee)

Comment 15

7 years ago
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 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.
Attachment #475752 - Flags: ui-review+
>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.
(Assignee)

Comment 18

7 years ago
(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.
>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?

Updated

7 years ago
No longer blocks: 578967
Depends on: 578967

Comment 20

7 years ago
(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.
Version: unspecified → Trunk
(Assignee)

Comment 21

7 years ago
(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.
(Assignee)

Comment 22

7 years ago
the simple relation is 1 popup <-> 1 bookmarks folder
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).
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. ;)
blocking2.0: ? → beta7+
Whiteboard: [can land]

Comment 25

7 years ago
(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
(Assignee)

Comment 26

7 years ago
(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.
Assignee: fryn → mak77
(Assignee)

Comment 27

7 years ago
Created attachment 477518 [details] [diff] [review]
patch v2.0

this is based on previous comments discussion, and fixes bogus code shared with appmenu button that was pretty broken as well.
Attachment #475752 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #477518 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Whiteboard: [can land] → [needs review]
Dolske reminds/informs that this isn't as tied to the RSS removal as I thought. No longer blocking b7.
blocking2.0: beta7+ → betaN+
(Assignee)

Updated

7 years ago
Blocks: 589155
(Assignee)

Comment 29

7 years ago
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.
(Assignee)

Comment 30

7 years ago
Created attachment 477911 [details] [diff] [review]
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.
Attachment #477518 - Attachment is obsolete: true
Attachment #477911 - Flags: review?(dolske)
Attachment #477518 - Flags: review?(dolske)

Comment 31

7 years ago
betaN+ is fine for string removals.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 599857
(Assignee)

Updated

7 years ago
Blocks: 575235
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?
Attachment #477911 - Flags: review?(dolske) → review-
(Assignee)

Comment 34

7 years ago
(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.
(Assignee)

Comment 35

7 years ago
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.
(Assignee)

Comment 36

7 years ago
Created attachment 479578 [details] [diff] [review]
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.
Attachment #477911 - Attachment is obsolete: true
Attachment #479578 - Flags: feedback?(faaborg)
1. No, leave it for keyboard access.
2. Keep the string for now.
Comment on attachment 479578 [details] [diff] [review]
patch v2.2

I agree with Mike's two answers
Attachment #479578 - Flags: feedback?(faaborg)
(Assignee)

Comment 39

7 years ago
Created attachment 479721 [details] [diff] [review]
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.
Attachment #479578 - Attachment is obsolete: true
Attachment #479721 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Blocks: 589126
(Assignee)

Updated

7 years ago
Blocks: 589140
(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 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!
Attachment #479721 - Flags: review?(dolske) → review+
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.
blocking2.0: betaN+ → beta7+
(Assignee)

Comment 43

7 years ago
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.
(Assignee)

Updated

7 years ago
Whiteboard: [needs review] → [needs review][strings]
(Assignee)

Comment 44

7 years ago
Created attachment 481175 [details] [diff] [review]
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.
Attachment #479721 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [needs review][strings] → [strings]
(Assignee)

Comment 45

7 years ago
http://hg.mozilla.org/mozilla-central/rev/9a00cdcdb869
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7

Updated

7 years ago
Depends on: 603605
You need to log in before you can comment on or make changes to this bug.