The default bug view has changed. See this FAQ.

Add support for separators to bookmarks API

ASSIGNED
Assigned to

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
ASSIGNED
8 months ago
a day ago

People

(Reporter: sheppy, Assigned: mstriemer, NeedInfo)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [design-decision-approved]triaged[bookmarks])

(Reporter)

Description

8 months ago
The WebExtensions bookmarks API currently doesn't let you create separators, and leaves separators out from result lists. These capabilities should be implemented.

Comment 1

8 months ago
Yeah, this seems like something we should do. I wonder if we should just implement this as bookmark nodes without titles or URLs, since folders are implemented with the same sort of hack: nodes with titles but no URLs. (Ugh.)
Whiteboard: [design-decision-needed]
Probably not, while testing the bookmarks API I came across extensions that assumed that no URL means a folder node.
(Reporter)

Comment 3

8 months ago
Yeah, unfortunately, since the "no URL means a folder" rule is already established (it's how both our and Google's docs say to identify a folder), that ship has probably sailed. We'll have to find another way.

Realistically, we should add a |type| field to the BookmarkTreeNode object which can be one of bookmarks.TYPE_BOOKMARK, bookmarks.TYPE_FOLDER, or bookmarks.TYPE_SEPARATOR. That could be set and provide the information without breaking existing add-ons which assume no URL means a folder, but we can then remove the recommendation to do it that way so that bad design pattern can fade away in favor of using the type.

Comment 4

8 months ago
Fair enough, but I don't think that solves the compatibility problem. Even if we add a type field (which I think is a good idea), separator nodes will still have no URL or title, and will still not be folders.
Keywords: dev-doc-needed

Updated

6 months ago
Whiteboard: [design-decision-needed] → [design-decision-needed]triaged
(Reporter)

Comment 5

6 months ago
(In reply to Kris Maglione [:kmag] from comment #4)
> Fair enough, but I don't think that solves the compatibility problem. Even
> if we add a type field (which I think is a good idea), separator nodes will
> still have no URL or title, and will still not be folders.

The title is easy enough: just report "-" as the title for separators; that actually has precedent here and there anyway. As for the URL, maybe you can do something like "data:text/plain;-". Attempts to open it just get a "-" character, but it's a valid URL.

Comment 6

4 months ago
To be discussed at Nov. 29, 2016 WE triage meeting. 

Agenda: https://docs.google.com/document/d/1IMBFXHNpg_A-15VdJM1Hh8DUUXF1xNFy87W1w8ZEOBk/edit?usp=sharing

Updated

4 months ago
Priority: -- → P3
Whiteboard: [design-decision-needed]triaged → [design-decision-approved]triaged
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED

Updated

3 months ago
Assignee: bob.silverberg → mstriemer
Whiteboard: [design-decision-approved]triaged → [design-decision-approved]triaged[bookmarks]

Comment 7

5 days ago
Regarding the two viable alternatives proposed in the comments above, I would not recommend title = "-" to indicate a separator, for the obvious reason that a user might use that as a legitimate name.  I just tried it, and it works.  I have seen many users assign short 1-3 character titles to folders in their Toolbar, so they can squeeze more into that horizontal space.

The other viable alternative, introducing a `type` property, should work.  I think Comment #4 may be regarding breaking extensions designed to the current WebExtensions API.  Indeed, on output (Firefox to Extension), separators are currently skipped.  So if there are any extensions designed to the current API, they will be getting an empty, untitled folder instead of nothing, which may be unexpected.  But how can you replace nothing with something and still have it recognized as nothing?  I think this is an unavoidable breakage.  On input, the absence of `type` should default to the current (Google Chrome) behavior.

But it seems shameful to introduce another property, a significant difference with Google's Chrome, just for this quite special case, so I'd offer a third viable alternative: Undefined `title` means Separator.

There are no ambiguities with undefined `title` on output.  If I delete the title of a bookmark or folder in Firefox, WebExtensions browser.bookmarks functions return a BookmarksTreeNode whose `title` is an empty string, not undefined.

On input, there is an ambiguity which would require a minor change to the API.  Currently, documentation of browser.bookmarks.create() says that the `title` property is optional, and if not given, the title of the new bookmark becomes an empty string.  Google Chrome documentation is more forgiving, though: it simply says that the `title` is optional but does not specify what happens if not given.  But I cannot think of how or why anyone would actually use the behavior as currently documented.  Creating a folder, or a bookmark, with no title kind of defeats the purpose.

All that being said, I can live with any of the three alternatives: undefined `title`, "-" `title`, or `type`, or any other viable alternative.  How do we get a design decision so we can proceed?
Flags: needinfo?(mstriemer)
(Reporter)

Comment 8

4 days ago
(In reply to Jerry Krinock from comment #7)

> On input, there is an ambiguity which would require a minor change to the
> API.  Currently, documentation of browser.bookmarks.create() says that the
> `title` property is optional, and if not given, the title of the new
> bookmark becomes an empty string.  Google Chrome documentation is more
> forgiving, though: it simply says that the `title` is optional but does not
> specify what happens if not given.  But I cannot think of how or why anyone
> would actually use the behavior as currently documented.  Creating a folder,
> or a bookmark, with no title kind of defeats the purpose.

Certainly the UI in Firefox automatically subs in the URL even if you specify an empty title, or try to leave it out entirely (such as bookmarking a page with no <title> and deleting the contents of the Name box in the new bookmark pane).

The no-title option is viable... but not necessarily clear.

> All that being said, I can live with any of the three alternatives:
> undefined `title`, "-" `title`, or `type`, or any other viable alternative. 
> How do we get a design decision so we can proceed?

Good question.

I feel like the `type` property addition is the wisest move for the long term; instead of being a hack or gaming an existing field, it's a clean solution that offers room to expand in the future if necessary. For instance, perhaps someday it's useful to have a type indicating that a bookmark is a "virtual" bookmark, generated by the browser to represent something useful, or a "cloud" bookmark, meaning one that's not on the device itself but is on the user's cloud account. Things like that. The best time to add something like a new property is now, before WebExtensions are any more widely used than they already are.

As someone who documents these things, and has to explain how they work, the `type` notion appeals to me greatly, as the solution which is the clearest to use. But, I agree that I could in principle live with any of those options.

Comment 9

4 days ago
Thank you, Eric.  I think you've stated the design tradeoffs quite well.  I would add that, although the new `type` property would simplify development of a new Firefox-only extension, one of the "gaming" alternatives would simplify development of a universal Firefox/Chrome/Opera extension, or updating an existing extension.

This morning, I thought of a fourth viable alternative which seems to have no conflicts with the existing API: Indicate Separator by assigning to `title` a JavaScript null.  On input, testing in the current nightly, passing an object with title = null to browser.bookmarks.create() or browser.bookmarks.update() causes the promise to reject with an "Invalid bookmark" message.  So we know no one is currently doing that.  To verify that this will work on output, I just patched one of the `convert()` functions in mozilla-central/browser/components/extensions/ext-bookmarks.js to return a null title for a certain bookmark.  It worked.  My extension got it and converted to a JSON null, as expected.  I just searched the API for all of the browser.bookmarks functions and, although null `url` has a meaning, none of them mention anything about null `title`.

We just need a decision now :)

Comment 10

a day ago
I hate to be a nag, but since Firefox 53 crashes when running my XUL-based extension which supports separators, I need a design decision on the API soon so that I can finish my WebExtension.  There are four alternatives, fairly well considered in the comments above.  I presume that someone in the CC list has the authority to make the decision.  I apologize if this is not an appropriate comment here.
You need to log in before you can comment on or make changes to this bug.