Last Comment Bug 385266 - New starring, bookmarking and tagging UI
: New starring, bookmarking and tagging UI
Status: VERIFIED FIXED
[places-ui]
: uiwanted
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P2 normal with 3 votes (vote)
: Firefox 3 alpha8
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
: Marco Bonardo [::mak]
Mentors:
http://wiki.mozilla.org/Places:User_I...
: 434694 (view as bug list)
Depends on: 387485 393239 393257 441326 385269 385272 385275 385535 385536 385609 385616 387486 388695 390197 391428 392389 392397 392427 392443 392512 392580 392820 393253 393398 393446 393546 393710 393887 394085 394088 394089 394159 394162 394285 394450 394605 396513 398011 398409 404122 405010 413483 435179
Blocks: 329261 358210 374524 387642 387749 389252 392976 393509
  Show dependency treegraph
 
Reported: 2007-06-20 17:49 PDT by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2010-02-24 06:22 PST (History)
34 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
checkpoint (45.88 KB, patch)
2007-06-23 05:59 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
Starring and Tagging i7 (134.82 KB, image/png)
2007-07-12 22:05 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
checkpoint #2 (44.93 KB, patch)
2007-07-17 12:15 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
checkpoint #2.1 (46.52 KB, patch)
2007-07-18 15:45 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
more... (45.36 KB, patch)
2007-07-22 18:40 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
something to land, v1 (81.21 KB, patch)
2007-08-14 15:52 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
dietrich: review-
Details | Diff | Splinter Review
more... (84.96 KB, patch)
2007-08-15 09:24 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
dietrich: review+
Details | Diff | Splinter Review
Temporary Starred icon (3.19 KB, image/png)
2007-08-15 16:48 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
Temporary Un-starred icon (3.00 KB, image/png)
2007-08-15 16:49 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
part 1, for checkin (86.52 KB, patch)
2007-08-15 18:01 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
New star implementation, star is stretched (5.72 KB, image/png)
2007-08-16 10:31 PDT, Jesse Maloney (jtmal0723 [at] gmail for email)
no flags Details
more... (24.48 KB, patch)
2007-08-16 15:28 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
dietrich: review+
Details | Diff | Splinter Review
more, wip (38.64 KB, patch)
2007-08-19 06:50 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
another round (43.25 KB, patch)
2007-08-19 14:57 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
dietrich: review+
Details | Diff | Splinter Review
comment 33 (25.94 KB, patch)
2007-08-27 16:24 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
dietrich: review+
Details | Diff | Splinter Review
comment 33, for checkin (25.94 KB, patch)
2007-08-28 14:46 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-06-20 17:49:56 PDT
 
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-06-23 05:59:38 PDT
Created attachment 269509 [details] [diff] [review]
checkpoint
Comment 2 Alex Faaborg [:faaborg] (Firefox UX) 2007-07-12 22:05:08 PDT
Created attachment 272128 [details]
Starring and Tagging i7

This is iteration 7 of the starring and tagging UI, mostly consisting of some visual tweaks to simplify the interface.  Also, the checkbox next to "place in folder" from i6 has now been removed.

Here are the notes in the mockup:

This dialog is displayed when the user double clicks on the star, or clicks a second time.  This dialog also replaces the new bookmarks dialog.

When nothing is entered in the tag field box the text "tag one, tag two, tag three," appears in a grey font.

The default location is based on how the UI was displayed:

Double click on star:  All Bookmarks

Bookmarks Menu>Bookmark this Page: Bookmarks Menu

Accel-D: Bookmarks Menu

The expanded list of all tags is sorted alphabetically.
Comment 3 Ivan Ičin 2007-07-13 09:46:34 PDT
I think that "All Bookmarks" and "Bookmarks menu" would be confusing for new users. Two suggestions:

1) Starred pages (e.g. like in Picasa) and Bookmarks menu. And Other starred pages for non-menu/toolbar items.

2) All Bookmarks and Classic Bookmarks menu and Classic Bookmarks toolbar.
Comment 4 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-07-17 12:15:24 PDT
Created attachment 272679 [details] [diff] [review]
checkpoint #2
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-07-18 15:45:38 PDT
Created attachment 272875 [details] [diff] [review]
checkpoint #2.1

Applies after ndef-MOZ_PLACES_BOOKMARKS removal.
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-07-22 18:40:51 PDT
Created attachment 273355 [details] [diff] [review]
more...
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-14 15:52:25 PDT
Created attachment 276709 [details] [diff] [review]
something to land, v1

there's _a lot_ more to do once this lands
Comment 8 Dietrich Ayala (:dietrich) 2007-08-15 02:05:03 PDT
Comment on attachment 276709 [details] [diff] [review]
something to land, v1

>+  // nsIDOMEventListener
>+  handleEvent: function PSB_handleEvent(aEvent) {
>+    if (aEvent.originalTarget != this.panel)
>+      return;
>+
>+    // This only happens for auto-hide. When the panel is closed from within
>+    // itself, doneCallback removes the listener and only then hides the popup
>+    gAddBookmarksPanel.saveItem();
>+    gAddBookmarksPanel.uninitPanel();

in the auto-hide case, do you need to remove the listener here?

>+  },
>+
>+  showBookmarkPagePopup: function PSB_showBookmarkPagePopup(aURL) {
>+    const bms = PlacesUtils.bookmarks;
>+
>+    var starIcon = document.getElementById("star-icon");
>+    var panel = this.panel;
>+    panel.showPopup(starIcon, -1, -1, "popup", "bottomright", "topright");
>+
>+    var itemId = -1;
>+    var bmkIds = bms.getBookmarkIdsForURI(this._url, {});
>+    for each (var bk in bmkIds) {
>+      // Find the first folder which isn't a tag container
>+      var folder = bms.getFolderIdForItem(bk);
>+      if (folder == PlacesUtils.placesRoot ||
>+          bms.getFolderIdForItem(folder) != PlacesUtils.tagRootId) {
>+        itemId = bk;
>+        break;
>+      }
>+    }
>+    if (itemId == -1) {
>+      // if the remaining items for this url are under tag containers,
>+      // star the page first
>+      itemId = this._star();
>+    }

for clarification, is this correct?

1. items are starred if they're not already bookmarked
2. bookmarked items are considered starred
3. starred but not filed items are stored in the places root

>+  // nsINavBookmarkObserver  
>+  onItemAdded: function(aItemId, aFolder, aIndex) {
>+    this.updateState(this._url);
>+  },
>+
>+  onItemRemoved: function(aItemId, aFolder, aIndex) {
>+    this.updateState(this._url);
>+  },
>+
>+  onItemChanged: function(aItemId, aProperty, aIsAnnotationProperty, aValue) {
>+    if (aProperty == "uri")
>+      this.updateState(this._url);
>+  },

these 3 preceding methods could all be called when some random extension starts doing it's
bookmark import, etc. maybe should compare against this._url first?

>+  // nsIMicrosummaryObserver
>+  onContentLoaded: function ABP_onContentLoaded(aMicrosummary) {
>+    var namePicker = this._element("editBMPanel_namePicker");
>+    var childNodes = namePicker.menupopup.childNodes;
>+
>+    // 0: user-entered item; 1: separator
>+    for (var i = 2; i < childNodes.length; i++) {
>+      if (childNodes[i].microsummary == aMicrosummary) {
>+        var newLabel = aMicrosummary.content;
>+        // XXXmano: non-editable menulist would do this for us, see bug 360220
>+        // We should fix editable-menulsits to set the DOMAttrModified handler
>+        // as well.

s/menulsits/menulists/

>+        //
>+        // Also note the order importance: if the label of the menu-item is
>+        // set the something different than the menulist's current value,

s/the/to/

>+      if (tagsToAdd.length > 0)
>+        PlacesUtils.tagging.tagURI(this._uri, tagsToAdd, tagsToAdd.length);
>+      if (tagsToRemove.length > 0)
>+        PlacesUtils.tagging.untagURI(this._uri, tagsToRemove, tagsToRemove.length);

no longer need the length parameters

>+    }
>+
>+    if (txns.length > 0)
>+      PlacesUtils.ptm.commitTransaction(PlacesUtils.ptm.aggregateTransactions("", txns));

why no aggregate txn name?

up to here, will continue in the AM.
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-15 05:58:45 PDT
(In reply to comment #8)
> (From update of attachment 276709 [details] [diff] [review])
> >+  // nsIDOMEventListener
> >+  handleEvent: function PSB_handleEvent(aEvent) {
> >+    if (aEvent.originalTarget != this.panel)
> >+      return;
> >+
> >+    // This only happens for auto-hide. When the panel is closed from within
> >+    // itself, doneCallback removes the listener and only then hides the popup
> >+    gAddBookmarksPanel.saveItem();
> >+    gAddBookmarksPanel.uninitPanel();
> 
> in the auto-hide case, do you need to remove the listener here?

Not for any good reason I could think of.

> >+  },
> >+
> >+  showBookmarkPagePopup: function PSB_showBookmarkPagePopup(aURL) {
> >+    const bms = PlacesUtils.bookmarks;
> >+
> >+    var starIcon = document.getElementById("star-icon");
> >+    var panel = this.panel;
> >+    panel.showPopup(starIcon, -1, -1, "popup", "bottomright", "topright");
> >+
> >+    var itemId = -1;
> >+    var bmkIds = bms.getBookmarkIdsForURI(this._url, {});
> >+    for each (var bk in bmkIds) {
> >+      // Find the first folder which isn't a tag container
> >+      var folder = bms.getFolderIdForItem(bk);
> >+      if (folder == PlacesUtils.placesRoot ||
> >+          bms.getFolderIdForItem(folder) != PlacesUtils.tagRootId) {
> >+        itemId = bk;
> >+        break;
> >+      }
> >+    }
> >+    if (itemId == -1) {
> >+      // if the remaining items for this url are under tag containers,
> >+      // star the page first
> >+      itemId = this._star();
> >+    }
> 
> for clarification, is this correct?
> 
> 1. items are starred if they're not already bookmarked
> 2. bookmarked items are considered starred
> 3. starred but not filed items are stored in the places root

right.

> >+  // nsINavBookmarkObserver  
> >+  onItemAdded: function(aItemId, aFolder, aIndex) {
> >+    this.updateState(this._url);
> >+  },
> >+
> >+  onItemRemoved: function(aItemId, aFolder, aIndex) {
> >+    this.updateState(this._url);
> >+  },
> >+
> >+  onItemChanged: function(aItemId, aProperty, aIsAnnotationProperty, aValue) {
> >+    if (aProperty == "uri")
> >+      this.updateState(this._url);
> >+  },
> 
> these 3 preceding methods could all be called when some random extension starts
> doing it's
> bookmark import, etc. maybe should compare against this._url first?
> 

hrm, onItemAdded/Removed don't take a uri parameter... I could optimize this listener for the batching case though.
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-15 09:24:09 PDT
Created attachment 276789 [details] [diff] [review]
more...
Comment 11 Dietrich Ayala (:dietrich) 2007-08-15 13:59:41 PDT
Comment on attachment 276789 [details] [diff] [review]
more...


some problems i noticed:

- folder popup appears behind the panel sometimes
- after autohide, the panel is sometimes partially visible behind the toolbar
- some of the back-end interaction is out of sync somehow: i saw some "removing item that doesn't exist" errors. don't know if this is tag or bookmark related.

however, all the actions that i tested worked ok: bookmarks added and removed, tags, descriptions, etc were all added and removed properly, etc.

clearly more work to do, but r=me given fixes for the comments inline.

can you please follow up by filing bugs for major known items that could be spun off for others could work on, or at the least list them here to minimize the level of dupes filed.

>@@ -1363,18 +1373,21 @@ var PlacesUtils = {
>     var annosvc = this.annotations;
>     aAnnos.forEach(function(anno) {
>       if (anno.type == annosvc.TYPE_BINARY) {
>         annosvc.setItemAnnotationBinary(aItemId, anno.name, anno.value,
>                                         anno.value.length, anno.mimeType,
>                                         anno.flags, anno.expires);
>       }
>       else {
>+        var flags = ("flags" in anno) ? anno.flags : 0;
>+        var expires = ("expires" in anno) ?
>+          anno.expires : Ci.nsIAnnotationService.EXPIRE_NEVER
>         annosvc.setItemAnnotation(aItemId, anno.name, anno.value,
>-                                  anno.flags, anno.expires);
>+                                  flags, expires);
>       }
>     });
>   },
> 

is this change also needed for setAnnotationsForURI?

> 
>   // nsITaggingService
>-  untagURI: function TS_untagURI(aURI, aTags, aCount) {
>+  untagURI: function TS_untagURI(aURI, aTags) {
>     if (!aURI)
>       throw Cr.NS_ERROR_INVALID_ARG;
> 
>+    if (!aTags) {
>+      // see IDL.
>+      // XXXmano: write a perf-sensitive version of this code path...
>+      var tags = this.getTagsForURI(aURI);
>+      if (tags.length > 0)
>+        this.untagURI(aURI, tags);
>+      return;

couldn't you set aTags = this.getTagsForURI(), return early if the uri has zero tags, and continue otherwise? that wouldn't be more performant, but might be less weird.

> 
>   // nsITaggingService
>   getTagsForURI: function TS_getTagsForURI(aURI) {
>     if (!aURI)
>       throw Cr.NS_ERROR_INVALID_ARG;
> 
>     var tags = [];
>-
>     var bookmarkIds = this._bms.getBookmarkIdsForURI(aURI, {});
>+    var root = this._tagsResult.root;
>+    var cc = root.childCount;
>     for (var i=0; i < bookmarkIds.length; i++) {
>       var parent = this._bms.getFolderIdForItem(bookmarkIds[i]);
>-      for (var j=0; j < this._tags.length; j++) {
>-        if (this._tags[j].itemId == parent)
>-          tags.push(this._tags[j].name);
>+      for (var j=0; j < cc; j++) {
>+        var child = root.getChild(j);
>+        if (child.itemId == parent)
>+          tags.push(child.title);
>       }
>     }

seems like this could be optimized by caching the tag folder ids the first time through, minimizing the xpconnect boundary crossings.
Comment 12 Alex Faaborg [:faaborg] (Firefox UX) 2007-08-15 16:48:20 PDT
Created attachment 276862 [details]
Temporary Starred icon
Comment 13 Alex Faaborg [:faaborg] (Firefox UX) 2007-08-15 16:49:05 PDT
Created attachment 276864 [details]
Temporary Un-starred icon
Comment 14 Alex Faaborg [:faaborg] (Firefox UX) 2007-08-15 16:54:08 PDT
Mano: here are some temporary icons to use
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-15 18:01:00 PDT
Created attachment 276881 [details] [diff] [review]
part 1, for checkin
Comment 16 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-15 18:20:51 PDT
mozilla/browser/base/content/browser-places.js 1.38
mozilla/browser/base/content/browser.js 1.821
mozilla/browser/base/content/browser.xul 1.355
mozilla/browser/components/places/jar.mn 1.38
mozilla/browser/components/places/content/editBookmarkOverlay.js initial revision: 1.1
mozilla/browser/components/places/content/editBookmarkOverlay.xul 
initial revision: 1.1
mozilla/browser/components/places/content/treeView.js 1.14
mozilla/browser/components/places/content/utils.js 1.55
mozilla/browser/locales/jar.mn 1.69
mozilla/browser/locales/en-US/chrome/browser/places/editBookmarkOverlay.dtd initial revision: 1.1
mozilla/browser/themes/pinstripe/browser/browser.css 1.62
mozilla/browser/themes/pinstripe/browser/jar.mn 1.45
mozilla/browser/themes/pinstripe/browser/places/editBookmarkOverlay.css initial revision: 1.1
mozilla/browser/themes/pinstripe/browser/places/pageStarred.png
initial revision: 1.1
mozilla/browser/themes/pinstripe/browser/places/starPage.png 
initial revision: 1.1
mozilla/browser/themes/winstripe/browser/browser.css 1.73
mozilla/browser/themes/winstripe/browser/jar.mn 1.40
mozilla/browser/themes/winstripe/browser/places/editBookmarkOverlay.css 
initial revision: 1.1
mozilla/browser/themes/winstripe/browser/places/pageStarred.png 
initial revision: 1.1
mozilla/browser/themes/winstripe/browser/places/starPage.png 
initial revision: 1.1
mozilla/toolkit/components/places/public/nsITaggingService.idl 1.3
mozilla/toolkit/components/places/src/nsTaggingService.js 1.3
mozilla/toolkit/components/places/tests/unit/test_tagging.js 1.4
Comment 17 Jesse Maloney (jtmal0723 [at] gmail for email) 2007-08-16 10:31:23 PDT
Created attachment 276973 [details]
New star implementation, star is stretched

With the new star implementation, the icon is stretched

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081605 Minefield/3.0a8pre ID:2007081605
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-16 15:28:30 PDT
Created attachment 277022 [details] [diff] [review]
more...
Comment 19 Dietrich Ayala (:dietrich) 2007-08-16 19:07:18 PDT
(In reply to comment #18)
> Created an attachment (id=277022) [details]
> more...
> 

testing this patch on osx, when bookmarking via "bookmark this page..." or "bookmark this link...", the popup loads in the top-left corner of the viewport.
Comment 20 Dietrich Ayala (:dietrich) 2007-08-17 00:04:46 PDT
Comment on attachment 277022 [details] [diff] [review]
more...

everything looks good except the placement issue. r=me otherwise.
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-17 00:07:07 PDT
[10:02]	<Mano>	dietrich: for "boomark this link/frame" that is by design
[10:03]	<dietrich>	really?
[10:03]	<Mano>	dietrich: yeah, we don't want to attach the popup to the location bar when it bookmarks something else
[10:04]	<Mano>	might change, but this is what mconnor and I came up with, for now.
[10:04]	<dietrich>	centered like the old dialog would be preferable to the corner, i think
[10:04]	<dietrich>	ok, tweakable later
[10:04]	<Mano>	dietrich: well, it's border-less so
[10:05]	<Mano>	dietrich: i will fix context-menu bookmark-this-page later
[10:05]	<Mano>	dietrich: we also use the content area when the location bar isn't visible
Comment 22 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-17 00:24:25 PDT
mozilla/browser/base/content/browser-places.js 1.41
mozilla/browser/base/content/browser-sets.inc 1.100
mozilla/browser/base/content/browser.js 1.822
mozilla/browser/base/content/browser.xul 1.358
mozilla/browser/base/content/nsContextMenu.js 1.22
mozilla/browser/components/places/content/utils.js 1.56
mozilla/browser/themes/pinstripe/browser/browser.css 1.63
mozilla/browser/themes/winstripe/browser/browser.css 1.74
Comment 23 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-19 06:50:41 PDT
Created attachment 277280 [details] [diff] [review]
more, wip

 - changes to the item being edited are are applied as they're done.
 - restore microsummaries support
 - various fixes to make the overlay reusable by the organizer.
 - partial work to make the overlay code item-type-agnostic.
Comment 24 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-19 14:57:22 PDT
Created attachment 277317 [details] [diff] [review]
another round

see comment 23.
Comment 25 Alfred Kayser 2007-08-20 02:15:34 PDT
When using 'Bookmark This Link', we popup as buttons 'Delete' and 'Ok'. As the bookmark is being created with the panel, the 'Delete' is confusing. Normal behaviour is then to 'Cancel' the action. 
So, can we when this dialog is called from Bookmark This Link from the context-menu on a page, have the normal buttons: Cancel and OK?
Comment 26 Alfred Kayser 2007-08-20 06:27:09 PDT
More issues:
the two buttons don't have an ID, so one cannot style them (giving them each specific button icon).
The panel has no title, so it can be confusing what it is really doing...
Furthermore the panel has fixed width, so with long titles, it becomes awkward.
Name and folder labels have a ':', but Keywords is missing it.
Tooltips are completely missing.
In the UI mockup (see http://wiki.mozilla.org/Places:User_Interface/I6
there is an text describing that keywords need ','. This is also missing from the panel.

Question: is there some attribute set to the 'star-icon' item, when the panel is shown? I want to restyle the icon when the panel is open.
Comment 27 Dietrich Ayala (:dietrich) 2007-08-20 16:07:43 PDT
Comment on attachment 277317 [details] [diff] [review]
another round

>   /**
>    * Initialize the panel
>    */
>-  initPanel: function ABP_initPanel(aItemId, aTm, aDoneCallback, aInfo) {
>+  initPanel: function EIO_initPanel(aItemId, aInfo) {
>     this._folderMenuList = this._element("folderMenuList");
>     this._folderTree = this._element("folderTree");
>-    this._tm = aTm;
>     this._itemId = aItemId;
>-    this._uri = PlacesUtils.bookmarks.getBookmarkURI(this._itemId);
>-    this._doneCallback = aDoneCallback;
>+    this._itemType = PlacesUtils.bookmarks.getItemType(this._itemId);
>     this._determineInfo(aInfo);
> 
>+    if (this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK) {
>+      this._uri = PlacesUtils.bookmarks.getBookmarkURI(this._itemId);
>+      // tags field
>+      this._element("tagsField").value =
>+        PlacesUtils.tagging.getTagsForURI(this._uri).join(", ");
>+
>+      this._element("locationField").value = this._uri.spec;
>+    }
>+
>     // folder picker
>     this._initFolderMenuList();
>-    
>+
>     // name picker
>     this._initNamePicker();
>-
>-    // tags field
>-    this._element("tagsField").value = this._currentTags.join(", ");
>-
>+    

nit: whitespace

>-    if (txns.length > 0) {
>-      // Mark the containing folder as recently-used if it isn't the
>-      // "All Bookmarks" root
>-      if (container != PlacesUtils.placesRootId)
>-        this._markFolderAsRecentlyUsed(container);
>+  onNamePickerInput: function EIO_onNamePickerInput() {
>+    var title = this._element("namePicker").value;
>+    this._element("userEnteredName").label = title;
>+  },
>+
>+  onNamePickerChange: function EIO_onNamePickerBlur() {

s/Blur/Change/

>-  toggleFolderTreeVisibility: function ABP_toggleFolderTreeVisibility() {
>+  onLocationFieldBlur: function EIO_onLocationFieldBlur() {
>+    // XXXmano: uri fixup
>+    var uri;
>+    try {
>+      uri = IO.newURI(this._element("locationField").value);
>+    }
>+    catch(ex) { return; }

hrm, currently the bookmark is not able to be saved w/o a valid URI.

in this scenario we should at least give feedback to the user that this edit did not actually get saved. followup is fine.

>-    this._folderTree.selectFolders([this._getFolderIdFromMenuList()]);
>+    // Update folder-tree selection
>+    if (isElementVisible(this._folderTree)) {
>+      var selectedNode = this._folderTree.selectedNode;
>+      if (selectedNode.itemId != container)
>+        this._folderTree.selectFolders(container);

selectFolders takes an array

>+  // nsINavBookmarkObserver
>+  onItemChanged: function EIO_onItemChanged(aItemId, aProperty,
>+                                            aIsAnnotationProperty, aValue) {
>+    if (this._itemId != aItemId)
>+      return;
>+
>+    switch (aProperty) {
>+    case "title":
>+      if (PlacesUtils.annotations.itemHasAnnotation(this._itemId,
>+                                                    STATIC_TITLE_ANNO))
>+        return;  // onContentLoaded updates microsummary-items
>+
>+      var userEnteredNameField = this._element("userEnteredName");
>+      if (userEnteredNameField.value != aValue) {
>+          userEnteredNameField.value = aValue;
>+        var namePicker = this._element("namePicker");
>+        if (namePicker.selectedItem == userEnteredNameField)
>+            namePicker.label = aValue;

nit: some messed up indenting on a couple of lines here

>+      }
>+      break;
>+    case "uri":
>+      var locationField = this._element("locationField");
>+      if (locationField.value != aValue) {
>+        locationField.value = aValue;
>+        this._uri = IO.newURI(aValue);
>+        this._initNamePicker(); // for microsummaries
>+        this._element("tagsField").value =
>+          PlacesUtils.tagging.getTagsForURI(this._uri).join(", ");
>+        this._rebuildTagsSelectorList();
>+      }
>+      break;
>+    case DESCRIPTION_ANNO:
>+      this._element("descriptionField").value =
>+        PlacesUtils.annotations.getItemDescription(this._itemId);
>+      break;
>+    }
>+  },

what's the use-case for this? the user leaves the panel open, and then makes edits via the organizer, or extensions changing bookmark properties?

i'm mildly concerned about a user making some edits, and then a sync-like extension runs in the background and overwrites the local edits as the user is making them.


r=me given the changes above.
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-22 07:30:50 PDT
> what's the use-case for this? the user leaves the panel open, and then makes
> edits via the organizer, or extensions changing bookmark properties?

the user leaves the the item-detail pane in the organizer open and changes the bookmark through another panel/dialog.
Comment 29 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-22 07:44:00 PDT
mozilla/browser/base/content/browser-places.js 1.42
mozilla/browser/base/content/browser.xul 1.359
mozilla/browser/components/places/content/editBookmarkOverlay.j 1.2
mozilla/browser/components/places/content/editBookmarkOverlay.xul 1.2
mozilla/browser/components/places/content/places.js 1.96
mozilla/browser/locales/en-US/chrome/browser/browser.dtd 1.63
mozilla/browser/locales/en-US/chrome/browser/places/editBookmarkOverlay.dtd 1.2
Comment 30 Michael Kraft [:morac] 2007-08-25 11:09:18 PDT
(In reply to comment #2)
> Double click on star:  All Bookmarks
> 
> Bookmarks Menu>Bookmark this Page: Bookmarks Menu
> 
> Accel-D: Bookmarks Menu
> 

Has this been implemented this way?  All 3 methods above default to "All Bookmarks" in Gecko/2007082505 Minefield/3.0a8pre (Windows).
Comment 31 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-25 13:28:15 PDT
Thanks, good catch; filed bug 393710.
Comment 32 u286998 2007-08-27 09:27:53 PDT
dismissing the dialog while the tag selector is expanded does not update the visual appearance of the collapse/expand button

>+  uninitPanel: function EIO_uninitPanel(aHideCollapsibleElements) {
>+    if (aHideCollapsibleElements) {
>+      // hide the folder tree if it was previously visible
>+      if (!this._folderTree.collapsed)
>+        this.toggleFolderTreeVisibility();
>+
>+      // hide the tag selector if it was previously visible
>+      var tagsSelector = this._element("tagsSelector");
>+      if (!tagsSelector.collapsed)
>+        tagsSelector.collapsed = true;

the last line should probably be this.toggleTagsSelector()
Comment 33 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-27 16:24:00 PDT
Created attachment 278477 [details] [diff] [review]
comment 33

 * comment 32
 * bug 393710
 * move the star icon out of the location bar
 * remove workarounds to bug 392512
 * remove workaround to bug 392397
 * fix some js errors in the overlay.
 * bug 393887
Comment 34 Dietrich Ayala (:dietrich) 2007-08-27 23:18:32 PDT
Comment on attachment 278477 [details] [diff] [review]
comment 33

>@@ -542,34 +542,29 @@ var gEditItemOverlay = {
>       // "All Bookmarks" root
>       if (container != PlacesUtils.placesRootId)
>         this._markFolderAsRecentlyUsed(container);
>     }
> 
>     // Update folder-tree selection
>     if (isElementVisible(this._folderTree)) {
>       var selectedNode = this._folderTree.selectedNode;
>-      if (selectedNode.itemId != container)
>+      //alert("selectedNode.itemId command: " + selectedNode.itemId);
>+      if (!selectedNode || selectedNode.itemId != container)
>         this._folderTree.selectFolders([container]);
>     }

remove the debugging line please

r=me otherwise.
Comment 35 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-28 14:46:03 PDT
Created attachment 278644 [details] [diff] [review]
comment 33, for checkin
Comment 36 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-28 14:49:43 PDT
mozilla/browser/base/content/browser-places.js 1.43
mozilla/browser/base/content/browser-sets.inc 1.101
mozilla/browser/base/content/browser.js 1.827
mozilla/browser/base/content/browser.xul 1.362
mozilla/browser/base/content/nsContextMenu.js 1.24
mozilla/browser/components/places/content/editBookmarkOverlay.js 1.4
mozilla/browser/components/places/content/editBookmarkOverlay.xul 1.3
mozilla/browser/components/places/content/utils.js 1.60
mozilla/browser/themes/pinstripe/browser/browser.css 1.67
mozilla/browser/themes/winstripe/browser/browser.css 1.82
Comment 37 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-08-28 14:50:48 PDT
Resolving, further work tbd. in follow-ups.
Comment 38 Alex Faaborg [:faaborg] (Firefox UX) 2007-08-30 01:11:58 PDT
Mano: the star needs to be placed inside the location bar, (as opposed to a separate button next to go) so that it lines up with the stars in auto-complete:

https://bugzilla.mozilla.org/attachment.cgi?id=277820
Comment 39 Tracy Walker [:tracy] 2007-09-14 09:08:57 PDT
verified with - Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091304 Minefield/3.0a8pre
Comment 40 Aronnax 2007-10-11 08:03:12 PDT
(In reply to comment #38)
> Mano: the star needs to be placed inside the location bar, (as opposed to a
> separate button next to go) so that it lines up with the stars in
> auto-complete:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=277820
> 

Hi, as far as i know is it decided to move the Star back inside of the location bar. Or is this changed? Would be nice to know, in particular some theme design options depends on it.
 
Cheers  
Comment 41 Alex Faaborg [:faaborg] (Firefox UX) 2007-10-12 18:28:25 PDT
>move the Star back inside of the location bar

We will pick up that change with the next iteration, but it doesn't need to hit M9.
Comment 42 u286998 2008-05-22 08:22:41 PDT
>the star needs to be placed inside the location bar, so that it lines up with the stars in auto-complete
Just noticed that the stars don't line up if there is no scrollbar, is that a problem/bug?
Comment 43 [:Aleksej] 2008-06-03 00:35:52 PDT
*** Bug 434694 has been marked as a duplicate of this bug. ***
Comment 44 Gervase Markham [:gerv] 2009-11-26 06:26:08 PST
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Note You need to log in before you can comment on or make changes to this bug.