Live Bookmarks should support Atom 1.0

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
--
enhancement
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: Jeff Schiller, Assigned: vlad)

Tracking

unspecified
x86
Windows XP
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050706 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050706 Firefox/1.0+

http://www.tbray.org/ongoing/When/200x/2005/07/14/Atom-1.0

According to Tim Bray, it's time for implementers to dig in...I suspect there
may already be a request out there for this but I could not find it.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Thanks to our indifference to namespaces, we're already 90% there: the only
thing I can see we're missing is support for <link href="..."/> -
rel="alternate" is now the default, and since it's absent in a good percentage
of the early example feeds in
http://www.intertwingly.net/wiki/pie/KnownAtomFeeds it's likely to wind up
absent in most feeds. If around
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp#699
we just test for rel="alternate" or no rel, I think we'll be as good with 1.0 as
we are with 0.3. Sam, anything else I'm missing that affects us?

But with our release timing, we really really need that one little change before
1.5 - the Atom community really wants to see 0.3 disappear, so by next year I'd
expect to see tens of millions more 1.0 feeds than 0.3 feeds.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.5?

Comment 2

13 years ago
Vlad, how difficult would it be to get this in for 1.5?
Flags: blocking-aviary1.5? → blocking1.8b4?

Comment 3

13 years ago
Titles also have a type attribute that indicate whether they are "text", "html"
or "xhtml".

If SeaMonkey expects plain text, then the content should be left alone,
unescaped, and stripped of all elements respectively.

If SeaMoney expects html, then the content should be escaped, left alone, and
something like the equivalent of innerXml should be used, again respectively.

If any of this is unclear, I'd be glad to write a test case.
What's SeaMonkey doing with titles?
SeaMonkey's waiting for us to port Live Bookmarks over to it, I'd guess ;)

I don't think we have any different problems with Atom 1.0 titles than our
existing (and, interestingly enough, unreported near as I can tell) problems
with Atom 0.3 titles with @type and @mode - either way it's the same situation:

type="text" we like,
type="html" we display <title>I &lt;b>really&lt;/b> like HTML</title> as "I
<b>really</b> like HTML"
type="xml" we fail on: for some reason, FindTextInNode fails to find anything in
<title type="xml"><div xmlns="http://www.w3.org/1999/xhtml">The title</div></title>

Comment 6

13 years ago
> type="html" we display <title>I &lt;b>really&lt;/b> like HTML</title> as "I
> <b>really</b> like HTML"

There are two common cases for HTML, and two common variations.

One is to use bold and italics as Phil did above.  These should be simply be
stripped.

Second case is to provide access to HTML's named entities.  "Bill de
h&amp;Oacute;ra" is an example.

The two variations are to directly escape, like you see in the examples above. 
Or to use CDATA, thus:

<![CDATA[Bill de h&Oacute;ra]]>
Hmm, we should probably have a separate bug on the title issue, because it's
pretty tricky -- the best we can ever do is to strip HTML entirely, otherwise we
open up ourselves to potential security holes.  (Why the heck is a "title"
allowed to be full HTML? or XML?  I can see a separate "displayTitle" or
something, but... ugh.)(In reply to comment #1)

> Thanks to our indifference to namespaces, we're already 90% there: the only
> thing I can see we're missing is support for <link href="..."/> -
> rel="alternate" is now the default, and since it's absent in a good percentage
> of the early example feeds in
> http://www.intertwingly.net/wiki/pie/KnownAtomFeeds it's likely to wind up
> absent in most feeds. If around
>
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp#699
> we just test for rel="alternate" or no rel, I think we'll be as good with 1.0 as
> we are with 0.3. Sam, anything else I'm missing that affects us?

I may be misremembering, but wasn't the default previously not "alternate", but
instead something else that shouldn't be used as a link?  (I vaguely remember
having code that just looked for a href attribute, and having to add explicit
rel="alternate" checking...)
(In reply to comment #7)
> wasn't the default previously not "alternate", but
> instead something else that shouldn't be used as a link?  (I vaguely remember
> having code that just looked for a href attribute, and having to add explicit
> rel="alternate" checking...)

There wasn't a default before, so that would have been the presence of rels
which aren't alternate: if you just take the first link you find, it might be
rel="alternate" or it might be rel="self" or anything else. Now, though,
(rel="alternate" | !rel) is the link we want.

I'll file a new bug on titles, and start the slew of testcases (with Sam cc'ed
to cover me) when I get home: I'd very much like to see link with no rel in
1.8b4 since it's clearly going to break us in a large share of 1.0 feeds, and I
fear we won't get the title part done in time.
Created attachment 190467 [details]
testcase for link @rel

Three <entry>s, defaulted and explicit @rel="alternate" that we should find,
@rel="self" but no @rel="alternate" that we shouldn't.
Filed bug 302133 for the title-escaping issues.

Comment 11

13 years ago
Created attachment 190544 [details] [diff] [review]
Fix that addresses the testcase Phil provided

Tested on Ubuntu 5.04

Comment 12

13 years ago
(In reply to comment #10)
> Filed bug 302133 for the title-escaping issues.

There probably should be a separate bug for relative URIs / xml:base support
Comment on attachment 190544 [details] [diff] [review]
Fix that addresses the testcase Phil provided

Twiddling review bits; it'll need checkin, too, since I don't think Sam's got
the permissions here that he does in Apache and PHP land.

Relative URLs is bug 262222, and the tricky xml:base testcases you've probably
already got would be most welcome.
Attachment #190544 - Flags: review?(vladimir)
Comment on attachment 190544 [details] [diff] [review]
Fix that addresses the testcase Phil provided

Not quite; other patch inc in a sec
Attachment #190544 - Flags: review?(vladimir) → review-
Created attachment 190711 [details] [diff] [review]
patch for empty/not specified rel

The rv test is gratuitious, as all the code truncates right now, but eh.
Attachment #190544 - Attachment is obsolete: true
Attachment #190711 - Flags: review?(shaver)
Attachment #190711 - Flags: approval1.8b4?
Comment on attachment 190711 [details] [diff] [review]
patch for empty/not specified rel

r+a=shaver
Attachment #190711 - Flags: review?(shaver)
Attachment #190711 - Flags: review+
Attachment #190711 - Flags: approval1.8b4?
Attachment #190711 - Flags: approval1.8b4+
Checked in slightly modified patch -- no rv check, because we'd never actually
hit it and it just confused the issue (and needed an extra include file to be
defined, etc.).
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+

Updated

7 years ago
Assignee: nobody → vladimir
You need to log in before you can comment on or make changes to this bug.