192624 - Empty bookmark folders should show '(Empty)'

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Pham]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030209 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030209 Phoenix/0.5

I empty all the bookmarks from what you give me and when I go over the
folder(Bookmarks.Bookmarks Toolbar Folder.) it gives out a tiny squished square.
I remember one of the other folders, called Mozilla and Phoenix Information,
whenever I highlighted it, it was a transparent "(empty ***)

Reproducible: Always

Steps to Reproduce:
1. Go to Bookmarks in the top toolbar.
2. Highlight an empty folder(bookmark toolbar folder)
3.

Actual Results:  
See a squished square

Expected Results:  
Said Empty with ghostly text, or a normal box.

Comment 1

[Pham]

Attachment: Bookmarks screenshot

Comment 2

[David Tenser [:djst]]

Confirming. I've seen this problem for a long time now, but I thought it was
already reported.

(Note: If this is a Mozilla bug, this is most certainly a duplicate. Is the
Bookmark service forked?)

Comment 3

[Pierre Chanial]

David: phoenix and mozilla have very little in common with respect of bookmarks.
the best would be not to dupe against mozilla bugs.

Btw, this bug is likely a duplicate of bug 192624: the bookmark toolbar is horked.

Comment 4

[David Tenser [:djst]]

> Btw, this bug is likely a duplicate of bug 192624: the bookmark toolbar is horked.

THIS is 192624! :)

Comment 5

[Pierre Chanial]

ooopps! :)

*** This bug has been marked as a duplicate of 191637 ***

Comment 6

[David Tenser [:djst]]

Slap me if I'm wrong, but this is NOT a dupe of 191637. That bug was some sort
of a regressen that happened at the same time as when the new Options window was
checked in by Ben.

This bug is about the fact that an empty sub-folder in the Bookmarks menu should
display something like "(Empty)" (disabled) when popping up, and not just a tiny
square. 

This bug has been there ever since Phoenix 0.1. Bug 191637 is very recent.
Reopening.

Comment 7

[Pierre Chanial]

re-ooops... Thanks!
updating summary

Comment 8

[Chu Alan "sprite"]

Attachment: patch with hardcoded string

i hope you have not worked on this bug yet :)

P.S. how to make the (Empty) string localizable? use something like
&empty.label; in setAttribute()? does it work?

Comment 9

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 121176 [details] [diff] [review]
patch with hardcoded string

>+  /////////////////////////////////////////////////////////////////////////////
>+  // show an empty item if the menu is empty
>+  showEmptyItem: function (aTarget)
>+  {
>+    if(aTarget.hasChildNodes())
>+      return;
>+
>+    var emptyElement = document.createElementNS(XUL_NS, "menuitem");
>+    emptyElement.setAttribute("id", "empty-menuitem");
>+    emptyElement.setAttribute("label", "(Empty)");
>+    emptyElement.setAttribute("disabled", "true");
>+
>+    aTarget.appendChild(emptyElement);
>+  },

needs to be localized

http://lxr.mozilla.org/mozilla/source/browser/base/locale/browser.properties
would be the right file.  Create say, emptyDirectory in that .properties file,
then you can use the following code snippet to get this working.

var gNavigatorBundle = document.getElementById("bundle_browser");
var EmptyMsg = gNavigatorBundle.getString("emptyDirectory");

then use EmptyMsg in place of the hardcoded string:
 emptyElement.setAttribute("label", EmptyMsg);

haven't looked at the rest of the patch yet, but this needs to be fixed before
formal review.

Comment 11

[Andrew]

Attachment: adds localization and updates patch

This localizes Chu Alan "sprite"'s patch as well as updates it to the current
trunk.

Comment 12

[Mike Connor [:mconnor]]

*** Bug 224982 has been marked as a duplicate of this bug. ***

Comment 13

[Mike Connor [:mconnor]]

*** Bug 232005 has been marked as a duplicate of this bug. ***

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 132285 [details] [diff] [review]
adds localization and updates patch 

>diff -u -r1.12 bookmarks.properties
>--- components/bookmarks/locale/bookmarks.properties	6 Aug 2003 09:14:03 -0000	1.12
>+++ components/bookmarks/locale/bookmarks.properties	28 Sep 2003 03:29:34 -0000
>@@ -104,3 +104,4 @@
> 
> search_button_label = Find
> 
>+emptyDirectory = Empty


shouldn't have the extra space there, string should be (Empty)

Comment 15

[benc]

This happens to me in Mozilla 1.7b, and Netscape 7.1 (in other words, everything...)

Comment 16

[Ali Ebrahim]

This is a minor polish bug. It certainly shouldn't block Firefox 0.9 which is a
feature-driven release.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

+ since there is a patch

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 132285 [details] [diff] [review]
adds localization and updates patch 

I need to review this at some point

Comment 19

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

Note that when you reload ffox, you correctly get the "Empty" item from the
appropriate bookmarks-template rule.  The template builder doesn't switch which
rule it applies once the container transitions from isempty=false ->
isempty=true, or the other way around -- if you reload ffox so that you get the
"Empty" menu item, and then add a bookmark to that folder, you'll get "Empty"
followed by the bookmark.

Comment 20

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

I'd suggest also removing the isempty="true" rules from bookmarks-template in
browser-menubar.inc, so that the "Empty" menu item gets inserted in one place
only (showEmptyItem).  (Fixing this issue in the template builder ended up being
very non-trivial, so the patch here is probably the best approach, IMO.)

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 132285 [details] [diff] [review]
adds localization and updates patch 

minusing based on vlad's comments.  Vlad, I expect after favicons, this one is
easier? :)

Comment 22

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

Attachment: bookmarks-empty-folders-1.patch

Updated version of Andrew's patch, basically changes the item to (Empty) and
removes the special-handling for empty elements from the template builder. 
(This means we can also get rid of the livemark special-handling, since they
just get treated like normal containers.)

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 152667 [details] [diff] [review]
bookmarks-empty-folders-1.patch

cleanup nit:

emptyItem.label can be removed from
http://lxr.mozilla.org/aviarybranch/source/browser/locales/en-US/chrome/browser
/bookmarks/bookmarks.dtd#95  and
http://lxr.mozilla.org/aviarybranch/source/browser/locales/en-US/chrome/browser
/browser.dtd#72 since you're removing the only two references to that (and I
don't know why its in both files, but neither is needed, so please remove both)

also, why the newline at the end of bookmarks.properties?

with those nits addressed, r=[e-mail]

Comment 24

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

in on aviary, in my queue for trunk.

Comment 25

[Andrew]

You should try hiding the menu item in the onDrop function the same way that the
'open in tabs' menu item is hidden. This results in a nice deletion of the
'(Empty)' item as soon as a bookmark is added to the folder. 

Comment 26

[Ryan Polk (Quark)]

setting fixed-aviary1.0 for bugfixes checked into branch

Comment 27

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change