Closed Bug 385266 Opened 12 years ago Closed 12 years ago

New starring, bookmarking and tagging UI

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 alpha8

People

(Reporter: mano, Assigned: mano)

References

()

Details

(Keywords: uiwanted, Whiteboard: [places-ui])

Attachments

(8 files, 8 obsolete files)

134.82 KB, image/png
Details
3.19 KB, image/png
Details
3.00 KB, image/png
Details
86.52 KB, patch
Details | Diff | Splinter Review
5.72 KB, image/png
Details
24.48 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
43.25 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
25.94 KB, patch
Details | Diff | Splinter Review
Summary: New starting, bookmarking and starring UI → New starring, bookmarking and starring UI
Summary: New starring, bookmarking and starring UI → New starring, bookmarking and tagging UI
Blocks: 374524
Blocks: 387642
Depends on: 329261
Attached image 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.
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.
Attached patch checkpoint #2.1 (obsolete) — Splinter Review
Applies after ndef-MOZ_PLACES_BOOKMARKS removal.
Attachment #269509 - Attachment is obsolete: true
Attachment #272679 - Attachment is obsolete: true
Attached patch more... (obsolete) — Splinter Review
Attachment #272875 - Attachment is obsolete: true
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Whiteboard: [places-ui]
Blocks: 358210
Attached patch something to land, v1 (obsolete) — Splinter Review
there's _a lot_ more to do once this lands
Attachment #273355 - Attachment is obsolete: true
Attachment #276709 - Flags: review?(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.
Attachment #276709 - Flags: review?(dietrich) → review-
(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.
Attached patch more... (obsolete) — Splinter Review
Attachment #276709 - Attachment is obsolete: true
Attachment #276789 - Flags: review?(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.
Attachment #276789 - Flags: review?(dietrich) → review+
Mano: here are some temporary icons to use
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
Depends on: 392427
Depends on: 392443
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
(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 on attachment 277022 [details] [diff] [review]
more...

everything looks good except the placement issue. r=me otherwise.
Attachment #277022 - Flags: review?(dietrich) → review+
[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
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
Depends on: 392580
Attached patch more, wip (obsolete) — Splinter Review
- 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.
Attached patch another roundSplinter Review
see comment 23.
Attachment #277280 - Attachment is obsolete: true
Attachment #277317 - Flags: review?(dietrich)
Depends on: 392820
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?
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 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.
Attachment #277317 - Flags: review?(dietrich) → review+
> 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.
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
Blocks: 389252
Depends on: 393446
Depends on: 393546
(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).
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()
Depends on: 393887
Attached patch comment 33 (obsolete) — Splinter Review
* 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
Attachment #278477 - Flags: review?(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.
Attachment #278477 - Flags: review?(dietrich) → review+
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
Resolving, further work tbd. in follow-ups.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Priority: -- → P2
Resolution: --- → FIXED
Depends on: 394159
Depends on: 394162
Depends on: 394088
Depends on: 394085
Depends on: 394089
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
Depends on: 393253, 393257
Depends on: 394605
verified with - Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091304 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Depends on: 398011
Depends on: 394450
Depends on: 398409
Blocks: 329261
(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  
>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.
Depends on: 393239
No longer depends on: 385274
No longer blocks: 405017
Depends on: 413483
Depends on: 435179
Version: unspecified → Trunk
>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?
Duplicate of this bug: 434694
Depends on: 441326
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.