385266 - New starring, bookmarking and tagging UI

Status: VERIFIED FIXED

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

Description

[Asaf Romano (gone)]

Comment 1

[Asaf Romano (gone)]

Attachment: checkpoint

Comment 2

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

Attachment: 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]

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

[Asaf Romano (gone)]

Attachment: checkpoint #2

Comment 5

[Asaf Romano (gone)]

Attachment: checkpoint #2.1

Applies after ndef-MOZ_PLACES_BOOKMARKS removal.

Comment 6

[Asaf Romano (gone)]

Attachment: more...

Comment 7

[Asaf Romano (gone)]

Attachment: something to land, v1

there's _a lot_ more to do once this lands

Comment 8

[Dietrich Ayala (:dietrich)]

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

[Asaf Romano (gone)]

(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

[Asaf Romano (gone)]

Attachment: more...

Comment 11

[Dietrich Ayala (:dietrich)]

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)]

Attachment: Temporary Starred icon

Comment 13

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

Attachment: Temporary Un-starred icon

Comment 14

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

Mano: here are some temporary icons to use

Comment 15

[Asaf Romano (gone)]

Attachment: part 1, for checkin

Comment 16

[Asaf Romano (gone)]

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)]

Attachment: 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

[Asaf Romano (gone)]

Attachment: more...

Comment 19

[Dietrich Ayala (:dietrich)]

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

Comment on attachment 277022 [details] [diff] [review]
more...

everything looks good except the placement issue. r=me otherwise.

Comment 21

[Asaf Romano (gone)]

[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

[Asaf Romano (gone)]

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

[Asaf Romano (gone)]

Attachment: 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

[Asaf Romano (gone)]

Attachment: another round

see comment 23.

Comment 25

[Alfred Kayser]

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]

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)]

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

[Asaf Romano (gone)]

> 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

[Asaf Romano (gone)]

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]]

(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

[Asaf Romano (gone)]

Thanks, good catch; filed bug 393710.

Comment 32

[u286998]

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

[Asaf Romano (gone)]

Attachment: 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)]

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

[Asaf Romano (gone)]

Attachment: comment 33, for checkin

Comment 36

[Asaf Romano (gone)]

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

[Asaf Romano (gone)]

Resolving, further work tbd. in follow-ups.

Comment 38

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

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]]

verified with - Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091304 Minefield/3.0a8pre

Comment 40

[Aronnax]

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

>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]

>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 44

[Gervase Markham [:gerv]]

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