Add support for separators to bookmarks API

ASSIGNED
Assigned to
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
ASSIGNED
10 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

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

Comment 2

10 months ago
Probably not, while testing the bookmarks API I came across extensions that assumed that no URL means a folder node.
(Reporter)

Comment 3

10 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.
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

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

Comment 5

8 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.
To be discussed at Nov. 29, 2016 WE triage meeting. 

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

Updated

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

Updated

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

Comment 7

2 months 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

2 months 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

2 months 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

2 months 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.
(Assignee)

Comment 11

2 days ago
I think making it look like a bookmark is likely the best way to go here. It should be compatible with existing extensions as far as not breaking them goes, but they might not behave correctly for separators.

Bookmark: { title: "My bookmark", url: "http://example.com", type: bookmarks.TYPE_BOOKMARK }
Folder: { title: "My folder", type: bookmarks.TYPE_FOLDER }
Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }

I figure the types will be Symbols and using bookmarks.Bookmark, bookmarks.Folder, bookmarks.Separator looks nicer to me but could cause some naming collisions? The values for `title` and `url` can be something else but they should be present to make it look like a bookmark.

Does this seem good to implement, Kris?
Flags: needinfo?(mstriemer) → needinfo?(kmaglione+bmo)
(Assignee)

Comment 12

a day ago
Bob, does the above API seem good to you?
Flags: needinfo?(bob.silverberg)
(In reply to Mark Striemer [:mstriemer] from comment #11)
> I think making it look like a bookmark is likely the best way to go here. It
> should be compatible with existing extensions as far as not breaking them
> goes, but they might not behave correctly for separators.
> 
> Bookmark: { title: "My bookmark", url: "http://example.com", type:
> bookmarks.TYPE_BOOKMARK }
> Folder: { title: "My folder", type: bookmarks.TYPE_FOLDER }
> Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }
> 

If we're going to add a type, which I think is a good idea, then we don't need a title for separators, do we? I think the suggestion of using a "-" was in case we wanted to be able to create a separator using the existing method signature, but if we introduce type then it becomes unnecessary. We can just return an empty string for title.

> I figure the types will be Symbols and using bookmarks.Bookmark,
> bookmarks.Folder, bookmarks.Separator looks nicer to me but could cause some
> naming collisions? 

We can introduce a new enumerated type for the bookmarkType in bookmarks.json and use "friendlier" names like "bookmark", "folder" and "separator". I'm not sure what sorts of collisions you are anticipating.

For the create() function, as suggested above, we can make type optional and if it is missing assume that no url means a folder, just as the code does now.
Flags: needinfo?(bob.silverberg)
You need to log in before you can comment on or make changes to this bug.